Repository navigation
Make AbortError public #38361
Description
Activity
I'm a bit torn on it. There is no
AbortErrorconstructor in the standard so we'd be exposing something that would only work in Node.js. I'm not necessarily -1 on that but we need to make sure we understand what is means from a code portability pov.Our AbortError is a Node.js specific API - it behaves like but isn't actually a DOMException or behave like the standard one. Like you said, AbortErrors aren't actually part of the standard.
That said: I do see the value in exporting it to userland to help make it easy to construct AbortErrors.
Wasn't there an effort to make the whole errors module public? What happened to that? That would implicitly make
require('errors').AbortErrorworkReacted by Derek Lewis, Felix Becker and 流浪大法师Reacted by Derek Lewis and 流浪大法师Wasn't there an effort to make the whole errors module public?
@benjamingr, any additional context you can provide on this would be nice to read up on. Was there a thread somewhere?
- Reacted by Derek Lewis
I am looking into integrating aborts in the hapijs ecosystem, and find that a standardised
AbortErroris sorely missing from node. Eg. in@hapi/lab, it would be nice to reliably know when something has errored due to an abort (outside of node APIs).In browsers this is standardised through
new DOMException(message, 'AbortError'), which creates the error with the correctcode = 20property. Of courseDOMExceptionsupport in v17 complicates this further. What will APIs throw?As it is, I guess we will end up creating our own
AbortErrorclass in@hapi/hoekand use it internally.As it is, I guess we will end up creating our own AbortError class in @hapi/hoek and use it internally.
That is totally fine as long as you have the right
.codeand.nameon it.Node.js has two AbortErrors mainly so that it can vendor parts (like streams or events) without having to ship DOMException as a dependency. APIs will throw the easy-to-vendor one when they are allowed and the DOMException one when they are required by a web specification Node.js is implementing.
I think the next step if you're up for it for exportable AbortError is to work on #14554
@benjamingr #14554 already seems to have a failed implementation in #38575 - why would a second go at it work?
@kanongil that PR was blocked on "Things require further consideration" (like in #38575 (review) and #38575 (review) ) . It wasn't blocked on "this is a bad idea" and I suspect one can pick up and finish the work (most easily by exposing a smaller subset).
Also please keep in mind that when writing code for a platform like Node.js used by millions of developers that is run on billions of devices - adding APIs (especially ones that are prone to these discussions) is challenging.
There is a lot of hesitation and considerations so that things don't end up causing harm. This isn't a bad thing - it is quite common for changes (e.g. promise APIs or workers) to succeed on the third or fifth attempt. That's because the problem is hard and we're all (mostly) volunteers.
I am looking into integrating aborts in the hapijs ecosystem, and find that a standardised
AbortErroris sorely missing from node. Eg. in@hapi/lab, it would be nice to reliably know when something has errored due to an abort (outside of node APIs).In browsers this is standardised through
new DOMException(message, 'AbortError'), which creates the error with the correctcode = 20property. Of courseDOMExceptionsupport in v17 complicates this further. What will APIs throw?As it is, I guess we will end up creating our own
AbortErrorclass in@hapi/hoekand use it internally.That is totally fine as long as you have the right
.codeand.nameon it.I think this is generally okay if people are careful, but I am wary of the fact this ad-hoc approach does make it rather easy for people to footgun, especially if many libraries need to implement their own
AbortError. In particular I think these things will be likely footguns:- People won't include
.code, this will likely happen as if people are checking.name === "AbortError"today there is a reasonable chance they won't even know about or consider the existence of.code- Also builtin errors like
RangeError,TypeError, and such are only really detectable by.name
- Also builtin errors like
- People won't include
.name, similar to the previous but they're only using.codeto check if an error is of a specific kind - The
.codeproperty is an inconsistent thing between Node-style errors and DOM-style errors- For
DOMExceptionthe.codeis always a number, for Node-Style errors.codeis a string - People trying to support both well, essentially can't
- Also JS-errors don't have it at all
- For
- The
.nameproperty on error subclasses is not automatically derived from the class-name, one actually needs to remember to set it- i.e.
class AbortError extends Error {}; console.log(new AbortError().name)producesErrornotAbortErrorlike one might expect - If the library uses
instanceof(or#privField in value) or other such brand-checking internally, then this could easily be overlooked
- i.e.
- People won't include
- added a commit that references this issue
on Nov 19, 2021 I'm in significant need of this and had to do the following hack:
import {EventEmitter} from "events" let AbortError try { const ctrl = new AbortController() ctrl.abort() on(new EventEmitter(), "", {signal: ctrl.signal}) } catch (e) { AbortError = e.constructor }
If
onever is rewritten to an async function, this will break. And it's the only one I know that throws that error synchronously.I use this broadly for signaling cancellation after operations like dynamic imports and raw syscalls that genuinely can't be cancelled once requested, so I can still honor the request in the same way Node's native APIs do. As an example:
export async function *chunkIterator(file, {maxChunkSize = 65536, signal} = {}) { const handle = await fs.open(file, "r") const cache = new Uint8Array(maxChunkSize * 2) let queued = 0 try { while (true) { if (signal?.aborted) throw new AbortError() const {bytesRead} = await handle.read(cache, queued) if (bytesRead === 0) break queued += bytesRead if (queued >= maxChunkSize) { queued -= maxChunkSize yield cache.subarray(0, maxChunkSize) } } if (queued > 0) yield cache.subarray(queued) } finally { await handle.close() } }
This is a stripped down version of real-world code that also retries on a number of errors.
The only alternative for me is to literally just reimplement the class, which would admittedly be rather brittle and potentially run into subtle incompatibilities down the road if/when those appear. And that's a risk I really don't feel like dealing with.
The only alternative for me is to literally just reimplement the class, which would admittedly be rather brittle and potentially run into subtle incompatibilities down the road if/when those appear. And that's a risk I really don't feel like dealing with.
Just to be clear there are at least two different classes of "AbortError" in Node, both the
AbortErrorclass and theDOMExceptionbasedAbortErrorthing. So subtle differences can already exist depending on how you're using it.Currently really only
error.name === "AbortError"is decently future proof and cross compatible with browsers.14 remaining items
I was trying to debug an AbortError and I tried to use
Error.stackTraceLimit = 100;which increases the Error stack trace, but unfortunately it didn't for AbortError. Since AbortError is not global, any suggestions on how to increase the stack trace limit? If not, I think we should re-discuss this.@anonrig that sounds like a separate issue where you want to ensure Error.stackTraceLimit impacts DOMException/AbortError.
I suspect it isn't working for you due to an optimization
It's a regular
Error- are you sure you weren't looking at a DOMException/user facing DOMException/fetch AbortError?Lines 961 to 973 in f692878
DOMException also calls Error.captureStackTrace which means it isn't an error subclass but it should be impacted by .stackTraceLimit
- also that one is publicDoes it work if you use the CLI flag i.e.
--stack-trace-limit?Does it work if you use the CLI flag i.e.
--stack-trace-limit?Unfortunately it didn't help. Here's my patch for a reproduction and the error:
diff --git a/test/parallel/test-net-connect-abort-controller.js b/test/parallel/test-net-connect-abort-controller.js index 9c259cc3fc..b5cc1d2c68 100644 --- a/test/parallel/test-net-connect-abort-controller.js +++ b/test/parallel/test-net-connect-abort-controller.js @@ -1,4 +1,3 @@ -'use strict'; const common = require('../common'); const net = require('net'); const assert = require('assert'); @@ -23,6 +22,7 @@ server.listen(0, common.mustCall(async () => { await once(socket, 'close'); assert.fail(`close ${testName} should have thrown`); } catch (err) { + console.log('error ', err); assert.strictEqual(err.name, 'AbortError'); } }; @@ -82,11 +82,11 @@ server.listen(0, common.mustCall(async () => { } await postAbort(); - await preAbort(); - await tickAbort(); - await testConstructor(); - await testConstructorPost(); - await testConstructorPostTick(); + // await preAbort(); + // await tickAbort(); + // await testConstructor(); + // await testConstructorPost(); + // await testConstructorPostTick(); // Killing the net.socket without connecting hangs the server. for (const connection of liveConnections) {
node test/parallel/test-net-connect-abort-controller.js --stack-trace-limit=100 error AbortError: The operation was aborted at stream.<computed>.AbortError.cause (node:internal/streams/add-abort-signal:47:22) at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:20) at AbortSignal.dispatchEvent (node:internal/event_target:762:26) at runAbort (node:internal/abort_controller:447:10) at abortSignal (node:internal/abort_controller:433:3) at AbortController.abort (node:internal/abort_controller:466:5) at postAbort (/home/yagiz/coding/node/test/parallel/test-net-connect-abort-controller.js:35:8) ... 2 lines matching cause stack trace ... at Object.onceWrapper (node:events:632:28) { code: 'ABORT_ERR', [cause]: DOMException [AbortError]: This operation was aborted at new DOMException (node:internal/per_context/domexception:53:5) at AbortController.abort (node:internal/abort_controller:465:18) at postAbort (/home/yagiz/coding/node/test/parallel/test-net-connect-abort-controller.js:35:8) at Server.<anonymous> (/home/yagiz/coding/node/test/parallel/test-net-connect-abort-controller.js:84:9) at Server.<anonymous> (/home/yagiz/coding/node/test/common/index.js:435:15) at Object.onceWrapper (node:events:632:28) at Server.emit (node:events:518:28) at emitListeningNT (node:net:1980:10) at process.processTicksAndRejections (node:internal/process/task_queues:89:21) }JW, if you run it with a connected inspector do you get the stack trace? There is probably a
thensomewhereThere has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale
Issues and PRs exempt from automated stale handling. label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.
For more information on how the project manages feature requests, please consult the feature request management document.
Would be useful to be able to throw
AbortErrors in userland.