-
Notifications
You must be signed in to change notification settings - Fork 287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider adding AbortController.prototype.follow(signal) #920
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
On a slightly unrelated note: what is missing here is if the followingSignal is aborted separately (i.e. through its own abort controller, not as a result of the parentSignal becoming aborted). In that case, the followingSignal should remove the abort steps it had attached to its parentSignal, so that the parentSignal does not keep the followingSignal alive unnecessarily. I don't think this shouldn't be too difficult to fix in the specification. I'll give it a try. |
Hmm yeah. I'm not sure you can ever be a followingSignal with your own controller. Fetch uses this mechanism and always creates an "unowned" signal for this (as part of creating a new |
So either a specification calls "signal abort" on a signal, or it makes it "follow" another signal, but it can't ever do both on the same signal? I guess that makes sense, it's just not explicitly documented as a restriction. Not sure if we need to. 🤷 I've been using an Anyway, sorry for the sidetrack. So yeah, I think GC is already pretty okay for signals following other signals. |
I had a use-case like this a couple of days ago fwiw. It was on squoosh.app, where there's a single controller that does overall things like decoding the input image, but then there are two other controllers for processing and encoding each side. Changing the encoding settings on one side only aborts the operations on that side, but dropping in a new image aborts main image decoding, and processing of both sides. |
I guess we're really discussing a new issue now, but that's fine with me.
|
Indeed, it's easy enough to do in userland, and I'm totally fine with keeping it there: function followSignal(followingAbortController, parentSignal) {
// 1. If followingSignal’s aborted flag is set, then return.
if (followingAbortController.signal.aborted) {
return;
}
// 2. If parentSignal’s aborted flag is set, then signal abort on followingSignal.
if (parentSignal.aborted) {
followingAbortController.abort();
return;
}
// 3. Otherwise, add the following abort steps to parentSignal:
const onAbort = () => {
// 3.1. Signal abort on followingSignal.
followingAbortController.abort();
};
parentSignal.addEventListener('abort', onAbort, { once: true });
// Non-standard: if followingSignal becomes aborted for some other reason,
// remove the abort steps from the parentSignal.
followingAbortController.signal.addEventListener('abort', () => {
parentSignal.removeEventListener('abort', onAbort);
});
} That said, I wouldn't mind having this exposed as |
Yes please! :) Coming from .NET, I use hierarchical cancellation all the time, it's very handy. I currently do it with |
I just realized that if we have #911, this could be simplified(?) to: // 3. Otherwise, add the following abort steps to parentSignal:
parentSignal.addEventListener('abort', () => {
// 3.1. Signal abort on followingSignal.
followingAbortController.abort();
}, {
once: true,
signal: followingAbortController.signal
}); It's abort signals all the way down! 😆 |
Hey, a few of us have been discussing this a lot and we think there needs to be a way for signals to "follow" other signals. This is because making linked signals is very hard at the moment. This is impossible to do in userland though I'll let Brian leave a more detailed comment. Node is very interested in this API and I believe it is very hard to use AbortSignal for large web apps with "linked signals" without it. The use case we've seen over and over is something like this one - I've actually seen this in browser environments and converted it to a Node case for our meeting :] Regarding the color of the bike shed: Is there any reason this is |
@MattiasBuelens hey :] Note your implementation of The handler would have to be weak so that if the operation completes successfully the parent controller's signal does not keep a reference to the child controller. This is impossible in userland because Edit: A correct implementation would be something like: function followSignal(followingAbortController, parentSignal) {
// 2. If parentSignal’s aborted flag is set, then signal abort on followingSignal.
if (parentSignal.aborted) {
followingAbortController.abort();
return;
}
// This bit isn't actually possible in user-land
parentSignal.addEventListenerWeakly('abort', () => followingAbortController.abort());
// also somehow keep a strong reference from the child to the parent
} |
@MattiasBuelens @annevk fwiw I don't believe this is particularly easy to do in userland without leaking signals. Consider an application which has a top-level signal that lasts basically forever (e.g. its job is to tell the application its about to go down so cancel any ongoing work and clean up). Every signal in the application follows that signal to handle more granular events like user input or timeouts. With the implementation above, assuming the happy path of no cancellation occurs, the parent signal keeps all its children signals alive forever. If I'm missing something please let me know, I would love a way to do this in userland! Follow signals are critical so we should definitely figure this out, and happy to see more interest along these lines! I have concerns about the proposed API considering linking multiple signals together (e.g. a cancel button plus timeout plus sigint listener) can be fairly common. |
Sorry if the conversation has moved on, but:
I implemented something like this in the browser as well. For cache.addAll() I needed to abort outstanding requests if one of them failed. The spec just uses verbiage like "terminate all ongoing fetches with the aborted flag set", but in practice the most direct way to do it was with an internally created AbortSignal and AbortController. And since I still needed to honor individual request.signal I chained them together with follow. |
@wanderview just to add to what you're saying, when you follow you basically use the capability we're asking for that isn't exposed to userland. In userland, you can't chain signals in a way that doesn't require |
Thanks for chiming in! Indeed, from my experience it's very easy to leak abort event listeners in user code. In my custom For my custom It's messy and tedious, since I need to carefully add a bunch of const tempController = new MyAbortController();
tempController.follow(signal);
try {
await doSomething(tempController.signal);
} finally {
// At this point, either the controller became aborted which caused `doSomething` to reject,
// or `doSomething` resolved/rejected normally. So we can clean up our temporary abort controller.
tempController.destroy();
} I'd be very interested in ways to get rid of this mess. 😁 |
@MattiasBuelens |
FWIW I think solving this properly in user-land requires signals exposing first-class, disposable subscriptions to abort events so users and libraries are encouraged to dispose those subscriptions, even in the happy non-abort path, and for fetch to use this mechanism rather than piercing into internal signal state. Even from the privileged position of implementer with tricks at your disposal I'm not sure you can solve the leakiness issues without such an API, or at least people getting much more diligent about removeEventListener in happy paths. |
A few points:
|
Re: API - I think the simplest API would probably be a constructor argument: const childController = new AbortController(parentSignal); // creates a linked AbortController I'd love to see a use case where adding the reference to the child controller at construction doesn't work. Re: weak event listeners and WeakRefs - I want to agree and echo there is no way to not "leak the thunk" - I get Node servers are not really a priority for WHATWG but I assume people with long running PWAs can likely run into this issue even if they implement the WeakRef solution you propose in userland (note you also need to keep a strong reference from the child to the parent in that case). Weak event listeners would address this as well though they feel like a much more powerful tool. |
I tried to link to a couple examples of that, where you want to create a new AbortSignal from two other AbortSignals. It's not obvious to me how to do that from a single follow like you describe. |
|
@domenic regarding 3.b, a big part of the problem is listeners sticking around in non-abort situations, so by itself clearing all listeners on controller abort doesn't solve the problem I think? |
Reading through some of the older comments in this thread, I just wanted to point out that every single use case I have requires the following signal to have its own controller so it can be aborted separately, which removes the abort logic that was added to the parent. |
@benjamingr I've been playing with this idea using |
@noseratio I made a PR (to Node) for adding weak listeners (internally, and as part of working on the explainer above which I am just getting to due to a surprise vacation my wife ordered). nodejs/node#36607 |
@benjamingr this is awesome, thank you! I believe this is going to be really useful beyond Once your PR is merged in, what would be best way to detect this feature? I have this |
@noseratio at the moment I want to not add any APIs and work understanding use cases better. So unfortunately I feel strongly about doing this as part of a web standard effort rather than Node providing its own API :] I would love contributions in the use cases. I intend to start writing things down and I'll probably invite people to meetings a few times :] |
Hey, I just wanted to let you know that in the past month I've spent a bunch of time on this - I've rewritten the Node weak event listeners PR around... 3 times now. I think this issue is tricker than one might expect. I am also considering several other alternatives (like a utility I'm just not very good at this yet so this is taking me time :] I just wanted to update that this is still very much something I'm spending time on and that any help and discussion would be appreciated. |
Hi all, we're (Chrome) interested in tackling this, and I've put together an explainer for |
Thanks @shaseley, I opened an issue there, mainly to suggest an additional requirement for this feature that it either uses or obsoletes the current "follow" primitive in the specification. @benlesh I wanted to ask if you could elaborate on what you wrote above:
In particular I'm not sure I understand what you mean by the very last part of that sentence. |
|
Shouldn't we consider |
What do you expect |
Stops following parent signal: child signal not aborted when parent is aborted. |
What would be the use case for unfollowing a signal? |
Imagine 2 http requests that should be aborted by timeout. First request successfully passed, and in that case we want to wait second request anyway to have app in some desired state. if second request's abort controller is following the first one, it will be aborted by timeout. I just want to highlight that if followed AbortControllers are passed as constructor arguments, there is no way to unfollow them. |
I think for that it makes sense to have a And note that the current proposal is #920 (comment), which does not touch |
See #920 (comment).
The text was updated successfully, but these errors were encountered: