Skip to content

Conversation

@tsctx
Copy link
Member

@tsctxtsctx commented Oct 28, 2024

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.

@tsctxtsctxforce-pushed the use-abortsignal-any branch from 5149e0d to c904048CompareOctober 28, 2024 12:53
@tsctxtsctx changed the title use AbortSignal.any instead of addEventListeneruse AbortSignal.any if possibleOct 28, 2024
@tsctx
Copy link
MemberAuthor

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

Copy link
Member

@KhafraDevKhafraDev left a comment

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
Copy link
MemberAuthor

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

@KhafraDev
Copy link
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.

Sign up for freeto 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

@tsctx@KhafraDev