Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AbortError public #38361

Closed
ronag opened this issue Apr 22, 2021 · 21 comments
Closed

Make AbortError public #38361

ronag opened this issue Apr 22, 2021 · 21 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@ronag
Copy link
Member

ronag commented Apr 22, 2021

Would be useful to be able to throw AbortErrors in userland.

@ronag ronag added the feature request Issues that request new features to be added to Node.js. label Apr 22, 2021
@ronag
Copy link
Member Author

ronag commented Apr 22, 2021

@benjamingr

@jasnell
Copy link
Member

jasnell commented Apr 22, 2021

I'm a bit torn on it. There is no AbortError constructor 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.

@benjamingr
Copy link
Member

@jasnell

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').AbortError work

@DerekNonGeneric
Copy link
Contributor

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?

@benjamingr
Copy link
Member

#14554

@kanongil
Copy link
Contributor

I am looking into integrating aborts in the hapijs ecosystem, and find that a standardised AbortError is 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 correct code = 20 property. Of course DOMException support in v17 complicates this further. What will APIs throw?

As it is, I guess we will end up creating our own AbortError class in @hapi/hoek and use it internally.

@benjamingr
Copy link
Member

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 .code and .name on 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

@kanongil
Copy link
Contributor

@benjamingr #14554 already seems to have a failed implementation in #38575 - why would a second go at it work?

@benjamingr
Copy link
Member

benjamingr commented Oct 21, 2021

@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).

@benjamingr
Copy link
Member

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.

@Jamesernator
Copy link

Jamesernator commented Oct 21, 2021

I am looking into integrating aborts in the hapijs ecosystem, and find that a standardised AbortError is 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 correct code = 20 property. Of course DOMException support in v17 complicates this further. What will APIs throw?

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 .code and .name on 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:

  1. 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
  2. People won't include .name, similar to the previous but they're only using .code to check if an error is of a specific kind
  3. The .code property is an inconsistent thing between Node-style errors and DOM-style errors
    • For DOMException the .code is always a number, for Node-Style errors .code is a string
    • People trying to support both well, essentially can't
    • Also JS-errors don't have it at all
  4. The .name property 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) produces Error not AbortError like one might expect
    • If the library uses instanceof (or #privField in value) or other such brand-checking internally, then this could easily be overlooked

kanongil added a commit to kanongil/node-1 that referenced this issue Nov 19, 2021
This is done to prepare user code for signal.reason, which will allow
custom errors to be thrown on aborts. Custom errors means that it
will not be possible to conclusively determine if an error is from an
`ac.abort()`, just by looking at it. By not declaring what error is
thrown, node is also free to change it to
`new DOMException(message, 'AbortError')` in a future release. This
also avoids the possible addition of a public `AbortError` to node.

The thrown errors will remain instances of the internal `AbortError`
for now, to avoid effecting existing user code.

While signal.aborted can be used to detect aborted errors in most
cases, it does fully not work for the stream pipeline API, where
individual streams are destroyed with
`stream.destroy(new AbortError())`. Here the stream can no longer
fully detect aborts. However, I don't think this is ever relevant, as
streams should always perform the same cleanup logic, regardless of
what error is passed. If it doesn't support a signal option, it does
not make sense to include logic to handle signal aborts.

Refs: nodejs#40692
Refs: nodejs#38361
Refs: whatwg/dom#1027
@dead-claudia
Copy link

dead-claudia commented Feb 19, 2022

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 on ever 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.

@Jamesernator
Copy link

Jamesernator commented Feb 19, 2022

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 AbortError class and the DOMException based AbortError thing. 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.

@kanongil
Copy link
Contributor

The most safe way to test for abort errors is to check for if (signal.aborted) in your catch handler. Of course you might not have the signal available, but these cases most likely don't need to differentiate aborts from other errors.

But you still need to throw something. For a more web-safe approach new DOMException(message, 'AbortError') is probably the best approach, though this also requires some extraction if you aren't on the latest node version.

@dead-claudia
Copy link

dead-claudia commented Feb 19, 2022

The most safe way to test for abort errors is to check for if (signal.aborted) in your catch handler.

What if the signal aborts, but in a finally block on the way down there's a TypeError thrown? I can't rely strictly on that - I check both signal.aborted and the presence of an abort error when catching that for this reason.

Note that I don't really care about web safety or compatibility here - this isn't for a public library.

@Jamesernator
Copy link

Jamesernator commented Feb 20, 2022

What if the signal aborts, but in a finally block on the way down there's a TypeError thrown? I can't rely strictly on that - I check both signal.aborted and the presence of an abort error when catching that for this reason.

Is there a reason to bother checking both? Like if the idea is that some lower down code might violate the AbortSignal contract you can just ignore the error there entirely and use abortSignal.throwIfAborted() as a checkpoint instead. i.e.:

try {
    // Do some work
    await someWork();
} finally {
    // Doesn't matter if someWork() threw a different kind of error
    // or anything, this will throw the AbortError regardless if the
    // abortSignal is currently aborted 
    abortSignal.throwIfAborted();
}

Note that I don't really care about web safety or compatibility here - this isn't for a public library.

Even without shipping to the web, DOMException still exists in Node and in fact is what is set on AbortSignal by default (at least since 17.2.0) i.e.:

const AbortError = /* Get the AbortError like you did above */;

const controller = new AbortController();
controller.abort();
controller.signal.reason instanceof DOMException; // true
controller.signal.reason instanceof AbortError; // false

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be 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.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be 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.

@dead-claudia
Copy link

@benjamingr What's the status of this? I see you've removed the stale label twice, but the highly related #14554 went stale and was ultimately closed.

Hoping for an update since it's been almost a year since the last meaningful status update.

@dtinth
Copy link

dtinth commented Aug 18, 2023

I came to this issue trying to detect/throw an AbortError. From my further research, it seems that there is no such thing as an AbortError class; only an “AbortError” DOMException.

In 2023, in Node 18, I think it's pretty straightforward to create the so-called AbortError object now:

// Option 1 - Use an AbortController.
// This returns an Error object with the default message.
function createAbortError() {
  const c = new AbortController()
  c.abort()
  return c.signal.reason
}

// Option 2 - Use a DOMException.
// This approach lets you specify a custom message.
function createAbortErrorWithMessage(message) {
  return new DOMException(message, 'AbortError')
}

To detect an abort error:

function isAbortError(e) {
  return e.name === "AbortError";
}

@kanongil
Copy link
Contributor

I agree with @dtinth – there is no longer a need for this.

It is entirely replaced by the inclusion of DOMException, where it can be instantiated as done in browsers. And instanceof will never work reliably for testing.

The only maintained release line that this is relevant for is v16, which is in maintenance mode and EOL in little more than 3 weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants