-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
lib: fix AbortSignal.any() with timeout signals #57867
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57867 +/- ##
========================================
Coverage 90.23% 90.24%
========================================
Files 630 630
Lines 185518 185726 +208
Branches 36369 36410 +41
========================================
+ Hits 167401 167604 +203
+ Misses 11005 10999 -6
- Partials 7112 7123 +11
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
const duration = Date.now() - start; | ||
|
||
// Verify the timeout happened at approximately the right time (with some margin) | ||
assert.ok(duration >= 8900, `Timeout happened too early: ${duration}ms`); | ||
assert.ok(duration < 11000, `Timeout happened too late: ${duration}ms`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this?
Can't we validate the reason
is a timeout signal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, does it look better?
Landed in 9d6626a |
Fixes #57736
This PR fixes an issue where AbortSignal.any([AbortSignal.timeout(ms)]) would sometimes fail. The problem occurred because timeout signals inside a composite signal (created by AbortSignal.any()) were being garbage collected before they could fire.
I don't know how to make a simpler test for this honestly. Open to suggestions.
I used the following test to check for memory leaks. I assume that it works because when I initially made a mistake trying to fix this, it did cause a memory leak and it was reflected in this test:
After:
Before: