HTTP client request cancellation #714
Conversation
DerGuteMoritz
changed the title
|
Completion of this patch depends on how the Netty issue linked above will be resolved. |
|
Some impressive spelunking!
Would we not still want to do this even if Netty doesn't want to change this for v4? After all, persistent conns are pretty common, so canceling the initial |
You can say that again 🙈
Yeah, the in-flight cancellation on its own would be good to have already, indeed. We could of course already merge this as-is and defer the connection establishment cancellation to a follow-up. I propose to include one more change, though - commit coming up! |
DerGuteMoritz
force-pushed
the
http-client-request-cancellation
branch
from
49716a1 to
9e27cae
Compare
test/aleph/http_test.clj
Outdated
| (deftest test-in-flight-request-cancellation | ||
| (let [conn-established (promise) | ||
| conn-closed (promise)] | ||
| (with-tcp-server (fn [s _] | ||
| (deliver conn-established true) | ||
| ;; Required for the client close to be detected | ||
| (s/consume identity s) | ||
| (s/on-closed s (fn [] | ||
| (deliver conn-closed true)))) | ||
| (let [rsp (http-get "/")] | ||
| (is (= true (deref conn-established 1000 :timeout))) | ||
| (http/cancel-request! rsp) | ||
| (is (= true (deref conn-closed 1000 :timeout))) | ||
| (is (thrown? RequestCancellationException (deref rsp 1000 :timeout))))))) |
There was a problem hiding this comment.
I'd like to see more testing, for both normal and error conditions. Maybe via overriding the conn-pool in some cases.
What's all the TCP handler/server/etc stuff get us? Why not the the HTTP code?
There was a problem hiding this comment.
I'd like to see more testing, for both normal and error conditions.
You mean for the case where cancel-request! is invoked after the request had already completed successfully or due to some other error?
What's all the TCP handler/server/etc stuff get us? Why not the the HTTP code?
IMHO using a raw TCP server which only accepts and holds the connection is the cleanest way to reliably test whether cancellation closes the underlying TCP connection (via the s/on-closed handler). One could probably concoct something like it with a raw-stream? HTTP request handler but I don't think it adds much value, does it?
There was a problem hiding this comment.
I'll give the latter idea a shot in any case!
There was a problem hiding this comment.
OK, the test uses the HTTP server(s) now, works just as well and probably causes less confusion 👍
This patch changes `aleph.http/request` so that setting the response deferred to an error status will terminate an in-flight request. This allows e.g. for `d/timeout!` to be used without potentially leaking connections. For convenient explicit cancellation, we provide `aleph.http/cancel-request!`. It sets the given response deferred to error with an instance of the new `aleph.utils.RequestCancellationException`. Closes #712.
The generic "connection failed" catch handler is redundant with the one on the `result` deferred. The same is the case for the logging in the "connection timeout" catch handler.
This works just as good as the raw TCP server but hopefully doesn't make future readers have to do a double-take.
DerGuteMoritz
force-pushed
the
http-client-request-cancellation
branch
from
75c4cbf to
810aad7
Compare
With #714 we added support for cancelling in-flight HTTP requests by putting the response deferred into an error state. However, this only worked once the underlying TCP connection was established. With this patch, it is now possible to cancel requests even while the connection is still being established (possible since Netty 4.1.108.Final via netty/netty#13849). This also works for `aleph.tcp/client`.
With #714 we added support for cancelling in-flight HTTP requests by putting the response deferred into an error state. However, this only worked once the underlying TCP connection was established. With this patch, it is now possible to cancel requests even while the connection is still being established (possible since Netty 4.1.108.Final via netty/netty#13849). This also works for `aleph.tcp/client`.
This patch changes
aleph.http/requestso that setting the response deferred to an error status will terminate an in-flight request. This allows e.g. ford/timeout!to be used without potentially leaking connections.For convenient explicit cancellation, we provide
aleph.http/cancel-request!. It puts the given response deferred into error state with an instance of the newaleph.utils.RequestCancellationException.Closes #712.
Review notes:
The patch uses the approach outlined in Cancellation API as a first-class citizen? manifold#167 (comment) for propagating cancellation.(not quite, it was merely an inspiration)Still marked as draft because the patch only pertains to in-flight requests. Connection establishment is not yet cancellable. Combined with the default-- deferred to a follow-up patch once Netty has responded.connection-timeoutof 60s, this can make for an unpleasant surprise 😬