Reconcile trailers-only and misc error behavior with grpc-go #690
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
force-pushed
the
jh/revisit-trailers-only
branch
from
80fba22 to
381995b
Compare
* This largely undoes a recent change to do more validation of trailers-only responses, which would disallow a body or trailers in what appeared to be a trailers-only response. * Instead, a trailers-only response is now defined by the lack of body and trailers, not the presence of a "grpc-status" header. * This also tweaks some other error scenarios. If trailers (or an end-stream message) is completely missing from a response, it's considered an internal error. But if trailers are present, but the "grpc-status" key is missing, it's considered an issue determining the status, which is an unknown error. * Similarly, if a response content-type doesn't appear to be the right protocol (like it may have come from a non-RPC server), the error code is now unknown. But if it looks like the right protocol but uses the wrong sub-format/codec, it's an internal error. * This is also now more strict about the "compressed" flag in a streaming protocol when there was no compression algorithm negotiated. This was previously not considered an error if the message in question was empty (zero bytes).
jhump
force-pushed
the
jh/revisit-trailers-only
branch
from
21dd00f to
4d6745d
Compare
emcfarlane
approved these changes
Feb 16, 2024
akshayjshah
changed the title
akshayjshah
reviewed
Feb 16, 2024
akshayjshah
reviewed
Feb 16, 2024
jhump
commented
Feb 16, 2024
| @@ -2442,97 +2483,6 @@ func TestClientDisconnect(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestTrailersOnlyErrors(t *testing.T) { | |||
There was a problem hiding this comment.
These tests were for the old classification and no longer apply.
For example, they'd check that if we classify a response as trailers-only (due to presence of grpc-status key), then the client complains (in the form of an RPC error with "internal" code) if there is a non-empty body or non-empty HTTP trailers.
But since we now define trailers-only to be a request without body or HTTP trailers, they are moot. In these situations (where a response has a grpc-status key in headers and a non-empty body) we now just ignore the grpc-status key in headers (which is also what grpc-go does).
akshayjshah
reviewed
Feb 16, 2024
akshayjshah
approved these changes
Feb 16, 2024
Co-authored-by: Akshay Shah <akshayjshah@users.noreply.github.com>
Merged
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.
This largely undoes a recent change to do more validation of trailers-only responses (#685), which disallows a body or trailers in what appeared to be a trailers-only response. In that change, a trailers-only response was identified by the presence of a "grpc-status" key in the headers.
In this PR, a trailers-only response is instead defined by the lack of body and trailers (not the presence of a "grpc-status" header). This PR also tweaks some other error scenarios:
internalerror. But if trailers are present, but the "grpc-status" key is missing, it's considered an issue determining the status, which is anunknownerror.unknown. But if it looks like the right protocol but uses the wrong sub-format/codec, it's aninternalerror.This PR also now makes connect-go more strict about the "compressed" flag in a streaming protocol when there was no compression algorithm negotiated. Previously, this library was lenient and did not consider it an error if the message in question was empty (zero bytes). But to correctly adhere to gRPC semantics, it MUST report this case as an
internalerror (see bullet 6 in the test cases section of the gRPC compression docs).