Handle HTTP/1.1 response cancelation same way at all levels #2266
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
|
@Scottmitch this is an alternative approach for #2263 to consistently handle cancellation from any operator at any level |
idelpivnitskiy
changed the title
Scottmitch
approved these changes
Jul 14, 2022
...alk-http-netty/src/main/java/io/servicetalk/http/netty/PipelinedStreamingHttpConnection.java
Outdated
Show resolved
Hide resolved
...alk-http-netty/src/main/java/io/servicetalk/http/netty/PipelinedStreamingHttpConnection.java
Outdated
Show resolved
Hide resolved
...-http-netty/src/main/java/io/servicetalk/http/netty/NonPipelinedStreamingHttpConnection.java
Outdated
Show resolved
Hide resolved
servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ResponseCancelTest.java
Outdated
Show resolved
Hide resolved
Motivation: When response cancel is generated at client level, `LoadBalancedStreamingHttpClient` has a logic in place to close the connection to mitigate a race between connection availability and closure. However, this logic is skipped if cancel is generated from connection-level filter. Modifications: - Move connection closure from `LoadBalancedStreamingHttpClient` to `PipelinedStreamingHttpConnection`; - Adjust tests and comments; Result: HTTP/1.1 connection is always closed in case of cancellation. We do that from the caller thread to proactively set `onClosing` state.
|
@Scottmitch I've changed the approach. Instead of splitting logic for pipelined/non-pipelined cases, cancellation is handled in |
Scottmitch
approved these changes
Jul 26, 2022
...talk-http-netty/src/main/java/io/servicetalk/http/netty/AbstractStreamingHttpConnection.java
Outdated
Show resolved
Hide resolved
| // BeforeFinallyHttpOperator. | ||
| // 4. Doing it before offloading of terminal signals helps to reduce the risk of closing a connection | ||
| // after response terminates. | ||
| // We use beforeFinally instead of beforeCancel to avoid closing connection after response terminates. |
There was a problem hiding this comment.
thank you for these comments 🔥
servicetalk-http-utils/src/main/java/io/servicetalk/http/utils/TimeoutHttpRequesterFilter.java
Show resolved
Hide resolved
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Aug 29, 2022
Motivation: apple#2266 (released in 0.42.13) introduces a race between cancel marking a connection as "closing" (requires a hop to the event-loop), followed by propagation of `onError` (on the cancel thread) that marks the request as "finished", and the next request selecting the same connection. This only affects rare users who cancel at the connection level, users who use `TimeoutHttpRequesterFilter` as `appendClientFilter` are not affected. Modifications: - Move `onClosing` to `NettyChannelListenableAsyncCloseable`; - Notify `onClosing` asap for `channelInactive` event; - Log a warning if `AbstractLBHttpConnectionFactory` can not access `onClosing` (users incorrectly wrapped a connection); - Log a warning if `LoadBalancedStreamingHttpClient` takes ownership of the `OnStreamClosedRunnable` (users prevented propagation of request context); - Don't do anything with HTTP/2 connection in `AbstractStreamingHttpConnection`; - Improve comments for concurrency control in `LoadBalancedStreamingHttpClient`, `AbstractStreamingHttpConnection`, and `H2ClientParentConnectionContext`; - Add a test for described issue; Result: No race for HTTP/1.x users who cancel at the connection level.
idelpivnitskiy
added a commit
that referenced
this pull request
Aug 30, 2022
…ter (#2331) Motivation: #2266 (released in 0.42.13) introduces a race between cancel marking a connection as "closing" (requires a hop to the event-loop), followed by propagation of `onError` (on the cancel thread) that marks the request as "finished", and the next request selecting the same connection. This only affects rare users who cancel at the connection level, users who use `TimeoutHttpRequesterFilter` as `appendClientFilter` are not affected. Modifications: - Move `onClosing` to `NettyChannelListenableAsyncCloseable`; - Notify `onClosing` asap for `channelInactive` event; - Log a warning if `AbstractLBHttpConnectionFactory` can not access `onClosing` (users incorrectly wrapped a connection); - Log a warning if `LoadBalancedStreamingHttpClient` takes ownership of the `OnStreamClosedRunnable` (users prevented propagation of request context); - Don't do anything with HTTP/2 connection in `AbstractStreamingHttpConnection`; - Improve comments for concurrency control in `LoadBalancedStreamingHttpClient`, `AbstractStreamingHttpConnection`, and `H2ClientParentConnectionContext`; - Add a test for described issue; Result: No race for HTTP/1.x users who cancel at the connection level.
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:
When response cancel is generated at the client level,
LoadBalancedStreamingHttpClienthas a logic in place to close theconnection to mitigate a race between connection availability and
closure. However, this logic is skipped if cancellation is originated from
a connection-level filter.
Modifications:
LoadBalancedStreamingHttpClienttoAbstractStreamingHttpConnection;OnStreamClosedRunnableusingrequest.context();StreamingHttpRequestWithContext;TimeoutHttpRequesterFilterintroduced in
TimeoutHttpRequesterFilteras a connection filter causesClosedChannelException#2263;Result:
Request to cancel is intercepted after all user-defined filters to make sure
we catch all sources of cancellation.