doc: remove all references to internal AbortError #40883

kanongil · 2021-11-19T13:44:00Z

This resolves issues around the use of an internal AbortError and better aligns node towards where AbortController / AbortSignal is headed.

Testing the rejected error is not really safe, especially if adopted for third-party code. This also makes for a smoother transition if node APIs adopt signal.reason. Refs: nodejs#40692 Refs: whatwg/dom#1027

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

aduh95 · 2021-11-19T16:58:45Z

doc/api/child_process.md


 ```js
 const { exec } = require('child_process');
 const controller = new AbortController();
 const { signal } = controller;
 const child = exec('grep ssh', { signal }, (error) => {
-  console.log(error); // an AbortError
+  console.log('aborted', signal.aborted); // true


If find the comment confusing. Can we change this?

Suggested change

console.log('aborted', signal.aborted); // true

console.log('aborted', signal.aborted); // aborted true

or

Suggested change

console.log('aborted', signal.aborted); // true

console.log(signal.aborted); // true

or

Suggested change

console.log('aborted', signal.aborted); // true

console.log('aborted');

console.log(signal.aborted); // true

Sure, this was just an initial pitch. I'd probably go with your first suggestion (which I also considered).

The second is cleaner, but makes less sense in real code, since there is no context to the logged value.

I'd prefer option 2, doc examples priority is clarity, it's OK if it doesn't have to look like "real" code (one could argue that "real" code should not use console.log at all). It seems to me that the rest of the docs tend to not have context with the console.log calls, but go straight to logging the actual value.

benjamingr · 2021-11-21T07:43:56Z

In all these cases you'd still get an AbortError no?

kanongil · 2021-11-21T10:24:31Z

In all these cases you'd still get an AbortError no?

Yes, as the code is right now. But node would no longer be bound to use these.

kanongil · 2021-11-21T10:30:23Z

This PR would probably work better if ABORT_ERR is also deprecated, since it won't be compatible with the DOMException .code property (though it says that it is named for that…).

node/doc/api/errors.md

Lines 620 to 631 in e31d1cb

    
           ### `ABORT_ERR` 
        
           <!-- YAML 
        
           added: v15.0.0 
        
           --> 
        
           Used when an operation has been aborted (typically using an `AbortController`). 
        
           APIs _not_ using `AbortSignal`s typically do not raise an error with this code. 
        
           This code does not use the regular `ERR_*` convention Node.js errors use in 
        
           order to be compatible with the web platform's `AbortError`.

benjamingr · 2021-11-21T10:42:04Z

I am not sure this change positively impacts the quality of our docs and I'd like to spend time thinking on this and see if we can improve the spec so there is a consistent way to figure out when an error originated in an AbortSignal.

kanongil · 2021-11-21T10:52:33Z

Relying on signal.aborted can potentially help prevent errors that could occur with just checking err.name === 'AbortError'. Eg. for existing user code could produce an error with that name (eg. to signal a client abort), and mistakenly handle it as a signal abort.

kanongil · 2021-11-21T10:55:45Z

I am not sure this change positively impacts the quality of our docs and I'd like to spend time thinking on this and see if we can improve the spec so there is a consistent way to figure out when an error originated in an AbortSignal.

Sure, I mostly created this to illustrate a potential way forward, that is more compatible with where DOM is heading.

kanongil · 2021-11-21T11:03:47Z

@benjamingr I do hope you don't take to deliberate, since I don't think the current recommendation to check err.name === 'AbortError' will stand the test of time, and the sooner node can stop recommending this practice, the better.

Maybe just merge the less controversial first part 7518743?

benjamingr · 2021-11-21T11:08:13Z

I would like to see what the guidance of whatwg/dom#1033 will be.

Sure, I mostly created this to illustrate a potential way forward, that is more compatible with where DOM is heading.

I would like to better understand where the DOM is heading with the error handling story. WHATWG has been nothing but cooperative in the past so I would want to better understand how they suggest we handle this use case.

I think a good error handling story is crucial for us and I'd like to make sure we give users proper guidance.

Note this isn't super urgent since reason isn't merged in yet.

kanongil · 2021-11-21T11:18:32Z

Note this isn't super urgent since reason isn't merged in yet.

It has been merged in whatwg/dom#1027, and also in WebKit: https://bugs.webkit.org/show_bug.cgi?id=232299

Or are you talking about node?

benjamingr · 2021-11-21T12:53:54Z

This is Node.js so yes, we are talking about Node.js :) Unlike the web Node.js is versioned, this is a breaking change in the API so it will likely go on v18 (I've tagged the reason PR appropriately).

We can change the doc recommendations but identifying cancellation is a really important use case in my opinion.

benjamingr

Blocking while I think about this, I think the reason change has big implications and we need to consider things carefully and figure out what the project strategy is.

I am thinking a call/summit/meeting with stakeholders as well as library maintainers and big codebase maintainers we spoke with last time.

jasnell

This LGTM with @aduh95's suggestions.

I disagree with the idea that this is a breaking change in any way. Since this PR is dealing only with documentation, it's recommendation to use signal.aborted to determine if the operation was canceled is perfectly fine.

benjamingr · 2021-11-21T17:07:40Z

@jasnell note in all those examples in the docs AbortError is still thrown so this makes the docs less specific about errors.

tilgovi · 2022-07-28T01:46:06Z

Is checking signal.aborted actually sufficient to know if the error is due to an abort? Might it be the case that abort was called but a different error was thrown because abort is a best effort?

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 19, 2021

kanongil mentioned this pull request Nov 19, 2021

Thrown AbortErrors are not DOMExceptions #40692

Open

Trott requested a review from benjamingr November 19, 2021 13:52

kanongil added 2 commits November 19, 2021 15:28

doc: never test for aborts using err.name

7518743

Testing the rejected error is not really safe, especially if adopted for third-party code. This also makes for a smoother transition if node APIs adopt signal.reason. Refs: nodejs#40692 Refs: whatwg/dom#1027

kanongil force-pushed the aborterror-doc-fix branch from 222f602 to 937a17c Compare November 19, 2021 14:28

aduh95 reviewed Nov 19, 2021

View reviewed changes

benjamingr requested changes Nov 21, 2021

View reviewed changes

jasnell approved these changes Nov 21, 2021

View reviewed changes

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: remove all references to internal AbortError #40883

doc: remove all references to internal AbortError #40883

kanongil commented Nov 19, 2021

aduh95 Nov 19, 2021

kanongil Nov 21, 2021

aduh95 Nov 22, 2021 •

edited

Loading

benjamingr commented Nov 21, 2021

kanongil commented Nov 21, 2021

kanongil commented Nov 21, 2021

benjamingr commented Nov 21, 2021

kanongil commented Nov 21, 2021

kanongil commented Nov 21, 2021

kanongil commented Nov 21, 2021 •

edited

Loading

benjamingr commented Nov 21, 2021

kanongil commented Nov 21, 2021

benjamingr commented Nov 21, 2021

benjamingr left a comment

jasnell left a comment

benjamingr commented Nov 21, 2021

tilgovi commented Jul 28, 2022

	console.log('aborted', signal.aborted); // true
	console.log('aborted', signal.aborted); // aborted true

	console.log('aborted', signal.aborted); // true
	console.log(signal.aborted); // true

	console.log('aborted', signal.aborted); // true
	console.log('aborted');
	console.log(signal.aborted); // true

doc: remove all references to internal AbortError #40883

Are you sure you want to change the base?

doc: remove all references to internal AbortError #40883

Conversation

kanongil commented Nov 19, 2021

aduh95 Nov 19, 2021

Choose a reason for hiding this comment

kanongil Nov 21, 2021

Choose a reason for hiding this comment

aduh95 Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

benjamingr commented Nov 21, 2021

kanongil commented Nov 21, 2021

kanongil commented Nov 21, 2021

benjamingr commented Nov 21, 2021

kanongil commented Nov 21, 2021

kanongil commented Nov 21, 2021

kanongil commented Nov 21, 2021 • edited Loading

benjamingr commented Nov 21, 2021

kanongil commented Nov 21, 2021

benjamingr commented Nov 21, 2021

benjamingr left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

benjamingr commented Nov 21, 2021

tilgovi commented Jul 28, 2022

aduh95 Nov 22, 2021 •

edited

Loading

kanongil commented Nov 21, 2021 •

edited

Loading