Skip to content
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

Userland AbortSignal #1195

Open
annevk opened this issue May 5, 2023 · 12 comments
Open

Userland AbortSignal #1195

annevk opened this issue May 5, 2023 · 12 comments
Labels
topic: aborting AbortController and AbortSignal

Comments

@annevk
Copy link
Member

annevk commented May 5, 2023

Splitting this from #1016.

Writing a robust userland AbortSignal consumer is tricky. In particular you might not get your event listener due to someone else listening calling stopImmediatePropagation().

Possible solutions:

  1. We invent a new way of dispatching that disallows preventing stopImmediatePropagation(). (@smaug---- is not a fan of this as per the other thread.)
  2. The consumer uses AbortSignal.any() to get a copy without other event listeners and registers their event listener there.
  3. We define userland "abort algorithms" somehow.

cc @rniwa @smaug---- @mfreed7 @domenic @shaseley

@annevk annevk added the topic: aborting AbortController and AbortSignal label May 5, 2023
@annevk annevk mentioned this issue May 7, 2023
4 tasks
@benjamingr
Copy link
Member

Thanks for opening,

Using AbortSignal.any([signal]) just to use the fact it exposes an underlying different mechanism in the DOM seems like not a great design. It's similar to making a dummy hanging fetch request and .catching it to know cancellation happened.

I'm trying to get around having another way to dispatch things, @smaug---- can you elaborate what specifically about not being able to prevent other listeners from running bothers you?

@smaug----
Copy link
Collaborator

It would be odd to have special event dispatch mechanism just for AbortSignal. We've had the consistent way for decades - stop*Propagation works everywhere. Why it should behave differently here?

Also, I'm having trouble to see the issue with using AbortSignal.any.

Btw, is there a concrete issue here, or is this just some theoretical one?

@benjamingr
Copy link
Member

There is a real issue, userland uses addEventListener("abort" for resource cleanup and other listeners being able to prevent that is unfortunate.

At the moment AbortSignal already has a second dispatch mechanism but it's only exposed to the dom internally and AbortSignal.any adds a third.

@benjamingr
Copy link
Member

Any update on this? I feel like we're adding a third way to do something (with any) - and the first two are problematic and there is no safe userland way to use AbortSignal until this is resolved (though to be fair in practice we haven't received complaints).

@annevk
Copy link
Member Author

annevk commented May 17, 2023

Well, AbortSignal.any() does give userland a way. And by-and-large folks are satisfied with the AbortSignal.any() design so I'm going to land that change, especially now it's getting multiple implementations.

I'll keep this open to see if many other people are running into this and if we need to make further improvements.

@benjamingr
Copy link
Member

So we should be recommending people always do AbortSignal.any([signal]).addEventListener("abort" when adding abort listeners?

@annevk
Copy link
Member Author

annevk commented May 17, 2023

Maybe? That's certainly the only way to do it if you run into this problem. As I said above, if enough people run into this problem we should consider solving it better.

@benjamingr
Copy link
Member

@annevk should we maybe add a note in the specification suggesting it as a safe way to ensure a listener runs?

@bojavou
Copy link

bojavou commented Jul 1, 2023

I would prefer if I could depend on abort signals being delivered. It seems odd if the signal offers it, I say I want it, and some other unrelated code shoots the messenger and prevents it from ever arriving.

This makes sense in a DOM tree where one node can say "OK I've handled this, it's done, no one else needs to handle it again". But responses to an abort are independent. Stopping an abort is like saying "OK I've stopped my piece, no one else needs to stop anything else, go ahead and leak all those resources". When would you ever need to legitimately cancel an abort partway through? The abort is the cancelation. You shouldn't be able to cancel cancelation.

What about a new property on Event? bubbles can prevent bubbling. cancelable can shut off preventDefault(). What if unstoppable could shut off stop*Propagation()?

signal.addEventListener('abort', event => {
  // Does nothing and raises a warning,
  // just like preventDefault() when cancelable === false
  event.stopImmediatePropagation()
})
const event = new Event('abort', { unstoppable: true })
signal.dispatchEvent(event)

This is in line with existing functionality. It keeps the existing dispatch mechanism. It keeps the current default of events being stoppable, and adds an option to control it for events that need it.

@bojavou
Copy link

bojavou commented Jul 2, 2023

Here's an example of the effect. The user tries to stop everything but only one operation stops.

https://bojavou.github.io/canceled-abort/

example

function aborted (event) {
  // The rest of my app doesn't need this, stop the event for efficiency
  event.stopImmediatePropagation()
  reject(signal.reason)
}

The developer thinks stopping the event is good for efficiency. That's reasonable and has been legitimate and beneficial since events began. But it's no longer true for an abort. Stopping an abort breaks user expectations.

Once delivered, the abort signal cannot be triggered again, and there's no way to provide a new signal to the ongoing operations, so this breaks cancelation nonrecoverably. User intuition is cancelation should work when requested. Developer intuition is an abort signal should deliver what it offers. This is counterintuitive for everyone involved.

Since there's no legitimate reason to stop an abort, and it has this highly visible and frustrating-to-the-user effect which the app can't recover from, I say it shouldn't even be possible. If AbortSignal becomes the standard for cancelation across the entire ecosystem, it shouldn't have this fragility built into it.

@benjamingr
Copy link
Member

benjamingr commented Jul 4, 2023

We (node) shipped (are shipping) our own helper to add a listener without the stopImmediatePropagation issue as since Node is a server runtime a resource leak is potentially catastrophic for us. We'll hopefully also add a resource argument for it so it holds the listener weakly on the resource but that's planned for later.

If AbortSignal becomes the standard for cancelation across the entire ecosystem, it shouldn't have this fragility built into it.

I honestly wish I would have listened better to library authors when they were complaining about this before adding it to Node APIs but I think this is mostly blocked in the WHATWG side on someone doing the spec work to fix this API wise.

@bojavou
Copy link

bojavou commented Jul 4, 2023

We (node) shipped (are shipping) our own helper to add a listener without the stopImmediatePropagation issue

Nice. I have special utils just for listening to abort signals, I'll at least be able to build it around this in Node. That's valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: aborting AbortController and AbortSignal
Development

No branches or pull requests

4 participants