Thrown AbortErrors are not DOMExceptions

[kanongil]

Version

v17.0.1

Platform

any

Subsystem

No response

What steps will reproduce the bug?

import { readFile } from 'fs/promises';

const ref = new DOMException('The operation was aborted', 'AbortError');
console.log('REF INSTANCE', ref instanceof DOMException);
console.log('REF OBJECT', ref);

const ab = new AbortController();
ab.abort();

try {
    await readFile('does_not_matter', { signal: ab.signal });
}
catch (err) {
    console.log('ERR INSTANCE', err instanceof DOMException);
    console.log('ERR OBJECT', err);
}

How often does it reproduce? Is there a required condition?

100%, including for all other native APIs that supports signals.

What is the expected behavior?

From the docs:

If a request is aborted the promise returned is rejected with an AbortError

There is no mention that this is a non-standard new DOMException(message, 'AbortError') error. As such, I expect 'REF' and 'ERR' logged objects to be the same (except the stack).

What do you see instead?

REF INSTANCE true
REF OBJECT DOMException [AbortError]: The operation was aborted
    at file://<snip>/fail.mjs:3:13
    at ModuleJob.run (node:internal/modules/esm/module_job:185:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)
ERR INSTANCE false
ERR OBJECT AbortError: The operation was aborted
    at checkAborted (node:internal/fs/promises:373:11)
    at readFile (node:internal/fs/promises:845:3)
    at file://<snip>/fail.mjs:11:11
    at ModuleJob.run (node:internal/modules/esm/module_job:185:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12) {
  code: 'ABORT_ERR'
}

Additional information

This is only an issue in node 17+, since previous versions did not have a DOMException global.

[benjamingr]

Hey, this is by design, you can see the context and meeting in #36084. Node.js does throw DOMExceptions for DOM APIs. For its own APIs Node.js prefers a simpler AbortError with the same .name property (which is the preferred way to check for AbortErrors according to the DOM spec). The reason for this is that Node.js vendors some of its modules (like readable-stream) and having to ship DOMException with them would be hard.

[kanongil]

@benjamingr That design does not appear to have been revised when node added support for native DOMException. This is causing extra confusion, and the root of this issue.

Essentially, this decision puts the onus on API consumers to handle the strange complexity, and on API documenters to clearly convey this, just so node can potentially simpler vendor readable-stream (which has not actually happened due to other complexities).

Given that there is no obvious central place to document this complexity, any API that uses a signal must clearly communicate this, which is currently failing.

I hope you will take a chance to reconsider this. You are prioritizing the efforts of a few core developers against the documentation team, and the entire developer base.

[benjamingr]

@benjamingr That design does not appear to have been revised when node added support for native DOMException. This is causing extra confusion, and the root of this issue.

Node has added DOMException before AbortSignal - the decision was to move away from DOMExceptions for non DOM APIs using AbortSignal and stick to .name (the reasoning is in the thread I linked to and in the meeting minutes).

(Node does vendor readable-stream as the readable-stream package on NPM btw - but it hasn't been updated)

Given that there is no obvious central place to document this complexity, any API that uses a signal must clearly communicate this, which is currently failing.

Would it help if the documentations clarified the AbortError interface we use like we do for AbortSignal and friends?

I hope you will take a chance to reconsider this. You are prioritizing the efforts of a few core developers against the documentation team, and the entire developer base.

I don't think that's an entirely fair or an accurate representation of how things transpired:

• AbortController/AbortSignal was added to the platform to enable cancellation scenarios.
• We iterated on them and added them to more APIS while soliciting responses from the community, library authors, WHATWG folks and other subject-matter-experts.
• When soliciting feedback from library vendors and core - the concern about DOMException being complicated and different was raised. There were no objections to changing it - you are in fact the first person to bring up this complaint in the ±year we've shipped this.
• The process was open and in the public, anyone from the community was welcome to participate and many were invited specifically so we collect user feedback. We've had a meeting about this in the summit (online, also open to everyone) and several other discussions.
• This decision was made considering the people who built AbortController/AbortSignal/AbortError in the DOM in the first place, people who worked on other AbortSignal APIs, the community, library authors and more.
• Things quieted down pretty quickly and we're getting positive feedback.

Also I want to point out that the decision isn't final, we can revisit anything here. We did not receive any pushback about this change from the documentation team nor developer base. We are happily willing to listen to feedback.

All of this is relatively new and improvement opportunities (in docs, APIs, debugging experience and more) are welcome. Contributions are welcome and Node.js in general and this area in particular is eager to receive them.

[kanongil]

Thanks for the thorough response, and good to know that there is still room for iterating the approach.

DOMException has only just been made public with node v17, so has only been there for a few weeks for API consumers. It was merged August 6'th in e4b1fb5, and I don't see any considerations on how this impacts AbortError in any of the relevant PRs. Issue #39098 explicitly mentions the ability to do instanceof checks. #39176 can hardly have been a consideration almost a year ago in #36084.

Also, most feedback you will have gotten so far is likely not from the wider dev community, as many still support node v12, and are only just starting to look into using the features v14+ provides due to v12 EOL coming up.

FYI, I'm very exited about this new feature, which is why I want to see it done right. Here I think unifying on new DOMException(message, 'AbortError') for node native and modules targeting v17+ will make it simpler to use. Both as providers (those throwing) and consumers (those passing signal / calling abort()) that are likely to have to ignore the subsequent rejections. Besides making it simpler to use the feature, it also enables compatibility with browser runtimes.

This still leaves module that target v14/v16 and v17+ "in the dust", having to deal with extra complexity to handle two different error implementations. However, they are no worse of than the current situation, and will eventually be able to drop the legacy codepaths.

Given that there is no obvious central place to document this complexity, any API that uses a signal must clearly communicate this, which is currently failing.

Would it help if the documentations clarified the AbortError interface we use like we do for AbortSignal and friends?

Yes – this would be simpler to document if the internal AbortError was public (as discussed in #38361).

[benjamingr]

Thanks for the thorough response, and good to know that there is still room for iterating the approach.

Absolutely.

DOMException has only just been made public with node v17, so has only been there for a few weeks for API consumers. It was merged August 6'th in e4b1fb5, and I don't see any considerations on how this impacts AbortError in any of the relevant PRs. Issue #39098 explicitly mentions the ability to do instanceof checks.

It's true that DOMException has been made public in Node.js 17 but it has been in core for a few years now (since things like URL raise it). Note that a public DOMException is even worse for stuff like readable-stream that exists for compatibility across versions since people may expect instanceof checks to work (and they won't) - neither will instanceof checks work across vm contexts and other "realms".

In general it makes perfect sense to expose DOMException as well as AbortError but people should never rely on instanceof checks for errors. That's why there were proposals for stuff like Error.isError.

If you read the WHATWG fetch specification carefully (I am happy to dig up the discussions where we initially met to discuss AbortController) - you will see it makes no requirements on raising AbortError. In fact when I added an API myself (signal in addEventListener) there were no errors involved.

DOM APIs are encouraged but not required to reject with an AbortError DOMException - non DOM APIs have no such requirements. If you look at the spec example it encourages checking .name rather than instanceof (for the aforementioned reasons) that is quite intentional.

#39176 can hardly have been a consideration almost a year ago in #36084.

I actually recall seeing (but not having time to review) that and trusting reviewers since I think it's overall a good idea.

Also, most feedback you will have gotten so far is likely not from the wider dev community, as many still support node v12, and are only just starting to look into using the features v14+ provides due to v12 EOL coming up.

This was less "users approaching us" and more "us approaching users and asking what they think" and "us talking to people who have experimented with userland AbortSignals (like SDK authors) and asking them what feedback their users are giving. Gathering user and library maintainer feedback is always a big challenge in Node.js and always something we could use more of.

FYI, I'm very exited about this new feature, which is why I want to see it done right. Here I think unifying on new DOMException(message, 'AbortError') for node native and modules targeting v17+ will make it simpler to use. Both as providers (those throwing) and consumers (those passing signal / calling abort()) that are likely to have to ignore the subsequent rejections. Besides making it simpler to use the feature, it also enables compatibility with browser runtimes.

Note that we switched from DOMException - I think this is where it was added after the meeting and consensus.

Unless the concerns raised by @mcollina regarding this being hard for readable stream and other parts were addressed in his perspective - I don't see us switching back to DOMExceptions (which wouldn't break a lot of code probably since people are supposed to check .name but it might).

Yes – this would be simpler to document if the internal AbortError was public (as discussed in #38361).

Is that something you'd be interested in contributing? I know we've talked about this in the past but I am honestly not sure what it's blocked on and I am happy for us to start with a simpler flagged errors module and add more codes/errors.

How can I help you with this? (I can review, ping the TSC to discuss the module inclusion, try to get more people to look at this etc).

[mcollina]

I don't have much time right now for joining a conversation with this depth. From a quick skim @benjamingr summarizes this correctly. My position has not changed on this matter.

[kanongil]

We agree that it makes sense for user code to distinguish AbortError from regular errors, eg. to avoid logging it, right? Thus it makes sense that the error included in the rejection (when triggered) is clearly identifiable. As you say, this is currently done through checking the .name property, but it relies on APIs to reject using properly crafted errors.

While the node APIs can be aligned on what is thrown, it is anyones guess what third-party modules will throw. And since those modules might forward the signal to submodules, the same API call could be able to reject using two or more completely different abort errors, depending on when it is aborted. Ie. you can't even reliably test against how abort errors are emitted.

For a different take on this, node could expose helpers like this to enable tests for .aborted and throwing using a standardized AbortError, making it less likely that third party modules get it wrong. And probably also something for the rare cases that the 'abort' event is used. This can be done without exporting the AbortError class.

Btw, this issue is really about standardizing the rejected error, be it through always using DOMException or some other means. I'm naturally inclined towards DOMException since it better aligns with browsers, but if it is a clear no-go, then there are still other ways to improve the situation.

[benjamingr]

I think we are all very much in favor of standardizing third-party thrown exceptions and providing guidance to people authoring async APIs with cancellation. I think that's a good idea and I don't think that was contested.

I think that is mostly blocked on someone doing the work. If exposing errors is a problem (I think it can be done and think the block is just technical) I am fine with exposing it elsewhere.

[domenic]

I saw claims like this a few times:

If you look at the spec example it encourages checking .name rather than instanceof (for the aforementioned reasons) that is quite intentional.

I can state that is not intentional, and using .name is not the recommended way. That one example in the DOM spec is just an example. All coding styles are valid and OK on the web; instanceof, for example, is fine (if you're working within a single realm). As is checking err.code === DOMException.ABORT_ERR.

I do think it would be beneficial to the ecosystem if Node didn't create its own separate AbortError type, and instead used and encouraged the standard DOMException. I think as @kanongil points out the fact that there are two is going to cause a lot of ecosystem confusion, as e.g. code that wants to be web/Deno compatible uses DOMException, and code which doesn't care about such compatibility uses Node's one-off version, and so on.

[ljharb]

Why would it be a good thing for environments without a DOM to have or use a DOM exception? If it was meant to be a universal exception type then it seems it was unwisely named.

[domenic]

Yes, it's pretty easy to complain about names of things in the JavaScript ecosystem; as I'm sure you're aware we haven't named everything optimally in its 20+ year history.

[benjamingr]

@domenic to be clear the thing blocking DOMExceptions for AbortErrors in Node.js isn't the fact people don't like DOMExceptions (we already ship DOMExceptions anyway).

It's the fact DOMExceptions become a dependency of libraries (like readable-stream) that are explicitly designed to run in non-modern Node.js environments and porting DOMException there would be hard.

The current solution is a compromise that enabled us to adopt AbortSignals in all APIs while staying spec-compliant. Obviously we would be happier shipping less kinds of errors for the same thing :)

@ljharb

Why would it be a good thing for environments without a DOM to have or use a DOM exception?

It's a communications thing but IMO the DOMness of the exception is "this exception comes from a DOM API" rather than requiring a DOM tree.

[benjamingr]

Oh, and regarding:

I saw claims like this a few times: I can state that is not intentional, and using .name is not the recommended way. That one example in the DOM spec is just an example.

@domenic I think we got that feedback/best-practice from @jakearchibald both in the AbortError meeting regarding it and back when cancellable fetch was discussed since it inter-ops between realms and is guaranteed.

If there is other preferred guidance we are happy to look at alternatives (though it would have been great a year ago).

(Also note I am happy to ping you more on these issues but I know you're busy and remember you don't like direct pings so I've been limiting the amount of pings to cases spec-compliance was on the line)

(And of course the thing that blocked using DOMExceptions wasn't their name anyway it was the difficulty vendoring)

[ljharb]

@benjamingr "came from a DOM API" means we'd be expecting users to know the difference between specifications, something web browser representatives have repeatedly insisted they do not and should not understand.

I'm not saying the name of DOMException is the issue; as much as it is that AbortError inherits from it. Is it really too late for the web to fix that?