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

lib: refactor transferable AbortSignal #43388

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 12, 2022

The use of makeTransferable on all AbortSignal instances made creating them always slow,
causing bottlenecks in stuff like Web Streams. This refactors that so that AbortSignals
are not transferable by default, which is actually the correct standard behavior anyway.
A new transferableAbortSignal and transferableAbortController utility is provided to
enable the transferable use case.

Signed-off-by: James M Snell [email protected]
Fixes: #43160

/cc @mcollina @ronag

The use of makeTransferable on all AbortSignal instances made creating them always slow,
causing bottlenecks in stuff like Web Streams. This refactors that so that AbortSignals
are not transferable by default, which is actually the correct standard behavior anyway.
A new transferableAbortSignal and transferableAbortController utility is provided to
enable the transferable use case.

Signed-off-by: James M Snell <[email protected]>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. worker Issues and PRs related to Worker support. labels Jun 12, 2022
@benjamingr
Copy link
Member

Note the failing tests

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

<!--
added: REPLACEME
-->

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> Stability: 1 - Experimental

<!--
added: REPLACEME
-->

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> Stability: 1 - Experimental

* }} [init]
* @returns {AbortSignal}
*/
function createAbortSignal(init) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function createAbortSignal(init) {
function createAbortSignal(init = kEmptyObject) {

and then requiring kEmptyObject from internal/util will probably fix the test failures. You might also want to rebase to include the change from #43159 for this to work.

@jasnell
Copy link
Member Author

jasnell commented Jul 30, 2022

Superseded by #44048

@jasnell jasnell closed this Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transferrable AbortController is very slow
6 participants