Skip to content

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

Merged
merged 4 commits into from
Apr 17, 2025

Conversation

gurgunday
Copy link
Member

@gurgunday gurgunday commented Apr 13, 2025

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.js

const ITERATIONS = 10000;
const BATCH_SIZE = 100;
const TIMEOUT = 100; // Small timeout so it resolves quickly

// Record initial memory
const getMemoryUsage = () => process.memoryUsage().heapUsed / 1024 / 1024;
let initialMemory = 0;
let finalMemory = 0;
let signals = [];

async function runTest() {
  console.log('Starting memory leak test for AbortSignal.any()');
  
  // Force garbage collection before we start
  if (global.gc) {
    global.gc();
    await new Promise(resolve => setTimeout(resolve, 100));
    global.gc();
  } else {
    console.warn('Start node with --expose-gc to get more accurate results');
  }
  
  // Record initial memory after GC
  initialMemory = getMemoryUsage();
  console.log(`Initial memory usage: ${initialMemory.toFixed(2)} MB`);
  
  // Run multiple batches and force GC between them
  for (let batch = 0; batch < ITERATIONS / BATCH_SIZE; batch++) {
    signals = [];
    for (let i = 0; i < BATCH_SIZE; i++) {
      const timeoutSignal = AbortSignal.timeout(TIMEOUT);
      const compositeSignal = AbortSignal.any([timeoutSignal]);
      
      // Hold a reference to the composite signal
      signals.push(compositeSignal);
    }
    
    await new Promise(resolve => setTimeout(resolve, TIMEOUT * 2));
    
    // Clear references and force GC
    signals = [];
    if (global.gc) {
      global.gc();
      await new Promise(resolve => setTimeout(resolve, 100));
      global.gc();
    }
    
    // Log memory usage
    if (batch % 10 === 0 || batch === (ITERATIONS / BATCH_SIZE) - 1) {
      const currentMemory = 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-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 13, 2025
Copy link

codecov bot commented Apr 13, 2025

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 lines Coverage Δ
lib/internal/abort_controller.js 98.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.

@geeksilva97 geeksilva97 requested a review from atlowChemi April 14, 2025 02:47
Copy link
Contributor

@geeksilva97 geeksilva97 left a comment

Choose a reason for hiding this comment

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

LGTM

@gurgunday gurgunday added the abortcontroller Issues and PRs related to the AbortController API label Apr 14, 2025
@geeksilva97 geeksilva97 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-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2025
@nodejs-github-bot
Copy link
Collaborator

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
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
Member Author

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?

@gurgunday gurgunday requested a review from atlowChemi April 15, 2025 16:34
@geeksilva97 geeksilva97 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-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@geeksilva97 geeksilva97 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 17, 2025
@nodejs-github-bot nodejs-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:main Apr 17, 2025
63 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 9d6626a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abortcontroller Issues and PRs related to the AbortController API author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs 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