Skip to content

lib: add AbortSignal.timeout#40899

Closed
jasnell wants to merge 2 commits intonodejs:masterfrom
jasnell:abortsignal-timeout
Closed

lib: add AbortSignal.timeout#40899
jasnell wants to merge 2 commits intonodejs:masterfrom
jasnell:abortsignal-timeout

Conversation

@jasnell
Copy link
Copy Markdown
Member

@jasnell jasnell commented Nov 20, 2021

Builds on from the AbortSignal.reason PR, which should land first.

whatwg/dom#1032 introduces a new AbortSignal.timeout() that returns an AbortSignal that triggers in the given number of milliseconds.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 20, 2021
Comment thread doc/api/globals.md
Comment thread lib/internal/abort_controller.js
Comment thread lib/internal/abort_controller.js
@benjamingr
Copy link
Copy Markdown
Member

I am not too happy this makes it hard to determine an error is cancellation

@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Nov 21, 2021

I am not too happy this makes it hard to determine an error is cancellation

I don't understand. This sets the reason to a TimeoutError whose stack clearly shows it coming from an AbortSignal. And the code still has to be written to use the reason, which is optional. Can you explain?

@jasnell jasnell force-pushed the abortsignal-timeout branch 2 times, most recently from a8362d8 to f04a7fb Compare November 21, 2021 15:31
@jasnell jasnell added the abortcontroller Issues and PRs related to the AbortController API label Nov 21, 2021
@benjamingr
Copy link
Copy Markdown
Member

I don't understand. This sets the reason to a TimeoutError whose stack clearly shows it coming from an AbortSignal. And the code still has to be written to use the reason, which is optional. Can you explain?

Yes, if I have code that might cancel because of a timeout or any other cancellation before .reason I could just check err.name === 'AbortError' which was the consensus reached last time this was discussed.

This method adds a new way to timeout an action that rejects with an error that isn't AbortError. I want to make sure users have a clean way to handle this.

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 21, 2021
@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Nov 21, 2021

Similar to the AbortSignal.reason, I don't think this actually breaks any patterns. Yes, it sets the reason to the TimeoutError but it's still up to applications to pay attention to that property. Any existing code that handles the abort event that ends up creating and throwing the AbortError instead will continue to work as expected.

@nodejs-github-bot

This comment has been minimized.

Copy link
Copy Markdown
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_

Comment thread test/parallel/test-abortcontroller.js Outdated
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Nov 25, 2021

@targos ... please take another look. I updated the implementation to ensure that the underlying timer would not prevent the AbortSignal from being garbage collected and to unref the timer object so that the timer would not keep the event loop from exiting.

@jasnell jasnell force-pushed the abortsignal-timeout branch from cf9cb71 to ed3f0f9 Compare November 25, 2021 18:25
@targos
Copy link
Copy Markdown
Member

targos commented Nov 25, 2021

I'll have a look tomorrow.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Signed-off-by: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

abortcontroller Issues and PRs related to the AbortController API author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants