Thrown AbortErrors are not DOMExceptions #40692
Comments
|
Hey, this is by design, you can see the context and meeting in #36084. Node.js does throw |
|
@benjamingr That design does not appear to have been revised when node added support for native 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 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. |
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)
Would it help if the documentations clarified the
I don't think that's an entirely fair or an accurate representation of how things transpired:
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. |
|
Thanks for the thorough response, and good to know that there is still room for iterating the approach.
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 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.
Yes – this would be simpler to document if the internal |
Absolutely.
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 In general it makes perfect sense to expose DOMException as well as AbortError but people should never rely on 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 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
I actually recall seeing (but not having time to review) that and trusting reviewers since I think it's overall a good idea.
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
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
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). |
|
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. |
|
We agree that it makes sense for user code to distinguish 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 For a different take on this, node could expose helpers like this to enable tests for Btw, this issue is really about standardizing the rejected error, be it through always using |
|
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 |
targos
added
the
errors
|
I saw claims like this a few times:
I can state that is not intentional, and using 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. |
|
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. |
|
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. |
|
@domenic to be clear the thing blocking It's the fact The current solution is a compromise that enabled us to adopt
It's a communications thing but IMO the |
|
Oh, and regarding:
@domenic I think we got that feedback/best-practice from @jakearchibald both in the AbortError meeting regarding it and back when cancellable 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) |
|
@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? |
|
Yes, I agree developers should not have to know where the error came from. That's why I think Node.js should follow other ecosystems in using DOMException for abort errors instead of inventing its own one-off. We have no plans to change how the web throws abort errors to use anything other than DOMException. |
Actually
Do you think it would be viable to make This would mean |
|
We have no plans to change how the web throws abort errors to use anything other than DOMException. |
|
That's clear, but I think the request is to perhaps create those plans. |
|
@DerekNonGeneric this is not a confirmed bug - the fact this happens was a design decision that carefully considered the trade-offs. We can re-evaluate this further and see if we can stop vendoring readable-stream (or only vendor it in environments that have DOMExceptions) or we can explore DOM changes (though I'm not sure how we'd make a compelling case for browsers) or anything really. Let's not "fix" this until there is consensus it should be fixed and how :) |
|
My only argument was being in favor of code portability, but open to finding what other ways we would be able to achieve that. |
|
@DerekNonGeneric I am not arguing for/against using In particular readable-stream would need to vendor DOMException inside readable-stream and would like to avoid that. |
Yes, there's plenty of improvements on the web for ergonomics of APIs all the time. I don't see why
And to be clear, I don't want to break existing code which is why my suggestion above made
For this, something worth pointing out is that the spec for const controller = new AbortController();
controller.abort();
console.log(controller.signal.reason); // DOMException: AbortError
``
This `.reason` property is intended to be [thrown from all APIs as part of its integration](https://github.com/whatwg/dom/issues/1030). i.e. If you're aborting from a signal it would be equivalent to this:
```js
if (signal.aborted) {
throw signal.reason;
}
signal.addEventListener('abort', (event) => {
reject(event.target.signal);
});The class MyCustomError extends Error { name="MyCustomError" }
const controller = new AbortController();
controller.abort(new MyCustomError());
console.log(signal.reason); // MyCustomErrorI don't know if this throwing behaviour is something Node will adopt or not, but if it does this essentially removes control of what kind of error is thrown from the code utilizing an |
|
Using this in node would make the |
|
@kanongil If I understand correctly @Jamesernator to explain: in order for the DOM to consider not using DOMExceptions it would have to be specifically interesting to the browser stakeholders. So you would need to present a compelling argument on how this would benefit browsers rather than a non-browser-related Node.js compatibility issue. |
|
Hmm, I think Node.js should still wrap errors with AbortError probably even if |
|
@benjamingr Yes, it should replace the generated error, as specified here. |
|
@jasnell Node.js moved from DOMEXceptions for vendoring reasons (so that readable-stream would not need to vendor DOMExcepiton). If that concern has been resolved we can revert that decision. |
What are the situations where you actually want to distinguish cancellation from other error kinds WHERE you don't control the
I do agree that Also it's not really any different to the fact that an API can just outright ignore the
If readable-stream simply throws |
Thinking about this for a minute, you actually can distinguish cancellations from other errors. This isn't something I'd recommend for general people to use (general users should just treat the errors by their kind, not how they were thrown), but if you really must you can do: async function myCancelableOperation(abortSignal) {
try {
await someOtherOperation(abortSignal);
} catch (error) {
if (abortSignal.aborted && error === abortSignal.reason) {
// The error was a cancellation
} else {
// Not a cancellation
}
}
} |
This only works if you have a reference to the signal.
The ask is not for true third-state cancellation only to be able to distinguish it as was possible until now. I realize that we don't have to
See examples in whatwg repo. Basically almost any time a framework is involved? |
|
If, instead of wrapping |
|
@benjamingr I think this is a better place to respond to your comment about fetch1 than on the closed issue for that, but please direct me if there's a more constructive way I could engage with this issue. I'd like to make the case that Node should switch to throwing As a consumer of a signal that forwards it to another function, any code I write that looks like this try {
await forward({ signal });
} catch (err) {
if (err.name === 'AbortError') { /* ... */ }
throw err;
}can just as easily be written like this try {
await forward({ signal });
} catch (err) {
if (err === signal.reason) { /* ... */ }
throw err;
}I'd like to argue that the latter is more accurate. Instead of reacting to the fact that something was thrown that has a Whether or not such a function should or should not propagate the Anyone creating an The only place where this falls down is on v16, where Footnotes |
|
I am currently both sick and on bereavement leave so I can't leave a proper full response but: There are two major issues with this:
|
|
No rush on any fuller response, but I'll leave my reactions for when you return:
This is a proper concern, but I'm hoping it's mitigated by the fact that for any case where
This is exactly the argument in favor of the change, I think. If one has multiple signals, one can tell them apart if a rejection surfaces an object that is one of the reasons of one of the signals. Otherwise, we're stuck guessing at which signal, if any, might be the cause. If one doesn't have the signal at all, checking properties of the error is still the best bet regardless, and properly re-throwing unexpected errors (if you catch them at all) is normal. The use case I have in mind as I think about this is one where there is more than one concurrent, asynchronous call that has yet to resolve. One might want to abort them gracefully, e.g. in response to SIGTERM. But one could throw exceptions into them that cause them to reject with something other than an abort error, effectively causing them to exit less gracefully. |
It would have been nice if it was but unfortunately this changes the API surface consumers have to deal with from "This API can reject with an AbortError" to "This API can reject with anything" and code surrounding it would now have to deal with other types of errors. Since virtually all of Node's APIs supports this this is a big breaking change in errors. I am honestly not sure semver-major is enough for such a big breaking change in error handling. Note that single error-code changes are semver-major and this is a lot more drastic. Node has a lot less lee-way in these sort of breaking changes.
We have mitigated this by setting the That said like with exceptions you typically shouldn't have to care which signal caused the abort only that the abort happened to clean up and propagate the error. So while this use case is possible I am not sure it's one people should reach out to often.
I acknowledge that communicating extra information or metadata is a totally valid use case. If the whatwg design forced AbortError as a base class or |
|
The situation where Node.js library functions don't honor Is it still the case that this will never change, not even in a semver-major change? |
|
I'm just running into this over and over again and finding that it really hurts usability to the point where I almost don't want to use signals at all. Here is a slightly modified snippet of real code I have in production: const signal = AbortSignal.timeout(WORK_TIME_MS);
try {
await performWork({ signal });
} catch (error) {
if (error instanceof Error && error.name === 'AbortError') {
// work time expired, just return
} else {
// some unexpected error actually occurred
throw error;
}
}In v16, this code works. In v18+, this broke because function performWork(options) {
const controller = new AbortController();
const signal = controller.signal;
const incomingSignal = options.signal;
if (incomingSignal) {
const onAbort = () => {
const error = new DOMException("The operation was aborted.", "AbortError");
error.cause = incomingSignal.reason;
controller.abort(error);
}
if (incomingSignal.aborted) {
onAbort();
} else {
incomingSignal.addEventListener('abort', onAbort, { once: true });
// ... hopefully remember to clean this up later ...
}
}
// ... finally on to the real function body
}This would be slightly easier if I've run through the process of creating a Following the WHATWG semantics is easy, but following the Node.js semantics is hard. As a result, it's tempting to write code, as I have, that rejects inconsistently depending on when the abort happens. |
Correct, now imagine every Node.js API would increase its error API surface from "these few errors" "to anything anyone with access to the controller may decide to throw". Not great IMO. As for your actual code can't you use Also
Your example shows zero Node.js code though? What you're complaining about is the API design itself isn't it? |
The whole point of Personally I've always thought the This is particularly bad for user code, which might be passing signals to a mix of Node APIs or Web APIs, i.e. if we have something like: async function loadAndWrite(url, file, { signal }={}) {
const response = await fetch(url, { signal });
await fs.promises.writeFile(file, new Uint8Array(await response.arrayBuffer()), { signal });
}then this function, when an abort is signaled, can either throw the given error to |
It's unfortunate however Node.js can't take all its APIs and change their exception surface from "a standard node error with a .code property" to "whatever the user can throw to you". It basically means every error handling code that may work with signals must expect anything to be thrown from whoever owns the controller. This effectively is a huge API surface change and not to the better.
In an ideal world it's consistent. To be fair there are a few things I think are very problematic from the Node.js point of view but unfortunately I don't the capacity to work on improving them. |
Why not? The only way to observe this is if you pass a signal that throws a different error, in which case you are presumably expecting that signal's error to propogate. Again this is a major part of the the reason that
I disagree, being able to signal different kinds of exceptions through is useful. Like
Code that sees exceptions it doesn't recognize should just rethrow them and stop continuing work, this is basic code design that has been understood for decades at this point. |
Yes but you do not own the controller just the signal. So someone from the outside can change your API surface. If you have error handling code that does something like: function myCode({ signal }) {
try {
await someFunction({ signal });
} catch (e) {
const aborted = someCheck(e); // e.g. e.name === 'AbortError' || e.code === 'ECONNABORTED";
if (!aborted) throw e;
else handleCancellation(e);
const retryable = someOtherCheck(e); // e.g. e.code === 'EAGAIN'
await retryLogic(e);
throw e;
}
}In this case - every error may become unrecoverable and unexpected from the signal. The controller isn't aware of all its consumers. Let's say my code does something like: const ac = new AbortController();
startServer(someData, ac); // gets AC to abort on request
await myCode({ signal: ac.signal });If
No argument regarding it being useful the disagreement is only about the core APIs rejection/error API surface. Something being useful doesn't mean it doesn't come with painful tradeoffs and I believe Node.js can't make this one for the reason above.
You are telling me that people don't write great code - as the platform I'm not sure what to say other than "Hyrum's law". There are many things I wish we could discourage more in terms of the code people write (rejecting/throwing with strings anyone?). Node servers crash when an exception happens, crashes can cause a DoS, cascading failures, financial damage etc. If some library I don't control using core APIs decides to abort a controller with a new type of cancellation/error - I get woken up at night (bad) or go down (even worse). I got woken up for forgetting to filter cancellation errors correctly from instrumentation back in 2014 times ^^ (Ironically enough (all parties were for it iirc), if we had third party cancellation this wouldn't be an issue ^^) |
|
(Also @domenic as you 🚀 ed @Jamesernator comment, I want to say that while I disagree with James's point I do see it. I'm happy to change my mind, debate this and hear more arguments. Your inputs here are always valuable and respected. I hope the ±10 years we've interacted about these sort of issues are enough to convince you I am sincere in that and am always happy to hear you out) |
|
And let me ask for some data: @mcollina @ronag from the From the API side, I'm wondering if there is some way to find out how common "I expect this list of errors" code is as well as find out how many people treat cancellation differently than exceptions in their error handling code. I'm happy to hear ideas on how to measure this and change my mind if I'm assuming Hyrum's law and being pessimistic. One thing we may be able to try is a flag to change the behavior and then to ask people to report issues with it. I'll also see if I can ask the Node.js using teams/groups in Microsoft to check. |
Yes, but again again this is the whole point of being able to signal any error.
This is exactly the sort've fragile pattern I was talking about, you shouldn't be handling cancellation specifically, EVERY exception that can't be handled explicitly should result in the work being cancelled not simply those with name A better pattern is to precisely reverse this sort've pattern: function myCode({ signal }) {
try {
await someFunction({ signal });
// Note we don't handle cancellation specifically at all
} catch (e) {
// If it's not a retry, then we don't know to handle it, so we just cleanup and stop work
if (e.code !== "EAGAIN") {
await cancelWorkAndCleanup();
throw e;
}
// Whatever to retry
await retry(...);
}
}
If you're giving If you don't want this, pass a different // Yes, I don't like how verbose this either, the AbortController/AbortSignal API does leave
// a lot to be desired in the sugar department
const ac = new AbortController();
const serverController = new AbortController();
serverController.addEventListener("abort", () => ac.abort(new WhateverIWantToAbortWith()));
startServer(someData, serverController); // gets AC to abort on request
await myCode({ signal: ac.signal });
There are general problems with JS async error handling that leaves a lot to be desired, but generally speaking cancellation errors shouldn't be propagating to a point where instrumentation could even observe them. I don't know what cancellation primitives were around in 2014, but for Like in the given example from before, given the code seems to be top level one should be prepared to handle the exception: const ac = new AbortController();
startServer(someData, ac); // gets AC to abort on request
// We handle cancellation at this level because we are the ones who are prepared to handle cancellation
try {
await myCode({ signal: ac.signal });
} catch (e) {
if (e === ac.signal.reason) {
// The code has been cancelled, so we don't need to throw an error
}
} |
|
Something else I do think is worth pointing out in all of this, is that for the most part users shouldn't be needing to handle cancellation at all in intermediate functions because generally speaking user code will be delegating to Node code that actually acts as sinks of Like to demonstrate what I mean, if we write something that can be cancelled: async function doSomething({ signal }) {
// ...
}then internally in most cases async function doSomething({ signal }) {
// ...
await fetch(..., { signal });
// ...
await fsp.readFile(..., { signal });
// ...
await timerPromises.setTimeout(..., { signal });
}here users don't need to handle cancellation at all, they just let the various Node APIs throw the error and it'll propagate up naturally. Also, usually speaking when users are implementing abortable APIs directly, the web-like behaviour is basically encouraged thanks to the exposure of // Supposing userland implemented timerPromises.setTimeout themselves
function setTimeout(delay, { signal }={}) {
return new Promise((resolve, reject) => {
const timeout = setTimeout(() => resolve(), delay);
signal?.addEventListener("abort", () => {
clearTimeout(timeout);
// Note that the easiest available option here is to use signal.reason
reject(signal.reason);
});
});
} |
|
@Jamesernator you are giving me examples of how one can author code to avoid pitfalls this is potentially creating, my argument hasn't been that it isn't possible to write code that works after this change it's that a bunch of people won't and likely already don't. That was the whole bit about Hyrum's law and errors being part of the API surface (enough that error code changes in Node are semver major and we "broke the ecosystem" a few times in the past before (and after) that). (Also it's depressing to me that we've given people an API where even experienced developers debating the semantics of these very APIs forget to remove the event listener on the signal, myself included sometimes) |
Well they certainly won't if Node decides to actively support behaviour that is divergent from what web browsers implement and the spec recommends. Also
I'm not suggesting this shouldn't be a semver major change, though I am wary of the fact that the longer this behaviour is around the more entrenched it becomes. And it's not like people in Node can even handle such errors in a consistent way anyway, as I pointed out with my earlier example. Ultimately if people rely on Node's error behaviour, and a library changes to use
Yes, I usually use a helper that cleans up the listener. There are suggestions for this problem in the DOM spec, though they all have the dreaded
I don't want this to come across as an attack on anyone, but I do really think whatwg dropped the ball when designing this API. It's beyond me why the fairly obvious problems were not considered at the time, and still fail to have implementer interest to fix. It also has always strongly bothered me that from the get go of |
Well, the web has a lot fewer APIs than Node that work with AbortSignal and (arguably) a lot more use cases where you'd need to abort ongoing actions. As I mentioned I am happy to change my mind if my intuition of "this breaks things" is wrong and I'm not sure how to get the data
There are actually two now whatwg/dom#1195 the recommendation is (?) to do As |
|
Currently readable-stream supports down to v12. However, the next release would likely cut down to v18, so this can land in a semver-major change. |
|
Coming back to check in on this issue. In my own code, most of my use of Node.js APIs that take signals was using Node.js streams. I've switched these all to use the Web Streams API and now I can reliably compare exceptions to the abort reason. I do think it's pretty confusing that Node.js has a mix of behaviors here. If it isn't already too entrenched to consider changing this, maybe we want to try to do it for v24 now that the v23.x branch is cut. |
Version
v17.0.1
Platform
any
Subsystem
No response
What steps will reproduce the bug?
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:
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?
Additional information
This is only an issue in node 17+, since previous versions did not have a
DOMExceptionglobal.The text was updated successfully, but these errors were encountered: