Some fixes noticed when testing against 'main' of conformance #252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhump
commented
Apr 1, 2024
Comment on lines
-162
to
-163
| val responseHeaders = | ||
| result.headers.filter { entry -> !entry.key.startsWith("trailer-") } |
There was a problem hiding this comment.
This filtering was not correct. Only unary RPCs use a "trailer-" suffix to distinguish headers and trailers. In streaming RPCs, everything in the response headers should be used as-is.
pkwarren
approved these changes
Apr 2, 2024
pkwarren
pushed a commit
that referenced
this pull request
Apr 19, 2024
We are hoping to cut a _final_ v1.0.0 of the conformance suite this week. It has a couple of minor changes from rc4. I wanted to make sure they wouldn't cause issues (particularly, [this pending PR](connectrpc/conformance#840), which could possibly impact some of the logic I just added in #248 related to reporting trailers and merging headers+trailers in error metadata). And... this ended up exposing some issues. For one, I had not updated the Connect stream protocol in the same way as the gRPC-Web and gRPC protocols regarding merging of headers+trailers in metadata. I didn't notice in the previous PR because it was actually the "trailers only" responses that tickled test failures related to this (the connect-go reference server will only use "trailers only" responses for gRPC-Web, not for other protocols). For two, this code path was not correctly setting the trailers of the `StreamResult.Completed` object. Oops 🤦. After I fixed that, I just happened to observe a conformance test flake that caused the client to hang. It turned out to be due to uncaught exception while trying to drain the response body in a unary RPC. So that's what warranted the other changes in here, adding `try/catch` to the unary flow of `ConnectOkHttpClient` and extracting the `safeTrailers` helper so it can also be used from that unary flow. While making the above change, I greatly simplified the `safeTrailers` helper because it seemed to be incorrect: it was almost always computing empty trailers in these unary flows (which causes issues for gRPC calls, which rely on trailers to convey the status code). After re-reading the code, it's not clear why it was wrong, and I didn't have the patience to try to put it in a debugger to step through it and understand it. I just simplified it, removing the questionable conditional, and that fixed it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We are hoping to cut a final v1.0.0 of the conformance suite this week. It has a couple of minor changes from rc4. I wanted to make sure they wouldn't cause issues (particularly, this pending PR, which could possibly impact some of the logic I just added in #248 related to reporting trailers and merging headers+trailers in error metadata).
And... this ended up exposing some issues.
For one, I had not updated the Connect stream protocol in the same way as the gRPC-Web and gRPC protocols regarding merging of headers+trailers in metadata. I didn't notice in the previous PR because it was actually the "trailers only" responses that tickled test failures related to this (the connect-go reference server will only use "trailers only" responses for gRPC-Web, not for other protocols). For two, this code path was not correctly setting the trailers of the
StreamResult.Completedobject. Oops 🤦.After I fixed that, I just happened to observe a conformance test flake that caused the client to hang. It turned out to be due to uncaught exception while trying to drain the response body in a unary RPC. So that's what warranted the other changes in here, adding
try/catchto the unary flow ofConnectOkHttpClientand extracting thesafeTrailershelper so it can also be used from that unary flow.While making the above change, I greatly simplified the
safeTrailershelper because it seemed to be incorrect: it was almost always computing empty trailers in these unary flows (which causes issues for gRPC calls, which rely on trailers to convey the status code). After re-reading the code, it's not clear why it was wrong, and I didn't have the patience to try to put it in a debugger to step through it and understand it. I just simplified it, removing the questionable conditional, and that fixed it.