Support cancellation during client connection establishment #721
Conversation
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`.
DerGuteMoritz
force-pushed
the
client-connection-establishment-cancellation
branch
from
4923350 to
9a66ceb
Compare
Thank you for your feedback so far! I've addressed it all but am not yet satisfied with the current state. I'll see if I can somehow make the error propagation a bit easier to grok, possibly by extracting a util function as discussed above. Stay tuned. |
The new namespace `aleph.util` introduces two new helpers: * `on-error` which is like `d/on-realized` but only for the error case (success case uses `identity`). * `propagate-error` which propagates error states from a source deferred to a destination deferred and optinally accepts a callback which is only run when the propagation has indeed occurred. These are now used in all places where we propagate error states back to upstream deferreds for the purpose of cancellation which hopefully makes it a bit more obvious what's going on.
See code comment for background
|
OK just pushed an attempt at making the error back-propagation more self-describing. @KingMob @arnaudgeiser WDYT? |
|
BTW if you're wondering about these weird "fixup!" commit messages, see e.g. this post for an explanation. I've adopted that workflow now instead of amending commits & force pushing while working on a branch. That should be less confusing during review hopefully 🙏 I'll rebase & autosquash these before merge then! |
If you like doing that, you'll love Jujutsu. Want to squash the current revision into some older one? jj can do this in a single command: Want to just edit that older revision without worry? Not ready to just edit that older rev in-place, or need to see the diffs of your new change more clearly? Want to insert a new blank revision between a parent and child? Here's a wild one. Need to work on three branches simultaneously, but don't want to rebase them off each other? Notice I mention editing "revisions", not commits. A commit is immutable, right? Well, in jj, a revision is a stable, semantically-related change that has a history of different commits underneath the hood. If you rebase a branch, the commit IDs all change in git, yes? That's true for jj too, but the revisions stay the same no matter what. Plus, it's all git-compatible, so your coworkers don't even have to know. |
| (defn on-error | ||
| [d f] | ||
| (d/on-realized d identity f)) |
There was a problem hiding this comment.
OK for now, but this should be in Manifold
| (defn propagate-error | ||
| "Registers an error callback with source which will attempt to propagate the error to destination. | ||
|
|
||
| If the error was propagated (i.e. destination wasn't yet realized), on-propagate is invoked with | ||
| the error value. | ||
|
|
||
| Returns source." | ||
| ([source destination] | ||
| (propagate-error source destination identity)) | ||
| ([source destination on-propagate] | ||
| (on-error source (fn [e] | ||
| (when (d/error! destination e) | ||
| (on-propagate e)))))) |
There was a problem hiding this comment.
source is stream terminology, not deferred terminology. We should call it something else to avoid confusion. I made that exact mistake on the first read-through.
| (d/error! src :boom) | ||
| (is (d/realized? dst)) |
There was a problem hiding this comment.
Isn't this a race condition? Might need to put them on same executor and thread.
| (is (= true (deref conn-established 1000 :timeout))) | ||
| (is (= true (deref @conn-established 1000 :timeout))) | ||
| (http/cancel-request! rsp) | ||
| (is (= true (deref conn-closed 1000 :timeout))) | ||
| (is (= true (deref @conn-closed 1000 :timeout))) |
There was a problem hiding this comment.
(deref @ ... looks weird. I suggest switching to unwrap/unwrap'.
| (ns aleph.util | ||
| (:require [manifold.deferred :as d])) | ||
|
|
||
| (defn on-error |
There was a problem hiding this comment.
I think it should be documented why you would use this over chain or on-realized.
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.Diff best viewed without whitespace changes.
Completes #712.