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

Node AbortError Behavior meeting #36084

Closed
benjamingr opened this issue Nov 11, 2020 · 17 comments
Closed

Node AbortError Behavior meeting #36084

benjamingr opened this issue Nov 11, 2020 · 17 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core.

Comments

@benjamingr
Copy link
Member

benjamingr commented Nov 11, 2020

Hey, the issue of "what errors do we abort with?" was raised up multiple times in PRs by several users.

Other than "because of the primitive, aborting should always cause an error" I'm not sure we have consensus on anything.

I figured it would be useful to get interested parties in one room a-la-summit-style and talk this through.

Please feel free to suggest items for the agenda below.

Pinging people I talked to about this from Node in issues:

Pinging people I talked to about this as well but are not node core:

  • @bterlson (TC39, azure SDK maintainer, looking into improving AbortSingal)
  • @littledan (TC39, is Daniel a project member I always see him in events 😅 also very helpful and helping with this)
  • @MadaraUchiha (wrote the HTTP2 AbortSignal PR)
  • @benlesh (RxJS author whom I talked to a lot about AbortSignals and seemed interested).

I'm also going to ping @jakearchibald in case he can make it because I think his feedback would be very valuable, though I know he's busy - worth a shot :]


I want to make it clear that participation is not limited to the above people or to people from the Node org or any standards body. If you are interested in cancellation and feel like you can contribute to this decision please speak up and let me know.

Doodle

https://doodle.com/poll/ug26rc9cnmeuacr4

Agenda

  • Overview of AbortSignal in the WHATWG DOM spec and their guidance regarding aborting actions.
  • Discuss how user code typically aborts code and how controllers/signals are used.
  • Discuss how user code typically checks for aborted operations:
    • Do we want different aborts to have different .codes or the same code?
    • Do we want our AbortErrors for core APIs to be DOMExceptions or not? (e.g. backporting/userland code implications for readable-stream).
    • How should users identify Abort errors? (Do we want to supply specific codes for Node specific APIs and provide .name === 'AbortError' for web compatibility?

Out of scope:

  • Usability improvements for AbortSignal (very interesting, but not for this meting).
@benjamingr benjamingr added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Nov 11, 2020
@benjamingr
Copy link
Member Author

Update, due to family reasons of a certain person we want to attend we'll try to make this next week and not the week after. I will update the doodle soon

@benjamingr
Copy link
Member Author

Update: new doodle, sorry https://doodle.com/poll/ug26rc9cnmeuacr4 ping @MadaraUchiha @ronag and @mcollina

@mcollina
Copy link
Member

Done

@benjamingr
Copy link
Member Author

Looks like we converged on a time :] I've sent you all a calendar invite with a zoom link - if anyone else wants to attend you are welcome to (send me an email and I'll add you to the invite). The time is also updated on the doodle.

@ronag
Copy link
Member

ronag commented Nov 12, 2020

@benjamingr I don't think I received an invite.

@benjamingr
Copy link
Member Author

I sent it to your iCloud account @ronag - I will re-invite you

@ronag
Copy link
Member

ronag commented Nov 12, 2020

It ended up in spam. Fixed. Thanks!

@jakearchibald
Copy link

I don't think I got an invite btw

@benjamingr
Copy link
Member Author

@jakearchibald sorry, I only sent it to people who filled the doodle - I sent you an invite now :]

@benjamingr
Copy link
Member Author

Made an empty doc to (hopefully) summarise during the meeting https://docs.google.com/document/d/1aZzdQbo2G2wpGewxnkXaBoY0z9BQgv_caq9651g-OLs/edit?usp=sharing

@mhdawson
Copy link
Member

@benjamingr can you send me an invite to the meeting as well?

@mhdawson
Copy link
Member

Nevermind I see I'm too late from the doodle.

@benjamingr
Copy link
Member Author

Hey sorry @mhdawson the meeting just happened the minutes are here: https://docs.google.com/document/d/1aZzdQbo2G2wpGewxnkXaBoY0z9BQgv_caq9651g-OLs/edit

We can discuss any compelling arguments to change the decision but it was basically:

-We want cancellation to be an exception (on streams in particular) with a .code property.

  • AbortError class that doesn’t have to be a DOMException
  • just class - AbortError extends Error
  • Use .name - AbortError for all cancellations
  • .code - `ABORT_ERR`` (just like the DOMException one)
  • Let’s not encourage instanceof checks.

I will update the PR at #36048 tomorrow :]

@mhdawson
Copy link
Member

@benjamingr thanks for the summary :)

@jakearchibald
Copy link

I'm really happy with the direction here, particularly "Use .name - AbortError for all cancellations", which means the typical pattern can be:

startSpinner();
try {
  await doThing({ signal });
} catch (error) {
  if (error.name === 'AbortError') return;
  reportError(error);
} finally {
  stopSpinner();
}

@jakearchibald
Copy link

I wonder if we can encourage folks to throw the right kind of error in the case of abort? whatwg/dom#927

@benjamingr
Copy link
Member Author

I've aligned the behaviour of http.request to the consensus #36048

http.request will throw with an error, after merging it I'll work on the draft PR for (general) readable streams.

MadaraUchiha added a commit to MadaraUchiha/node that referenced this issue Nov 21, 2020
- Builds on nodejs#36048 and nodejs#36084
- Modify test to verify this fact
benjamingr added a commit to benjamingr/io.js that referenced this issue Dec 7, 2020
We added an `AbortError` with the same code and name as the web's as
part of the consensus in nodejs#36084 but
never actually documented the error in our error codes list.

This PR adds the error code.
benjamingr added a commit that referenced this issue Dec 9, 2020
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.
Trott pushed a commit that referenced this issue Dec 9, 2020
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

PR-URL: #36319
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Dec 21, 2020
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

PR-URL: #36319
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue May 16, 2021
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

PR-URL: #36319
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Jun 11, 2021
We added an `AbortError` with the same code and name as the web's as
part of the consensus in #36084 but
never actually documented the error in our error codes list.

This PR adds the error code.

PR-URL: #36319
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

No branches or pull requests

5 participants