Expose an aborted promise on AbortSignal?
#946
Comments
Just to clarify, the resource only stays in memory until
What mechanism is it using for that? Weakrefs? |
|
@jakearchibald in Node.js yes - but:
I am already prototyping (internal only) weak listeners in Node.js :] nodejs/node#36607 |
The issue is that sometimes you have a (very) long living signal - a common pattern observed is: const ac = new AbortController();
process.on('SIGINT', () => ac.abort());
// Use ac around in the project ...I have seen this pattern in Node.js and in browsers (mostly around router navigation). |
|
If you replace this with a promise you would no longer be able to synchronously act on it already being aborted, right? I think we do have this pattern, e.g., https://fetch.spec.whatwg.org/#dom-global-fetch first does a synchronous check and then adds a listener. And https://dom.spec.whatwg.org/#aborting-ongoing-activities-spec-example recommends it. But it seems to me that changing it to await a promise might change the runtime semantics. |
|
@benjamingr is there a real example you could point to that would benefit from this? I find that I'm either using checkpoints, which is why I proposed #927, or I'm taking a thing that doesn't support signal and passing it to something like this: async function abortable(signal, promise) {
if (signal.aborted) throw new DOMException('AbortError', 'AbortError');
let listener;
return Promise.race([
new Promise((_, reject) => {
listener = () => reject(new DOMException('AbortError', 'AbortError'));
signal.addEventListener('abort', listener);
}),
promise,
]).finally(() => signal.removeEventListener('abort', listener));
} |
The |
|
Yeah, I suppose you could use both, but that seems inelegant? |
I am regularly using both (via my CAF library). I consider the need to use both an artifact of the larger dichotomy between sync and async and the balancing act we play between lifting everything to promises (for nicer code) vs sometimes needing to optimize with a quicker sync path. IOW, that "cost" is already baked in as far as I'm concerned. So I'd be happy to see the platform add the promise (rather than replace the boolean) so that these dual efforts are more canonical (and likely more efficient) than doing so manually in my own lib and userland code. |
|
It isn't clear to me how a promise that only ever resolves on 'abort' solves the problem. Doesn't that just become a different leak point? |
|
@jakearchibald sure, that's a good question/point. Here are two quick examples: With timers (since they're the simplest API): // before
async function setTimeout(ms, { signal } = {}) {
return await new Promise((resolve) => {
const timer = setTimeout(() => {
signal?.removeEventListener("abort", listener);
resolve();
}, ms);
const listener = () => clearTimeout(timer) || resolve(Promise.reject(new AbortError()));
if (signal?.aborted) {
listener();
} else {
signal?.addEventListener("abort", listener);
}
});
}After: // after
async function setTimeout(ms, { signal } = {}) {
return await new Promise((resolve) => {
const timer = setTimeout(() => {
resolve();
}, ms);
// works in Node.js since timers are objects
aborted(signal, timer).then(() => clearTimeout(timer) || || resolve(Promise.reject(new AbortError())));
});
}With a database layer: class DB {
// wraps a database with AbortSignal API support
async query(string, { signal } = {}) {
await this.open();
const query = this.createQuery(string);
if (signal?.aborted) {
query.cancel(new AbortError());
} else {
signal?.addEventListener('abort', () => {
query.cancel(new AbortError());
});
}
return await query.promise();
}
// rest of the implementation
}After: class DB {
// wraps a database with AbortSignal API support
async query(string, { signal } = {}) {
await this.open();
const query = this.createQuery(string);
aborted(signal, query).then(() => query.cancel(new AbortError()))
return await query.promise();
}
// rest of the implementation
}Let me know if the examples are what you had in mind or you had something else in mind :] |
|
I haven't been following much of the subsequent discussion. However, my main comments on the OP are around the listener removal. In particular, my understanding is that the proposal is not equivalent to the OP's resource.on('close', () => signal.removeEventListener(listener));example, but instead something more like // This code probably leaks the signal, but I think a more complicated version could avoid that...
const registry = new FinalizationRegistry(() => {
signal.removeEventListener(listener);
});
registry.register(resource);because the web platform (and really Node as well) don't have a uniform This might be the first time the web platform uses GC observation in such a way? But, this seems like a reasonable use of GC observation: it's using GC observation to help other things get GCed, not to do important program logic. So I'm not too worried there. Another angle is, how does this contrast with weak listeners? My impression (but not a very confident one) is that this proposal actually has two parts: weak listeners, plus sugar to bundle together the function aborted(signal, resource) {
if (signal.aborted) {
return Promise.resolve();
}
else {
return new Promise(resolve => {
// hypothetical method
signal.addWeakEventListener('abort', resolve, { fireOnlyIfStillAlive: resource });
});
}
}I think this Anyway, my instinct is that if we want a feature like this, it'd be best to spec both the high-level helper, and the lower-level building block (like a weak listener). |
That's a good point.
That is exactly the sort of implementation I've had in mind - I initially investigated weak listeners for Note that having such an There is precedent of things holding other things weakly in the spec (for example mutation observers have a weak reference to the node they are observing). This use case might just be a generalisation of that one. I'm happy to follow up the (internal) weak listeners PR in Node.js with a
The idea is to provide a safe API for users to use to listen to the signal becoming aborted. |
|
OK, I'm glad to confirm my understanding of the proposal as (a type of) weak listeners plus some sugar. I take your point that it might be less scary to expose only the higher-level API. IMO the cat's out of the bag since WeakRefs and FinalizationRegistry exist, and it's scarier to make people use those directly than it is to give them higher-level "do what I mean" sugar. But I have no idea if that's a widely-shared opinion. Let's see what other folks think! |
annevk
added
the
needs implementer interest
|
Here's my understanding of the problem: An async task uses a signal to know when/if the task should be aborted. The listener the async task adds to the signal is no longer needed once the task is over, or once the listener is called (which should also end the task). However, the listener added by the async task ends up in reference as long as the signal is still in reference, which in some cases is much longer than necessary, since the signal's lifetime extends beyond the single async task. So, a solution needs to have some knowledge of the length of the async task. Here's the solution from @benjamingr:
(I removed a I'm really struggling to understand how the above works. I'm never the smartest developer in the room, but I think others would struggle with it too. In the above, assuming the reference that the body of
It fulfills if the signal is aborted, but I don't understand when the other things can happen. From other comments in the thread, it seems like finalization of the |
No no that was just a typo ^^
Basically there are two problems we've encountered with
Basically - in
const retainerMap = new WeakMap();
function aborted(signal, retainer) {
if (signal.aborted) return Promise.resolve();
return new Promise(resolve => {
// only hold the listener as long as "retainer" is alive, this means if retainer is not alive no one has a reference
// to the resolve function and the promise can be GCd (and so can its then callbacks etc)
// In node we implement this with a FinalizationRegistry and WeakRefs
signal.addEventListener('abort', resolve, { magicInternalWeakOption: retainer });
// explicitly make the retainer hold a reference to the resolve function so we have a strong reference to it
// and the lifetime of `resolve` is bound to `retainer` since it's the only one that holds it strongly
// note that this is a weak map so `retainer` itself is held weakly here.
retainerMap.set(retainer, resolve);
});
}I'd also like to emphasise I'm not fixated on this particular solution. I just brought it up as an idea and I'm not convinced myself it's a good one though I am tempted to implement as experimental and see what people say. I figured that it should be a promise because that's our async primitive for "timing independent multicast one-off things". I figured it needs a way to tie its lifetime to a resource since that's the bit everyone is doing manually in the meantime. It's entirely possible there are much better ways to accomplish this I haven't thought of yet. I am in no hurry :] |
|
Based on the idea that a solution needs to have some knowledge of the length of the async task, here's a pattern: function abortableSetTimeout(ms, { signal } = {}) {
return abortableTask(signal, (setOnAbort) => new Promise((resolve) => {
const timerId = setTimeout(() => resolve(), ms);
setOnAbort(() => clearTimeout(timerId));
}));
}Where this is the implementation of /**
* @param {AbortSignal | undefined} signal
* @param {(setAbort: (abortCallback: (() => void) | undefined) => void) => Promise} taskCallback
*/
async function abortableTask(signal, taskCallback) {
if (signal?.aborted) throw new DOMException('', 'AbortError');
let onAbort, listener;
const setOnAbort = (callback) => { onAbort = callback };
const promise = taskCallback(setOnAbort);
return Promise.race([
new Promise((_, reject) => {
listener = () => {
onAbort?.();
reject(new DOMException('', 'AbortError'));
};
signal?.addEventListener('abort', listener);
}),
promise,
]).finally(() => signal?.removeEventListener('abort', listener));
}Some benefits of this:
I don't think it leaks, but I'd like a second opinion on that 😄. Here's the DB example: class DB {
// wraps a database with AbortSignal API support
query(string, { signal } = {}) {
return abortableTask(signal, async (setOnAbort) => {
await this.open();
const query = this.createQuery(string);
setOnAbort(() => query.cancel());
return query.promise();
})
}
// rest of the implementation
} |
|
@jakearchibald this is pretty similar to what James had in mind in nodejs/node#37220 (comment) @getify what do you think? Would Jake's idea version (returning a promise) address your use case? @jasnell any opinion on the above API compared to what you discussed in nodejs/node#37220 (comment) ? |
|
I gave it a spin on squoosh.app, which uses abort signals quite a bit https://github.com/GoogleChromeLabs/squoosh/pull/954/files - it helped in one place! The other helpers I needed were /**
* Take a signal and promise, and returns a promise that rejects with an AbortError if the abort is
* signalled, otherwise resolves with the promise.
*/
export function abortable(signal, promise) {
return abortableTask(signal, () => promise);
}…which is kinda nice.
|
I'm not sure, lemme try to clarify. Could it be used kinda like this? var ac = new AbortController();
var pr = abortableTask(ac.signal,async (setOnAbort) => {
setOnAbort(() => "aborted!");
});
pr.then(msg => console.log(msg));
ac.abort();
// aborted!Am I understanding it correctly? Or would it be: var ac = new AbortController();
var pr = abortableTask(ac.signal,async () => {
return "aborted!";
});
pr.then(msg => console.log(msg));
ac.abort();
// aborted! |
|
The second one. The implementation works, so you can test it yourself. |
Sorry, didn't see the implementation, missed that. So, now that I've looked at it and played with it, I think I would end up using it like this: var ac = new AbortController();
ac.signal.pr = abortableTask(ac.signal,() => new Promise(()=>{}));
// elsewhere
setTimeout(()=>ac.abort(),5000);
try {
await Promise.race([
someAsyncOp(),
ac.signal.pr,
]);
}
catch (err) {
// ..
}So, IOW... a util like It's slightly beneficial for my purposes, but not as nice as if there was just a On that last point, is there any chance that |
|
I've been doing a similar thing since Node got support for // doAbortable.ts
export default async function doAbortable<R>(
signal: AbortSignal | undefined,
func: (abortSignal: AbortSignal) => R | Promise<R>,
): Promise<R> {
const innerController = new AbortController();
const abortInner = () => innerController.abort();
if (signal?.aborted) {
throw new AbortError();
} else {
signal?.addEventListener("abort", abortInner, { once: true });
}
try {
return await func(controller.signal);
} finally {
// this allows innerController to be garbage collected
// and hence if nothing is holding a strong reference to the signal
// that too may be collected
signal?.removeEventListener("abort", abortInner);
}
}export default function animationFrame(abortSignal) {
// the inner abortSignal can be garbage collected
// once the promise returned resolves as there'll be
// no references to it remaining once doAbortable
// calls removeEventListener("abort", abortInner);
return doAbortable(abortSignal, (abortSignal) => {
return new Promise((resolve, reject) => {
const frameRequest = requestAnimationFrame(time => resolve(time));
abortSignal.addEventListener("abort", () => cancelAnimationFrame(frameRequest));
});
});
}I've found it works fairly cleanly, although the extra wrapper is slightly annoying. This might be able to improved if function and parameter become a thing cause then I could just annotate the abort param and decorate it e.g: @abortable
export default function delay(time, @abortable.signal abortSignal) {
// ... create delay promise
} |
|
I don't have strong feelings about my version vs @Jamesernator's. It avoids the Edit: oh wait, I've just noticed that @Jamesernator's solution depends on the inner task to manually reject its promise if the task aborts. That seems less ergonomic to me. Maybe that's unintentional, since the @getify Your example looks pretty weird and I'm not sure what it's trying to achieve. I don't understand why it isn't: const ac = new AbortController();
// elsewhere
setTimeout(()=>ac.abort(),5000);
try {
await asyncTask(signal, someAsyncOp);
// or
// await asyncTask(signal, () => someAsyncOp());
// depending on if someAsyncOp supports reacting to abort.
}
catch (err) {
// ..
} |
|
It's the same reason you see the "deferred" pattern still used in promises, like by extracting the resolve/reject methods from inside a promise constructor and then using them elsewhere externally to what was inside the promise constructor: sometimes it's really inconvenient (or impractical) to stuff all the logic that determines resolution of the promise inside that single constructor. In my case, I actually have many different places that need to observe the promise attached to the signal, some inside the internals of my library, and some in the userland functions that people write and pass into my library. So, I need to be able to pass around both a At any given moment, there might be quite a few places in the program that are observing the single So, similar to resolve/reject extraction, I am essentially needing to "extract" a promise for the signal aborting, and store that and use it around. Right now I do this by making my own calls to I was asked if this utility being contemplated (for Node and for the web) could be useful for my purposes. Hopefully this helps explain the ways it does and doesn't fit with what you're suggesting here. |
|
Makes sense! Yeah, I guess the same as the promise design, it's nice that it still helps use-cases that depend on pulling the insides outsides (I've run into these cases too), but we shouldn't over-index on them. |
Agreed, to an extent. I don't think we should design a util that only serves my use case, but I also hope we don't design a util that falls short of it either. Would be nice to find a flexible compromise. If the util you suggested had an option to be called without the For clarity, here's sorta what I'm suggesting: async function abortableTask(signal, taskCallback) {
if (signal?.aborted) throw new DOMException('', 'AbortError');
let onAbort, listener;
const setOnAbort = (callback) => { onAbort = callback };
const taskPromise = taskCallback?.(setOnAbort);
const signalPromise = new Promise((_, reject) => {
listener = () => {
onAbort?.();
reject(new DOMException('', 'AbortError'));
};
signal?.addEventListener('abort', listener);
});
return (
taskPromise ?
Promise.race([ signalPromise, taskPromise ]) :
signalPromise
)
.finally(() => signal?.removeEventListener('abort', listener));
}
// elsewhere
const ac = new AbortController();
ac.signal.pr = abortableTask(ac.signal);With just a few changes here, it makes the If instead I have to pass a function for |
It was intentional at the time, but thinking about it more it might not be necessary. I think I was concerned about the fact the promise would never resolve, but actually it should be fine as if its simply raced against the abort it should in theory be able to be collected as there would be no remaining references to it as once it does a cleanup step like |
|
@Jamesernator I'm not sure I follow. Are you saying your proposal should change or should stay as it is? |
That it should change. Your It'd be quite cool as a method of abort signal so that we could just do: function animationFrame(abortSignal) {
return abortSignal.task((setOnAbort) => {
return new Promise((resolve) => {
const frameRequest = requestAnimationFrame(time => resolve(time));
setOnAbort(() => cancelAnimationFrame(frameRequest));
});
});
}I don't think it quite meets the OP use case. But I've generally found the |
|
@getify it's kinda nice that one function could do both, but I'm worried it muddies the intent of the method. Although these things could be the same method implementation-wise, would it make more sense in terms of intent and documentation to make them different methods? |
|
Here's how I'd document the proposal:
const result = await signal.doAbortable(async (setAbortAction) => {
// Do async work here.
setAbortAction(() => {
// Abort the async work here.
});
});
If abort is signalled, the last callback passed to Example: An abortable that resolves with const controller = new AbortController();
const signal = controller.signal;
const promise = signal.doAbortable((setAbortAction) => {
return new Promise((resolve) => {
const timerId = setTimeout(() => resolve('hello'), 10_000);
setAbortAction(() => clearTimeout(timerId));
});
});
ImplementationAbortSignal.prototype.doAbortable = function (callback) {
if (this.aborted) throw new DOMException('', 'AbortError');
let onAbort, listener, onAbortReturn;
const setAbortAction = (c) => { onAbort = c };
const promise = callback(setAbortAction);
return Promise.race([
new Promise((_, reject) => {
listener = () => {
reject(new DOMException('', 'AbortError'));
onAbortReturn = onAbort?.();
}
this.addEventListener('abort', listener);
}),
promise,
]).finally(() => {
this.removeEventListener('abort', listener)
return onAbortReturn;
});
}; |
|
From #981 (comment):
Would add that async function wait(signal, duration) {
let id;
try {
await signal.race(new Promise(resolve => id = setTimeout(resolve, duration)));
} finally {
clearTimeout(id);
}
}
// or:
function wait(signal, duration) {
let id;
return signal
.race(new Promise(resolve => id = setTimeout(resolve, duration)))
.finally(() => clearTimeout(id));
}There’s no material difference between (Apologies if these ideas duplicate things which were previously discussed here or elsewhere since I haven’t read all relevant comments yet.) Sample of other abort-utils we use that seem notable or get used often (fwiw)export function coalesce(...signals) {
let controller = new AbortController;
signals.forEach(signal => signal.addEventListener('abort', () => controller.abort()));
return controller.signal;
}
export function frame(signal) {
let id, promise = new Promise(resolve => id = requestAnimationFrame(resolve));
return race(signal, promise).finally(() => cancelAnimationFrame(id));
}
export function idle(signal, opts) {
let id, promise = new Promise(resolve => id = requestIdleCallback(resolve, opts));
return race(signal, promise).finally(() => cancelIdleCallback(id));
}
export async function * idling(signal, opts) {
for (;;) yield await idle(signal, opts);
} |
|
Maybe One of the nice things about I don't think Is this your implementation of AbortSignal.prototype.race = async function(promise) {
if (this.aborted) throw new DOMException('', 'AbortError');
let onAbort;
return Promise.race([
new Promise((_, reject) => {
onAbort = () => reject(new DOMException('', 'AbortError'));
this.addEventListener('abort', onAbort);
}),
promise,
]).finally(() => this.removeEventListener('abort', onAbort));
}There'd need to be some education around usage of all of these patterns, as: // Good (assuming './search-helpers.js' has no side-effects)
const response = await fetch(searchURL, { signal });
const { addSearchResults } = await signal.race(import('./search-helpers.js'));
await addSearchResults(response, { signal });
// Very bad
await signal.race((async () => {
const response = await fetch(searchURL, { signal });
const { addSearchResults } = await import('./search-helpers.js');
await addSearchResults(response, { signal });
})());…since the latter may still add search results after the operation appear successfully aborted. |
|
Our race currently looks like this: export function race(signal, promise) {
return Promise.race([ toPromise(signal), promise ]);
}
// The `toPromise` part is used internally but hasn’t needed to be exposed as part of the
// public API of our abort-utils module to date:
let promises = new WeakMap;
function toPromise(signal) {
if (!promises.has(signal)) {
promises.set(signal, new Promise((resolve, reject) => {
let propagate = () => reject(new DOMException('Aborted', 'AbortError'));
if (signal.aborted) {
propagate();
} else {
signal.addEventListener('abort', propagate);
}
}));
}
return promises.get(signal);
}It’s not on the prototype for us since it wasn’t polyfilling any (current!) proposed feature. Other than that, the differences seem to be:
(The same-value returning stuff for
For sure. I suspect there’s no way around this stuff being a little tricky to grasp, but one of the reasons I came to favor |
My bad. It should have been an async promise to enforce that. I've updated my implementation.
That's a good way to think of it. |
|
FWIW,
|
Yeah, it wouldn’t account for multiple-signal cases on its own (and would not account for “all” at all*). In our utils, that pattern ends up being (A native Re: custom reasons — I haven’t used / thought about that before but I suspect you are correct that the pattern I suggested is not friendly to it. * I’d be curious about an example scenario where “all” is needed. I’m sure such cases exist, but I’m having trouble imagining one. |
The use-cases are much more limited than for However, there was one use-case I came across and was glad I had it: an app where multiple operations can be initiated/running at a given time, and each one can either complete or be canceled independent of the others. If you cancel (or timeout) all of them, the whole state of the application is treated as "reset", but if some of them are able to complete, you don't reset, you just proceed.
|
|
One thing to point out since I'm seeing folks duplicate the const { const } = require('events');
const ac = new AbortController();
once(ac.signal, 'abort').then(() => console.log('aborted!'))
ac.signal();... is something that just works. The |
|
Yeah, I'm aware of that, but you can't rely on |
|
That doc clearly shows we’re in territory that’s been tread before. What happened to the last search party who went down here? (Were their bodies recovered at least?) Reading it also made me realize that in my first comment here, I failed to explain one of the key reasons I think the API guiding folks towards an “every await should honor the signal” model is important. Picture a |
Agreed. This is not only true from an ergonomic/safety perspective (we don't want people to forget to race the signal and thus fall into the pit of failure) but also in the learning/documentation perspective (we should want to make it easy and obvious to do the right thing and hard to do the wrong thing). CAF took that philosophy to heart in its design, and that was the reason that it went with generators instead of async functions. In this way, CAF treats |
|
One issue with |
That's true when the signal itself is input and would be important to call out. For library code, this is likely the common case. For app/UI code, it seems quite rare(?). About 90% of AC/AS use in our codebase is internal state (e.g. of a custom element) in service of the abort-replace-proceed pattern. The signal can never be aborted at the beginning of the work in such cases; you've just created it one line earlier. |
Hey,
Node.js AbortController integration is working great - almost all Node.js APIs now take a
signalparameter to allow cancellation.The Problem
I've spoken to a bunch of library authors regarding AbortSignal ergonomics and one feedback that has been brought up a bunch is that the following pattern is very common and rather easy to get wrong:
The issue is that this is pretty easy to get wrong and cause memory leaks because:
One alternative suggested was to have a utility method that helps with this, the above code bemes:
It was also brought up this has several other use cases like
Promise.race([somePromiseICantCancel, aborted(signal)])which is very nifty because it integrates AbortSignal with promise combinators without requiring JS spec changes.Ref: nodejs/node#37220 and getify/CAF#21
Idea
It would be very cool if instead of Node.js shipping this as a utility this could be part of the specification (a method on AbortSignal). That way we have a consistent cross-platform way to handle the use case.
Doubts
I am still not entirely convinced this is what we should do (either for Node or WHATWG). I think this is pretty nifty but I wanted to ask (the nice) people here if there are any alternatives (both for the Node.js utility and for the spec).
Additionally, I wanted to know if browsers think this is even an issue and find this interesting. I think this is important since it makes comosing cancellation APIs without bugs easier. Regardless of browser interest being a prerequisite for spec inclusion it would be very interesting to me as information to decide what route to pursue in Node.js.
Alternatives
I know I wrote this above as well but I am very interested in knowing if other people thought about this problem as well and have different ideas about how to solve it.
Implementation
I am happy to contribute the needed spec changes to the WHATWG spec if you are willing to put up with the low quality of my work. The feedback provided (by Domenic and Anne) last time was very helpful and I learned a bunch.
(I am starting a new job soon in a company who is a member of WHATWG and I'll need to re-sign the license I think)
cc helpful people who helped with the ideas last time I worked on related stuff @domenic @annevk @jakearchibald
cc library authors I talked to about cancellation and brought up use-cases that encouraged this pattern @bterlson @benlesh @getify
Note 1: I am putting on my Node.js hat because that's the one I put on when I talked to users - but to be fair/honest the majority of use cases people brought up were browser API based and not Node.js.
Note 2: As mentioned in the
.followissue I am still very much "on it" and in fact this utility could significantly simplify that problem (hopefully!).Note 3: I am in no rush, I am hopeful that this will be slightly less mind-melting than that
.followstuff :DThe text was updated successfully, but these errors were encountered: