Skip to content

lib: fix AbortSignal.any() with timeout signals - #57867

Merged
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
gurgunday:fix/abortsignal-any
Apr 17, 2025
Merged

lib: fix AbortSignal.any() with timeout signals#57867
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
gurgunday:fix/abortsignal-any

Conversation

@gurgunday

@gurgundaygurgunday commented Apr 13, 2025

Copy link
Copy Markdown
Member

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.

  • Modified AbortSignal.any() to identify timeout signals and add them to the gcPersistentSignals set
  • Added cleanup for source signals when a composite signal aborts

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:

gurgunday@DELL-G15:~/work/oss/node$ ./out/Release/node --expose-gc leak-test.js
Starting memory leak test for AbortSignal.any()
Initial memory usage: 3.62 MB
Batch 1/100, Memory: 3.74 MB
Batch 11/100, Memory: 3.83 MB
Batch 21/100, Memory: 3.84 MB
Batch 31/100, Memory: 3.84 MB
Batch 41/100, Memory: 3.86 MB
Batch 51/100, Memory: 3.87 MB
Batch 61/100, Memory: 3.86 MB
Batch 71/100, Memory: 3.90 MB
Batch 81/100, Memory: 3.92 MB
Batch 91/100, Memory: 3.92 MB
Batch 100/100, Memory: 3.92 MB
Test completed:
- Initial memory: 3.62 MB
- Final memory: 3.93 MB
- Difference: 0.30 MB

Before:

gurgunday@DELL-G15:~/work/oss/node$ node --expose-gc leak-test.js
Starting memory leak test for AbortSignal.any()
Initial memory usage: 3.65 MB
Batch 1/100, Memory: 3.76 MB
Batch 11/100, Memory: 3.78 MB
Batch 21/100, Memory: 3.82 MB
Batch 31/100, Memory: 3.87 MB
Batch 41/100, Memory: 3.87 MB
Batch 51/100, Memory: 3.88 MB
Batch 61/100, Memory: 3.88 MB
Batch 71/100, Memory: 3.93 MB
Batch 81/100, Memory: 3.94 MB
Batch 91/100, Memory: 3.94 MB
Batch 100/100, Memory: 3.95 MB
Test completed:
- Initial memory: 3.65 MB
- Final memory: 3.96 MB
- Difference: 0.30 MB
'use strict';// This test creates many composite signals with timeouts // and checks if memory usage grows unbounded// We use --expose-gc to manually trigger garbage collection// Run with: node --expose-gc leak-test.jsconstITERATIONS=10000;constBATCH_SIZE=100;constTIMEOUT=100;// Small timeout so it resolves quickly// Record initial memoryconstgetMemoryUsage=()=>process.memoryUsage().heapUsed/1024/1024;letinitialMemory=0;letfinalMemory=0;letsignals=[];asyncfunctionrunTest(){console.log('Starting memory leak test for AbortSignal.any()');// Force garbage collection before we startif(global.gc){global.gc();awaitnewPromise(resolve=>setTimeout(resolve,100));global.gc();}else{console.warn('Start node with --expose-gc to get more accurate results');}// Record initial memory after GCinitialMemory=getMemoryUsage();console.log(`Initial memory usage: ${initialMemory.toFixed(2)} MB`);// Run multiple batches and force GC between themfor(letbatch=0;batch<ITERATIONS/BATCH_SIZE;batch++){signals=[];for(leti=0;i<BATCH_SIZE;i++){consttimeoutSignal=AbortSignal.timeout(TIMEOUT);constcompositeSignal=AbortSignal.any([timeoutSignal]);// Hold a reference to the composite signalsignals.push(compositeSignal);}awaitnewPromise(resolve=>setTimeout(resolve,TIMEOUT*2));// Clear references and force GCsignals=[];if(global.gc){global.gc();awaitnewPromise(resolve=>setTimeout(resolve,100));global.gc();}// Log memory usageif(batch%10===0||batch===(ITERATIONS/BATCH_SIZE)-1){constcurrentMemory=getMemoryUsage();console.log(`Batch ${batch+1}/${ITERATIONS/BATCH_SIZE}, Memory: ${currentMemory.toFixed(2)} MB`);}}finalMemory=getMemoryUsage();console.log(`\nTest completed:`);console.log(`- Initial memory: ${initialMemory.toFixed(2)} MB`);console.log(`- Final memory: ${finalMemory.toFixed(2)} MB`);console.log(`- Difference: ${(finalMemory-initialMemory).toFixed(2)} MB`);}runTest().catch(console.error);

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 13, 2025
@codecov

codecovBot commented Apr 13, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (964e41c) to head (77f6539).
Report is 31 commits behind head on main.

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 
Files with missing linesCoverage Δ
lib/internal/abort_controller.js98.26% <100.00%> (+0.12%)⬆️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@geeksilva97geeksilva97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@gurgundaygurgunday added the abortcontroller Issues and PRs related to the AbortController API label Apr 14, 2025
@geeksilva97geeksilva97 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 14, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment threadtest/parallel/test-abort-controller-any-timeout.js Outdated
Comment on lines +29 to +33
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`);

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.

Do we really need this?
Can't we validate the reason is a timeout signal?

Copy link
Copy Markdown
MemberAuthor

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?

@geeksilva97geeksilva97 added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 16, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@geeksilva97geeksilva97 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 17, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 17, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 9d6626a into nodejs:mainApr 17, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 9d6626a

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 18, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 19, 2025
PR-URL: #57867
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@ghostghost mentioned this pull request Jun 8, 2025
sholladay added a commit to sholladay/sky that referenced this pull request Aug 7, 2025
The relevant bug was fixed upstream:
nodejs/node#57867
jamesbhobbs added a commit to deepnote/deepnote that referenced this pull request Jul 27, 2026
`AbortSignal.any([caller, AbortSignal.timeout(ms)])` has a history of dropping
the timeout: the composed signal can be collected while the request is in
flight, so the deadline never fires and the request hangs — the exact failure
the deadline exists to prevent (nodejs/node#57736, fixed by nodejs/node#57867).
Raising the package's Node floor would fix it, but that is a breaking
constraint on every consumer to work around an implementation detail. Wiring
the controller by hand behaves identically on every supported version, and
lets the timer be cleared once the request settles rather than leaving one
pending per request for its full timeout. The body read moved inside the
deadline too — headers can arrive promptly while the body stalls.
Also from review:
- Docs: `--session` is cloud-only, so the flag table says so.
- Docs: the isolation wording implied `storageMode: 'readonly'` removed
project-file interaction. It stops *writes*; the session still reads the same
shared files. Split into what is and is not isolated.
- Docs: the sessions example called `getNotebook` without importing it, and
passed the optional `sessionNotebookId` where a string is required — neither
would have compiled if copied.
- Tests: the pending-fetch helper now rejects immediately for an
already-aborted signal, as real fetch does. Added cases for timer cleanup on
both the success and failure paths, and for an already-aborted caller signal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitaicoderabbitaiBot mentioned this pull request Jul 30, 2026
16 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

abortcontrollerIssues and PRs related to the AbortController APIauthor readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AbortSignal.any() is unreliable and breaks timeouts

5 participants

@gurgunday@nodejs-github-bot@jasnell@geeksilva97@atlowChemi