HTTP StreamingService to BlockingStreaming backpressure and error propagation fixes #1328
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
…pagation fixes Motivation: StreamingHttpServiceToBlockingStreamingHttpService (used by HttpJerseyRouterBuilder.buildBlockingStreaming) doesn't apply any backpressure and also may not propagate error status correctly in the event the StreamingHttpResponse fails. Modifications: - PayloadPump uses the new PayloadWriter.close(Throwable) method to propagate status in the event the StreamingHttpResponse fails. - PayloadPump requests in chunks of 64 by default to apply backpresusre. That way if the blocking PayloadWriter APIs block, the source providing data will be limited to 64 chunks at most. Result: HttpApiConversions.toBlockingStreamingHttpService(StreamingHttpService) applies backpressure and propagates error status to the PayloadWriter.
tkountis
reviewed
Jan 20, 2021
| this.cancel(); | ||
| payloadWriter.write((Buffer) bufferOrTrailers); | ||
| } catch (IOException e) { | ||
| throwException(e); |
There was a problem hiding this comment.
Doesn't this need onError
There was a problem hiding this comment.
If onNext throws the async source is responsible for catching the exception and propagating an error back downstream. Throwing the exception also effectively cancels the subscription and lets the upstream source cleanup.
We generally don't throw from terminal methods (e.g. onComplete(), onError(Throwable)) because the upstream source isn't allowed to invoke terminal methods more than once, so we must handle the error locally.
...rc/main/java/io/servicetalk/http/api/StreamingHttpServiceToBlockingStreamingHttpService.java
Outdated
Show resolved
Hide resolved
|
build failure attributed to #999 |
…isherAsBlockingIterable
|
build failure attributed to #1323 |
|
build failure attributed to #1192 |
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:
StreamingHttpServiceToBlockingStreamingHttpService (used by
HttpJerseyRouterBuilder.buildBlockingStreaming) doesn't apply any
backpressure and also may not propagate error status correctly in the
event the StreamingHttpResponse fails.
Modifications:
propagate status in the event the StreamingHttpResponse fails.
That way if the blocking PayloadWriter APIs block, the source
providing data will be limited to 64 chunks at most.
Result:
HttpApiConversions.toBlockingStreamingHttpService(StreamingHttpService)
applies backpressure and propagates error status to the PayloadWriter.