Consider adding AbortController.prototype.follow(signal)

[annevk]

See https://github.com/whatwg/dom/issues/920#issuecomment-726203604.

[jakearchibald]

The controller already has a strong reference to the signal, no? Via controller.signal. I guess that value could be overridden, but do we generally try to cater for cases like that?

[annevk]

Sorry, yeah, the controller keeps the signal alive, but not the other way around. I'll fix OP.

[jakearchibald]

Abort algorithms need to stick around while the controller is in reference and the signal is not aborted.

Abort events listeners need to stick around while the signal is in reference, or the controller is in reference and the signal is not aborted. But some of that seems redundant given the controller has a strong reference to the signal.

[annevk]

Oh yeah, I think the basic scenarios are fine already, indeed. My bad.

I guess the one thing that complicates things somewhat might be signals that follow other signals. Edit: would that be as simple as keeping it alive from the signal that it is following? At least as long as the aborted flag isn't set.

[MattiasBuelens]

I guess the one thing that complicates things somewhat might be signals that follow other signals. Edit: would that be as simple as keeping it alive from the signal that it is following? At least as long as the aborted flag isn't set.

Again, isn't that already the case by virtue of the followingSignal attaching abort steps to its parentSignal, and those steps necessarily keeping the followingSignal alive?

[MattiasBuelens]

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.

[annevk]

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 Request object).

[MattiasBuelens]

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 AbortController/AbortSignal implementation in my own code, and I sometimes find it useful being able to do both on the same signal. For example, I'd have a "long-living" abort signal for the lifetime of an entire component, and then create "short-living" signals for tasks spawned by that component. These task signals can be aborted separately, but they also follow the long-living signal so all tasks are automatically aborted when the component is destroyed. But then again, this might be a very specific use case that isn't very useful for the whole of the web platform. 😅

Anyway, sorry for the sidetrack. So yeah, I think GC is already pretty okay for signals following other signals.

[jakearchibald]

But then again, this might be a very specific use case that isn't very useful for the whole of the web platform.

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.

[annevk]

I guess we're really discussing a new issue now, but that's fine with me.

1. Following is not exposed, but we should perhaps document the limitation in case a specification tries to use it for something it doesn't work. I'd rather do that than make it work as it would be redundant at the moment.
2. Does following need to be exposed? It seems it should be doable to implement the scenarios you sketch in userland?

[MattiasBuelens]

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 AbortController.prototype.follow(signal) if there's enough interest. 🙂

[noseratio]

That said, I wouldn't mind having this exposed as AbortController.prototype.follow(signal) if there's enough interest. 🙂

Yes please! :) Coming from .NET, I use hierarchical cancellation all the time, it's very handy. I currently do it with CancellationTokenSource(linkedTokens?) from @rbuckton's Prex.

[MattiasBuelens]

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! 😆

[benjamingr]

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 AbortController.prototype.follow(signal) and not new AbortController(...signals)?

[benjamingr]

@MattiasBuelens hey :] Note your implementation of followSignal does not work in case the signal is never aborted. It works for the "operation was aborted" case but not for the "everything went fine" case.

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 addEventListener's API takes a function.

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 
}

[bterlson]

@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.

[wanderview]

Sorry if the conversation has moved on, but:

Hmm yeah. I'm not sure you can ever be a followingSignal with your own controller.

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.

[benjamingr]

@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 addEventListener which means the parent signal always holds a reference to the child controller (which can live for much longer) - this is the memory leak we've run into :]

[MattiasBuelens]

Thanks for chiming in! Indeed, from my experience it's very easy to leak abort event listeners in user code.

In my custom AbortController implementation, I have an additional destroy() method on AbortController. This first fires a 'destroy' event on its signal, and then removes all 'abort' and 'destroy' listeners that were attached on that signal. (I know, this isn't possible with a regular EventTarget, so I'm also rolling my own implementation there. 😛).

For my custom AbortController.follow() implementation, I add a 'destroy' listener to both the parent signal and the following signal, which removes all listeners that were added to either signal. This ensures that all the "links" between the two signals are removed, so they no longer keep each other alive.

It's messy and tedious, since I need to carefully add a bunch of destroy() calls throughout my code whenever I'm "done" with a certain signal. For example, this pattern pops up several times:

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. 😁

[bterlson]

@MattiasBuelens destroy on a controller doesn't necessarily mean its associated signal and its listeners can be disposed. I explained this some here.

[bterlson]

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.

[domenic]

A few points:

1. I agree this would be a good thing to add, if only because so many users are asking for it. This holds regardless of my next two points.

2. The "follow" operation has always seemed unintuitive to me, compared with a "race" or "create linked token source" style API (described here in the context of the defunct cancelable promises proposal). But, I don't trust my inutition here. Would someone with more experience be able to discuss the differences between these, and argue for one or the other?

3. Even if we add this high-level combinator, it'd be ideal if it could be expressed in userland code. There are a few potential missing primitives here, and I believe any one of them would suffice:

   • Weak event listeners. @syg and I discussed this in some private chat some months ago. You can get close to emulating these today with WeakRef, but you'll leak a bit of memory for the wrapper thunk, which looks like e => weakRef.get().?(e).

   • Some way of clearing all event listeners for the signal's "abort" event after the controller's abort() is called. E.g. https://github.com/whatwg/dom/issues/412 would allow this. If we exposed this then I'd suggest abort() auto-clears all listeners. (Which would be a backward-incompatible change, but hopefully a safe one.)

   • Probably some other ideas.

4. I don't fully understand the relationship between the issues discussed here, and how fetch() and other platform features ignore the event listener mechanisms in favor of their own internal "abort steps". But, in the past I've advocated against this magic; see https://github.com/whatwg/dom/issues/493 and the somewhat-related https://github.com/whatwg/dom/issues/878 .

[benjamingr]

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.

[domenic]

I'd love to see a use case where adding the reference to the child controller at construction doesn't work.

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.

[rbuckton]

2. The "follow" operation has always seemed unintuitive to me, compared with a "race" or "create linked token source" style API (described here in the context of the defunct cancelable promises proposal). But, I don't trust my inutition here. Would someone with more experience be able to discuss the differences between these, and argue for one or the other?

race is the approach I went with in https://esfx.js.org/esfx/api/async-canceltoken/canceltoken.html#esfx_async_canceltoken_CancelToken_race_member_1 (which is the successor to prex and based on my Cancellation Protocol proposal).

[bterlson]

@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?

[MattiasBuelens]

@MattiasBuelens destroy on a controller doesn't necessarily mean its associated signal and its listeners can be disposed. I explained this some here.

I see. For my use cases, a child signal following a parent signal never needs to "outlive" its parent signal, so it's fine for me to destroy the parent signal (and therefore all links with child signals) once I'm done with it. But I understand that this may not apply to all use cases, and a more general solution would be desirable.

[benjamingr]

@domenic

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.

I think new AbortController(...signals) is actually very similar to CancelToken.race API in that the construction is at the creation. For example the first example is:

const ac1 = new AbortController();
const ac2 = new AbortController();

const combined = new AbortController(ac1.signal, ac2.signal);

Though in this case, since there is a hierarchy you don't actually usually create 3 sources but two.

I think doing this on the signal and not controller can work though having users write:

const parent = new AbortController();
const child = new AbortController();
// ...
action(AbortSignal.race(parent.signal, child.signal));

const parent = new AbortController();
// child controller
const child = new AbortController(parent.signal);
// ...
action(child.signal);

Is a bit more code and doesn't establish hierarchy between the parent/child controllers (which may be desireable?).

[benjamingr]

What is a good way to make progress here? Would it help if I work on the spec? Do implementors also agree that this is something that this change is good?

What about a meeting to discuss API alternatives?

[annevk]

I wonder if we should flush out the use cases a bit more to ensure we are all on the same page and have something to evaluate a proposal against. Precedent in other ecosystems would also be great to see. Is there anything besides .NET?

(This definitely sounds like the kind of thing implementers would be happy to add though, nobody likes memory leaks.)