Skip to content

use AbortSignal.any if possible - #3779

Open
tsctx wants to merge 2 commits into
nodejs:mainfrom
tsctx:use-abortsignal-any
Open

tsctx wants to merge 2 commits into
nodejs:mainfrom
tsctx:use-abortsignal-any

Conversation

@tsctx

@tsctx tsctx commented Oct 28, 2024 •

Copy link
Copy Markdown
Member

AbortSignal.any exists from the lowest version currently supported, which simplifies the handling of AbortSignal.
Also, tests that are no longer needed as a result of this have been removed.

@tsctx
tsctx force-pushed the use-abortsignal-any branch from 5149e0d to c904048 Compare October 28, 2024 12:53
@tsctx tsctx changed the title use AbortSignal.any instead of addEventListener use AbortSignal.any if possible Oct 28, 2024
@tsctx

tsctx commented Oct 28, 2024

Copy link
Copy Markdown
Member Author

Only available after v23 (nodejs/node@d473606) due to event firing order issues.

@KhafraDev KhafraDev left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe there are a number of memory leaks related to AbortSignal.any.

edit: Yeah, there are.
nodejs/node#54614
nodejs/node#55328
nodejs/node#55354 (which has only made it into v23.1.0 so far)

@tsctx

tsctx commented Oct 29, 2024

Copy link
Copy Markdown
Member Author

Let's wait until the fix patch lands on LTS.

@KhafraDev

Copy link
Copy Markdown
Member

There are two issues without patches yet and then we'll have to manage both the AbortSignal.any path and our own. Is there any benefit in using AbortSignal.any? I see that the issues mentioned that this PR would fix were removed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants