Don't close entire HTTP/2 connection when request is cancelled #1307

idelpivnitskiy · 2021-01-08T21:39:32Z

Motivation:

LoadBalancedStreamingHttpClient pessimistically assumes the connection
that serves a request should be closed on request cancellation. However,
in case of HTTP/2 it closes the parent TCP connection instead of a stream
(because stream is not accessible from the connection API). It also fails
all other in-flight requests on the same connection.

Modifications:

In case of HTTP/2 and above mark request as finished instead of closing
the connection when cancellation happens;
Test that request cancellation doesn't close parent TCP connection for
HTTP/2 protocol;
Change HttpConnectionContext#protocol() to return HttpProtocolVersion;
Change GrpcServiceContext.GrpcProtocol#httpProtocol() to return
HttpProtocolVersion;

Result:

HTTP/2 request cancellation doesn't close the TCP connection.

Motivation: `LoadBalancedStreamingHttpClient` pessimistically assumes the connection that serves a request should be closed on request cancellation. However, in case of HTTP/2 it closes the parent TCP connection instead of a stream (because stream is not accessible from the connection API). It also fails all other in-flight requests on the same connection. Modifications: - In case of HTTP/2 and above mark request as finished instead of closing the connection when cancellation happens; - Test that request cancellation doesn't close parent TCP connection for HTTP/2 protocol; Result: HTTP/2 request cancellation doesn't close the TCP connection.

idelpivnitskiy · 2021-01-08T21:42:21Z

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/H2ResponseCancelTest.java

+                .then(() -> {
+                    try {
+                        secondRequestReceivedLatch.await();
+                        cancellable.get().cancel(); // If I use thenCancel() the current then(Runnable) does not run


@Scottmitch if I do this way:

stepVerifier(requester.request(defaultStrategy(), newRequest(requester, "second")) .whenOnSuccess(responses::add)) // Add response to the queue to verify that we never receive it .expectCancellable() .then(() -> { try { secondRequestReceivedLatch.await(); } catch (InterruptedException e) { // ignore } }) .thenCancel() .verify();

then(Runnable) is not invoked, after expectCancellable() the next step will be thenCancel(). Is this expected behavior?

discussed offline, we will investigate clarifying/improving as part of followup issues. tldr; the "then" runnable operations are run on the verifier thread, the expectations are run on the subscriber thread (including thenCancel), so these aren't sequential.

Scottmitch · 2021-01-12T23:38:14Z

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/H2ResponseCancelTest.java

+                // .thenCancel()
+                .verify();
+
+        assertThat("Unexpected responses", responses, hasSize(0));


nit: hasSize(0) -> is(empty())

Scottmitch · 2021-01-12T23:38:53Z

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/H2ResponseCancelTest.java

+        firstResponseLatch.countDown();
+
+        assertResponse(responses.take(), HTTP_2_0, OK, "first");
+        // Mak sure we can use the same connection for future requests:


nit: Mak -> Make

Scottmitch · 2021-01-12T23:40:07Z

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/H2ResponseCancelTest.java

+        requestCancellationResetsStreamButNotParentConnection(streamingHttpClient());
+        int connectionsAfter = newConnectionsCounter.get();
+        assertThat("Client unexpectedly created more connections instead of reusing the existing one",
+                connectionsAfter - connectionsBefore, is(0));


nit: assertThat(connectionsAfter, is(connectionsBefore)); or assertEquals(connectionsBefore, connectionsAfter) will give a more informative error message if a failure occurs.

Motivation: `H2StreamResetException` was moved in apple#1304, but used old location in apple#1307. Those PRs were merged concurrently without updating the import. Modifications: - Update import for `H2StreamResetException` in `H2ResponseCancelTest`; Result: Build does not fail.

Motivation: `H2StreamResetException` was moved in #1304, but used old location in #1307. Those PRs were merged concurrently without updating the import. Modifications: - Update import for `H2StreamResetException` in `H2ResponseCancelTest`; Result: Build does not fail.

Motivation: In apple#1307 we changed `LoadBalancedStreamingHttpClient` to not close the connection on cancel for HTTP/2 protocol, because our existing API does not expose methods for closing h2 stream instead of h2 connection. Instead, we had to _prematurely_ mark the request as finished, because in case of cancellation there is no guarantee we will receive a terminal signal on the response. Also, the stream is not aware of `LoadBalancedStreamingHttpConnection` API and therefore it can not mark the request as finished when it closes the stream. That change significantly increased a probability users see "Maximum active streams violated for this endpoint." Even though it's a `RetryableException` it creates pain and still may pop up after auto-retry strategy used all attempts. Modification: - Introduce `OwnedRunnable` that can be passed to the transport layer with the request object; - Add pkg-private `StreamingHttpRequestWithContext` wrapper which delivers `OwnedRunnable` to the transport; - Generate that `OwnedRunnable` in `LoadBalancedStreamingHttpClient`; - `LoadBalancedStreamingHttpClient` marks the request as finished only if it owns `OwnedRunnable`; - If h2-transport takes ownership of the `OwnedRunnable`, `LoadBalancedStreamingHttpClient` is not responsible for marking the request as finished anymore; - Add tests to verify the fix; Result: HTTP/2 requests marked as finished only before they reach transport or only after the stream closes.

…nt level (#1373) Motivation: In #1307 we changed `LoadBalancedStreamingHttpClient` to not close the connection on cancel for HTTP/2 protocol, because our existing API does not expose methods for closing h2 stream instead of h2 connection. Instead, we had to _prematurely_ mark the request as finished, because in case of cancellation, there is no guarantee we will receive a terminal signal on the response. Also, the stream is not aware of `LoadBalancedStreamingHttpConnection` API and therefore it can not mark the request as finished when it closes the stream. That change significantly increased the probability users see "Maximum active streams violated for this endpoint." Even though it's a `RetryableException`, it creates pain and still may pop up after auto-retry strategy used all attempts. Modification: - Introduce `OwnedRunnable` that can be passed to the transport layer with the request object; - Add pkg-private `StreamingHttpRequestWithContext` wrapper which delivers `OwnedRunnable` to the transport; - Generate that `OwnedRunnable` in `LoadBalancedStreamingHttpClient`; - `LoadBalancedStreamingHttpClient` marks the request as finished only if it owns `OwnedRunnable`; - If h2-transport takes ownership of the `OwnedRunnable`, `LoadBalancedStreamingHttpClient` is not responsible for marking the request as finished anymore; - Add tests to verify the fix; Result: HTTP/2 requests marked as finished only before they reach transport or only after the stream closes.

idelpivnitskiy requested review from tkountis and Scottmitch January 8, 2021 21:39

idelpivnitskiy commented Jan 8, 2021

View reviewed changes

Revert changes for ResponseCancelTest

ab9229d

Scottmitch approved these changes Jan 12, 2021

View reviewed changes

idelpivnitskiy added 2 commits January 12, 2021 15:59

Address comments

6e1579e

Merge remote-tracking branch 'upstream/main' into h2-cancel

1ba1579

idelpivnitskiy mentioned this pull request Jan 13, 2021

Flaky test: io.servicetalk.http.router.jersey.CancellationTest.cancelRsStreams #999

Closed

idelpivnitskiy merged commit 1e88cfd into apple:main Jan 13, 2021

idelpivnitskiy deleted the h2-cancel branch January 13, 2021 00:20

idelpivnitskiy mentioned this pull request Jan 13, 2021

Update import for H2StreamResetException #1313

Merged

idelpivnitskiy mentioned this pull request Feb 12, 2021

Avoid "Maximum active streams violated for this endpoint" at the client level #1373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't close entire HTTP/2 connection when request is cancelled #1307

Don't close entire HTTP/2 connection when request is cancelled #1307

idelpivnitskiy commented Jan 8, 2021 •

edited

Loading

idelpivnitskiy Jan 8, 2021

Scottmitch Jan 12, 2021

Scottmitch Jan 12, 2021

Scottmitch Jan 12, 2021

Scottmitch Jan 12, 2021

Don't close entire HTTP/2 connection when request is cancelled #1307

Don't close entire HTTP/2 connection when request is cancelled #1307

Conversation

idelpivnitskiy commented Jan 8, 2021 • edited Loading

idelpivnitskiy Jan 8, 2021

Choose a reason for hiding this comment

Scottmitch Jan 12, 2021

Choose a reason for hiding this comment

Scottmitch Jan 12, 2021

Choose a reason for hiding this comment

Scottmitch Jan 12, 2021

Choose a reason for hiding this comment

Scottmitch Jan 12, 2021

Choose a reason for hiding this comment

idelpivnitskiy commented Jan 8, 2021 •

edited

Loading