gRPC Trailers-Only response doesn't mark a streaming request as finished #2313
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
Motivation: If server returns a `Trailers-Only` response for a streaming request, we never drain the response message-body, because `concat` never subscribes to the next source if the first source fails. As the result, concurrency controller won't mark the request as finished. If the request publisher never completes we will start leaking connections. Modifications: - Enhance `TrailersOnlyErrorTest` to cover blocking client and make sure the HTTP response terminates; - In case of an error in `Trailers-Only` response, subscribe and cancel the response message body to abort the request; - Always check that `Trailers-Only` response doesn't have any further frames on the wire; Result: gRPC response correctly terminates even if client receives a `Trailers-Only` response.
tkountis
reviewed
Aug 10, 2022
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Completable.java
Outdated
Show resolved
Hide resolved
tkountis
reviewed
Aug 10, 2022
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcUtils.java
Outdated
Show resolved
Hide resolved
|
excellent find 🎸 |
tkountis
approved these changes
Aug 10, 2022
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.
Motivation:
If server returns a
Trailers-Onlyresponse for a streaming request, wenever drain the response message-body, because
concatnever subscribesto the next source if the first source fails. As the result, concurrency
controller won't mark the request as finished. If the request publisher
never completes we will start leaking h2 connections because streams
won’t be closed.
Modifications:
TrailersOnlyErrorTestto cover blocking client and make surethe HTTP response terminates;
Trailers-Onlyresponse, subscribe and cancelthe response message body to abort the request;
Trailers-Onlyresponse doesn't have any furtherframes on the wire;
Result:
gRPC response correctly terminates even if client receives a
Trailers-Onlyresponse.