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

Expose signal.reason as error.cause for AbortError #1090

Closed
LiviaMedeiros opened this issue Jun 27, 2022 · 10 comments
Closed

Expose signal.reason as error.cause for AbortError #1090

LiviaMedeiros opened this issue Jun 27, 2022 · 10 comments

Comments

@LiviaMedeiros
Copy link

Current situation with AbortSignal aborting looks confusing.

fetch('foo://bar', {
  signal: AbortSignal.timeout(100)
}).catch(errorHandler);

async function errorHandler(err) {
  /* what `err` should have there to be useful? */
};

As an user, I want to handle err and know what happened. There are three main possibilities:

  • AbortError: in case of more complicated code, a manual/conditional cancellation
  • TimeoutError: standardized timeout
  • everything else: generic errors, specific for fetch in this case

In current implementations (Chrome 103.x, Firefox 101.x, node 18.x), err is an AbortError. There's no way to know it's a timeout, nor to retrieve signal.reason if any.
From my understanding of current specs and plans on applying them to fetch in particular, err should be the signal.reason itself: TimeoutError in this example or arbitrary non-undefined value in general.

"Current" version has an obvious issue: we can't distinguish timeout from manual abortion, and can't distinguish different abortion reasons.
"Future" version has another issue: it breaks backwards compatibility with userland code that relied on AbortError, and more importantly, disallows us to distinguish abortion errors from everything else.

With Error Cause proposal being included in ES2022, it became possible to chain errors. It sounds perfectly reasonable to me if functions aborted by AbortSignal will always throw an abortion caused by signal, i.e. AbortError having signal.reason in its cause property.

  • It would keep backwards compatibility with userland code that relies on err?.name === 'AbortError' robustness
  • It would ensure that any new standards, similar to TimeoutError, will be still recognized as aborts
  • It would allow handling various abortion reasons separately from arbitrary errors

Is there anything that makes this concept impossible or unwanted?

@annevk
Copy link
Member

annevk commented Jul 22, 2022

The discussion that introduced this feature goes into this. It's essentially a way to signal an exception. If you want to say it's an "AbortError" DOMException and provide additional data, you'd have to annotate that exception object with that data and signal with that.

It's a bit unfortunate that the Fetch integration isn't complete yet. I've added a comment to whatwg/fetch#1343 to help speed that along.

@tilgovi
Copy link

tilgovi commented Jul 23, 2022

The whatwg/fetch#1343 seems to be going in the direction of aborting with the reason from the signal, but this PR is suggesting instead to abort with an AbortError and its cause set to the signal's reason.

I've been searching for several hours through issues and don't see a resolution anywhere that's being consistently applied across environments. Node.js seems to have consistently opted for using cause (examples).

I can understand both perspectives. However, one aspect of the cause approach that seems unaddressed is that DOMException doesn't seem to specify cause yet. I haven't found an issue for that. Should I open one?

@annevk
Copy link
Member

annevk commented Jul 24, 2022

#1027 is where we decided on the design. And that mirrors whatwg/streams#1165 (comment). At least @jasnell was involved on behalf of Node.js.

I don't know where cause originates but that was never part of the design and I don't see us adding it now.

@LiviaMedeiros
Copy link
Author

From reading through the referred issues, I don't really see any exact arguments against using cause. In fact, earlier issues even had non-standard suggestions such as #951 (comment) with reason property.

During that time, error cause proposal was at Stage 3 so it's natural that it wasn't used in prior standards. It is also natural to keep consistent behavior between subsystems (hence why it's a dom issue, not fetch).
However, if compatibility is the most important factor, the upcoming changes in Fetch are going to break both backward and forward compatibility anyway. TimeoutError and arbitrary errors will appear; and in userland there will be no way to ensure that any future abort reasons will be caught and handled as abort.

If you want to say it's an "AbortError" DOMException and provide additional data, you'd have to annotate that exception object with that data and signal with that.

This is somewhat possible if we call AbortController.abort(), but not with AbortSignal.timeout().

I've been searching for several hours through issues and don't see a resolution anywhere that's being consistently applied across environments. Node.js seems to have consistently opted for using cause (examples).

Apparently there is an issue with Node.js perspective and very similar concerns: #1033.
Node.js has its own AbortError which is not a DOMException. It allows to have the desired behavior independently, but it's also likely to become a headache with more DOM API parts implemented (most notably atm, undici is spec-compliant).

However, one aspect of the cause approach that seems unaddressed is that DOMException doesn't seem to specify cause yet. I haven't found an issue for that. Should I open one?

Yes, good idea. DOMException are instances of Error and spec asks it to have all features of Errors; it would be great if DOMException() constructor supported cause as well.

@annevk
Copy link
Member

annevk commented Jul 26, 2022

The error cause proposal is archived. I don't see Error.prototype.cause defined in ECMAScript. Where is it defined?

@LiviaMedeiros
Copy link
Author

20.5.1.1 Error constructor -> 20.5.8.1 InstallErrorCause.
It's not defined as Error.prototype.cause and can be unset (i.e. not just undefined) if no cause was given: tc39/proposal-error-cause#13, tc39/proposal-error-cause#35.

@annevk
Copy link
Member

annevk commented Jul 26, 2022

Ah, whatwg/webidl#969 tracks adding cause to DOMException. Anyway, per the linked discussions we quite explicitly decided on aborting essentially being a way to signal exceptions. It's unfortunate that it isn't completed for Fetch yet, but it is for other APIs.

@domenic any thoughts on this?

@domenic
Copy link
Member

domenic commented Jul 26, 2022

Not really, I think you covered it! IMO this and #1033 should be closed.

@tilgovi
Copy link

tilgovi commented Jul 26, 2022

Thank you. I had forgotten about #1033. I think this issue is actually a duplicate of that.

I'm looking for implementation guidance for APIs that use AbortError. I wanted to see if anyone's assessment of #1033 had changed in light of cause, note that Node.js APIs and fetch seem to be taking different approaches, and that the Node.js approach was not ergonomic without the ability to create DOMExceptions with causes.

@annevk
Copy link
Member

annevk commented Jul 27, 2022

Thanks, this is a duplicate of #1033.

@annevk annevk closed this as completed Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants