Encourage use of AbortError for cancellation?

[jakearchibald]

Bit of a half baked idea, but should we make it easier to throw abort errors from signals? Something like:

AbortSignal.prototype.assertNotAborted = function(message = 'Aborted') {
  if (this.aborted) throw new DOMException(message, 'AbortError');
};

I know it's a one-liner, but it might encourage using the right kind of error?

[annevk]

@domenic might have thoughts given whatwg/webidl#933 (comment).

[benjamingr]

I think Domenic's (also interesting) suggestion is the inverse of this one.

Jake is suggesting a way for signals to "throw if cancelled" and Domenic is suggesting a way for consumers to filter out cancellations as not exceptional in cases.

Both seem useful to me in their own right.

Worth mentioning that for Node it would be more convenient if the method Domenic is proposing was on AbortController or AbortSignal and not on DOMException since we can't guarantee internal methods will always throw DOMExceptions and DOMException isn't exposed as a global.

[annevk]

I meant the part where @domenic doesn't necessarily want to couple DOMException to AbortSignal. And if we did decide that was okay for this use case, we might want to reconsider the design for the other use case.

[domenic]

I think coupling them is fine overall. My comment there was more about layering. That proposal has to do with exceptions, not Abort signals, i.e. ignoring AbortErrors could be useful independent of AbortSignals. So just from a technical architecture point of view putting it on DOMException made more sense to me.

The OP's idea sounds good to me, and has precedent in .NET and other cancel-token using ecosystems.

[Jamesernator]

Worth mentioning that for Node it would be more convenient if the method Domenic is proposing was on AbortController or AbortSignal and not on DOMException since we can't guarantee internal methods will always throw DOMExceptions and DOMException isn't exposed as a global.

One option would be to add a global AbortError constructor, which delegates to whatever a given host thinks is appropriate. For example in browsers it could just be:

function AbortError(message="Aborted") {
    throw new DOMException(message, "AbortError");
}

In node it might vend something different (but still with .name === 'AbortError').

While @jakearchibald's suggestion is great for checkpoints, it doesn't quite work for cases where you need to produce an AbortError asynchronously e.g.:

function animationFrame(abortSignal) {
    return new Promise((resolve, reject) => {
        abortSignal?.assertNotAborted(); // Checkpoint use case as suggested in OP
        const frameRequest = requestAnimationFrame(time => resolve(time));
        abortSignal?.addEventListener("abort", () => {
            cancelAnimationFrame(frameRequest);
            reject(new AbortError()); // Case I'm suggesting, this isn't covered by OP
        }, { once: true });
    });
}

[domenic]

Note that Node.js has DOMException so I don't thing there's really a need for such an intermediate layer.

[benjamingr]

Node.js doesn’t currently expose its DOMException and throws errors that look like the DOM AbortError but are not actually DOMExceptions (same name and same code but without some of the quirks - quite painful since Node.is codes have a different format)

Of course in implemented DOM APIs Node will behave in a spec complaint way and throw a DOM AbortError.

However since Node vendors certain bits of its core as user land modules (readable-stream) and exposing DOMException was a show stopper there we ended up with that compromise.

I believe Jake Dor Robert Matteo and myself were on that call.

We are open to changing this behavior.

Note we currently do not expose our AbortError either.

[benjamingr]

And I am +1 on adding A utility method like assertNotAborted

I think that if that is done it should always throw the same DOMException AbortError regardless of environment.

I am pretty content with Node.is absorbing the complexity of this and am happy to implement.

[Jamesernator]

Node.js doesn’t currently expose its DOMException and throws errors that look like the DOM AbortError but are not actually DOMExceptions (same name and same code but without some of the quirks - quite painful since Node.is codes have a different format)
However since Node vendors certain bits of its core as user land modules (readable-stream) and exposing DOMException was a show stopper there we ended up with that compromise.

If Node doesn't expose DOMException how can we vend abort errors for cases like the reject example above? Doing something hacky is possible like try { abortSignal.assertNotAborted(); } catch (err) { reject(err); } but this seems awkward.

Just a thought I had, it could be possible to do it the other way around and have .abort() create an AbortError which is then attached to the abort event. This same exception would be thrown from .assertNotAborted() e.g.:

const controller = new AbortController();

controller.abort("user left the page");

const { signal } = controller;


signal.addEventListener("abort", ({ abortError }) => {
  reject(abortError); // abortError has "user left the page" message
  doCleanup();
});

signal.assertNotAborted(); // throws the Abort Error with message "user left the page"

One bonus of this approach is the stack trace on the abort error would clearly show where the abort is triggered, which would massively help in tracing the origins of aborts.

[benjamingr]

If Node doesn't expose DOMException how can we vend abort errors for cases like the reject example above?

Basically it's up to userland to create a class AbortError extends Error { code = 'ABORT_ERR' }.

We can expose it (there has been talk of exposing the internal errors nodejs/node#34348 nodejs/node#14554 ) - honestly no one has asked for Node to expose AbortError yet.

[Jamesernator]

Basically it's up to userland to create a class AbortError extends Error { code = 'ABORT_ERR' }.

This is different to browsers, abortError.code is just a number in DOMException. Currently I've been using .name === "AbortError", however this is somewhat fragile against userland classes as they might forget to add name = "AbortError"; into the class.

This is one reason I think it'd strongly be best if there was a global exposed in both Node and Browsers that could construct a "blessed" AbortError so that people can reliably both construct them and check if a given error is an AbortError without relying on userland to correctly implement many copies of AbortError that have the right code and name. (Or something like the alternative I described above where abortController.abort() constructs one).

[kanongil]

FYI, public AbortError is being requested here: nodejs/node#38361

Also, this just got a lot more complicated with node v17's native DOMException support, which means that there are now two official (and conflicting) AbortError types along whatever ends up in userland, eg: https://github.com/nodejs/node/blob/341312d78a8b5b4d5ef03429161242dd9d9b9206/deps/npm/node_modules/minipass-fetch/lib/abort-error.js#L2

[Jamesernator]

So the core reason for this issue has effectively been resolved by the addition of the new abortSignal.reason. However that PR does give a rather nice basis for methods like abortSignal.assertNotAborted() in that we already have the error available.

i.e. Instead of the original code in the OP we could now just have (roughly):

AbortSignal.prototype.assertNotAborted = function() {
    if (this.aborted) throw this.reason;
}

[annevk]

Note that having signal.reason also addresses the concerns raised earlier about how to deal with the asynchronous part of the implementation. You'd no longer have to create an exception yourself as you can simply throw signal.reason.