Skip to content

lib: defer AbortSignal.any() following#62367

Open
Han5991 wants to merge 5 commits intonodejs:mainfrom
Han5991:fix/abortsignal-any-retention
Open

lib: defer AbortSignal.any() following#62367
Han5991 wants to merge 5 commits intonodejs:mainfrom
Han5991:fix/abortsignal-any-retention

Conversation

@Han5991
Copy link
Contributor

@Han5991 Han5991 commented Mar 21, 2026

Summary

  • defer following source signals for AbortSignal.any() until the composite is observed
  • keep composite state in sync via lazy refresh and follow paths
  • unregister fired timeout signals from clearTimeoutRegistry to reduce timeout churn retention
  • add regression coverage for long-lived and timeout churn cases

Problem

AbortSignal.any() eagerly registered listener-less composites as dependants of their source signals. With a long-lived source, repeated AbortSignal.any([source]) calls accumulated in kDependantSignals and retained memory (#62363).

While investigating broader AbortSignal churn, timeout-only churn also kept timeout finalization registry entries alive after timers had fired, which delayed cleanup.

What Changed

AbortSignal.any()

  • listener-less composites no longer follow source signals eagerly
  • composites follow their sources only when observed
  • composite state is refreshed lazily when reading aborted, reason, or throwIfAborted()

AbortSignal.timeout()

  • fired timeout signals now unregister themselves from clearTimeoutRegistry
  • this releases timeout churn bookkeeping sooner after the timer has fired

Results

#62363 repro

  • out/Release/node stayed flat through 2,000,000 iterations
  • kDependantSignals stayed at 0

Timeout churn

Repro Before patch With patch
AbortSignal.timeout(1) after GC rss 164.55 MiB, heap 23.15 MiB rss 67.19 MiB, heap 3.96 MiB
AbortSignal.any([AbortSignal.timeout(1)]) after GC rss 135.30 MiB, heap 7.85 MiB rss 74.22 MiB, heap 4.01 MiB
Timeout churn logs

Before patch

AbortSignal.timeout(1)
start {"rss":"45.67","heap":"3.49"}
before-gc {"rss":"153.39","heap":"79.54"}
after-gc {"rss":"164.55","heap":"23.15"}
after-1-immediate {"rss":"160.23","heap":"6.28"}
after-50ms {"rss":"158.67","heap":"6.26"}

AbortSignal.any([AbortSignal.timeout(1)])
start {"rss":"45.72","heap":"3.49","handles":0}
before-gc {"rss":"133.58","heap":"32.48","handles":2}
after-gc {"rss":"135.30","heap":"7.85","handles":2}
after-1-immediate {"rss":"135.31","heap":"5.06","handles":2}
after-50ms {"rss":"135.31","heap":"5.04","handles":2}

With patch

AbortSignal.timeout(1)
start {"rss":"45.52","heap":"3.49"}
before-gc {"rss":"66.55","heap":"6.84"}
after-gc {"rss":"67.19","heap":"3.96"}
after-1-immediate {"rss":"67.20","heap":"3.88"}
after-50ms {"rss":"67.23","heap":"3.87"}

AbortSignal.any([AbortSignal.timeout(1)])
start {"rss":"45.58","heap":"3.49","handles":0}
before-gc {"rss":"73.52","heap":"9.82","handles":2}
after-gc {"rss":"74.22","heap":"4.01","handles":2}
after-1-immediate {"rss":"74.22","heap":"3.91","handles":2}
after-50ms {"rss":"74.22","heap":"3.90","handles":2}

Notes

The same-tick fresh AbortController microbenchmark from #54614 still shows heap growth until the next turn. That appears to be due to WeakRef cleanup semantics rather than long-lived source retention: the heap drops back after a setImmediate() and the weak references clear on the next turn.

Testing

  • python3 tools/test.py test/parallel/test-abortsignal-any.mjs test/parallel/test-abort-controller-any-timeout.js test/parallel/test-abortsignal-drop-settled-signals.mjs test/parallel/test-abortsignal-timeout-memory.js test/parallel/test-abortcontroller.js

Fixes: #62363
Refs: #54614

Avoid registering AbortSignal.any() composites as dependants
until they are actually observed. This fixes the long-lived
source retention pattern from nodejs#62363 while preserving abort
semantics through lazy refresh and follow paths.

Also unregister fired timeout signals from the timeout
finalization registry so timeout churn releases memory more
promptly.
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 21, 2026
@codecov
Copy link

codecov bot commented Mar 21, 2026

Codecov Report

❌ Patch coverage is 76.74419% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.68%. Comparing base (7547e79) to head (f7a442c).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/abort_controller.js 76.74% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62367      +/-   ##
==========================================
- Coverage   91.60%   89.68%   -1.92%     
==========================================
  Files         337      676     +339     
  Lines      140745   206752   +66007     
  Branches    21802    39593   +17791     
==========================================
+ Hits       128925   185433   +56508     
- Misses      11595    13465    +1870     
- Partials      225     7854    +7629     
Files with missing lines Coverage Δ
lib/internal/abort_controller.js 95.29% <76.74%> (-2.98%) ⬇️

... and 459 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
Copy link
Contributor

cc @atlowChemi

@geeksilva97 geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2026
The test depends on GC and heap behavior under a very small memory
limit, which makes it too sensitive for CI.

Remove the test and keep the cleanup change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression bug: AbortSignal.any() causing memory leak on long lived parent signal

4 participants