Repository navigation
Conversation
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66265 +/- ##
==========================================
+ Coverage 90.30% 90.48% +0.18%
==========================================
Files 789 797 +8
Lines 272880 277705 +4825
Branches 52110 53358 +1248
==========================================
+ Hits 246418 251290 +4872
+ Misses 16912 16806 -106
- Partials 9550 9609 +59
🚀 New features to boost your workflow:
|
aduh95
added
author ready
github-actions
Bot
removed
the
request-ci
The default AbortError captured a stack. Until that stack is formatted, V8 keeps the call sites, which retain the receivers and functions of the aborting caller's frames, so a retained signal kept the aborted work alive. Formatting the stack eagerly fixes the retention, but costs several microseconds per abort(). Create the default reason with Error.stackTraceLimit set to 0 instead. It retains nothing and is cheaper than capturing a stack. The default AbortError no longer shows where abort() was called; callers that want that can pass their own reason. Refs: nodejs#66192 Refs: nodejs#66265 Assisted-by: Opus 5.5 Signed-off-by: Robert Nagy <ronagy@icloud.com>
Speed up creating and aborting signals, adding and removing abort listeners, AbortSignal.any() and AbortSignal.timeout(), and stop the default abort reason from retaining the aborting caller. - Only allocate and dispatch the abort event when the signal has abort listeners, and only allocate the dependent signal list when there are dependent signals. - Track whether an event is trusted with a private field instead of a WeakSet. - Flatten addEventListener() options into locals instead of allocating a normalized options object, only validate the signal option when one is passed, and use the listener flags directly. - Call the EventTarget kNewListener and kRemoveListener implementations directly. V8 does not optimize keyed super property access such as super[kNewListener](). - Initialize the internal slots that AbortSignal.any() adds to its sources up front, and the composite-only slots for composites, so live signals don't change shape. - Give every signal a single WeakRef that is shared by all the composites that follow it, and register each composite and each source once with their cleanup registries instead of registering every (source, composite) pair with an unregister token. Registering with an unregister token is several times more expensive. - Register AbortSignal.timeout() timers through a holder that is emptied when the timer fires, instead of using the signal as an unregister token. - Take positional arguments in AbortSignal's internal constructor instead of destructuring an options object that defaults to kEmptyObject. kEmptyObject is a null-prototype object, which V8 keeps in dictionary mode, so each of those reads was an uncached dictionary miss that cost more than the rest of the constructor. - Create the default abort reason with Error.stackTraceLimit set to 0. Until a captured stack is formatted, V8 keeps the call sites, which retain the receivers and functions of the aborting caller's frames, so a retained signal kept the aborted work alive. Formatting the stack eagerly would fix that but costs several microseconds per abort(). The default AbortError no longer shows where abort() was called; callers that want that can pass their own reason. - Add benchmarks for aborting, adding and removing abort listeners, and AbortSignal.any(). Refs: nodejs#66192 Refs: nodejs#66265 Assisted-by: claude:opus-5.5 Signed-off-by: Robert Nagy <ronagy@icloud.com>
b03f41d to
8602dbb
Compare
kuldeeepy
changed the title
There was a problem hiding this comment.
Shouldn't we be using lazyDOMException?
Lines 684 to 697 in 9347db1
There was a problem hiding this comment.
I don't think it fits here. lazyDOMException sets the limit to 0 only to skip the constructor frame, then calls ErrorCaptureStackTrace(ex, lazyDOMException), which captures the caller's stack again. Those call sites are what keep the aborted work alive.
I checked it with the repro from the issue: with lazyDOMException the 4 × 32 MiB jobs stay retained (128 MiB), with this PR they're collected. It's also ~2.5x slower than the current version.
There was a problem hiding this comment.
I haven’t checked your findings, but shouldn’t we then fix lazyDOMException?
There was a problem hiding this comment.
I don't think it's a lazyDOMException bug. Any error with an unformatted stack retains the caller's frames, a plain new Error() does the same (128 MiB with the same repro). lazyDOMException is mostly used for errors that get thrown, where the stack is what users want, so dropping it there would be a much bigger change.
The default abort reason is different because it's created eagerly, usually never thrown, and the signal often outlives the work. That's why I kept the fix local to it. The general retention is more of a V8 question, as @jasnell mentioned in the issue.
There was a problem hiding this comment.
Having two copies of the same code with a slight unexplained variation is a maintenance nightmare, so I feel strongly the current state is not acceptable. We need to either make this code reuse the other implementation (by e.g. adding a flag to it), or add comments explaining what’s the variation and why it was chosen
There was a problem hiding this comment.
Fair point, I now added a captureStack flag to lazyDOMException (defaulting to true, so existing callers are unchanged) and createDefaultAbortReason() now uses it.
There was a problem hiding this comment.
Did you validate it performs as well as the separate implementation? Asking in case the performance drop was coming from the "laziness" on top of the stack trace capture
There was a problem hiding this comment.
Yes, they perform the same, ~3.4M ops/s for both on the same build. The earlier drop was from formatting the stack, which neither version does now.
The default AbortError created by AbortController.prototype.abort() and AbortSignal.abort() captures a stack that V8 keeps unformatted. Until it is formatted, V8 holds on to the call sites, which retain the receivers and functions of the calling frames. A signal that outlives the work it cancelled can then keep that work alive, including large buffers. Create the default reason through lazyDOMException() with a new captureStack flag set to false, so no call sites are captured. This also makes abort() without a reason faster, as the stack is no longer collected. Callers that want a stack can pass their own reason. Reasons passed by the caller are left untouched. Signed-off-by: Kuldeep Yadav <kuldeeep.yadav1@gmail.com>
8602dbb to
8622466
Compare
4 participants
When
abort()is called without a reason, the defaultAbortErrorkeeps its stack unformatted, and V8 then holds on to the caller's frames (receivers and closures). A retained signal can keep the cancelled job and its buffers alive.This creates the default reason with
Error.stackTraceLimitset to 0, so no call sites are captured in the first place. It covers bothcontroller.abort()andAbortSignal.abort().AbortSignal.timeout()doesn't have this problem because its reason is created in a timer callback. Reasons passed by the caller aren't touched.The default reason's
stackis now justAbortError: This operation was aborted. Callers that want the stack can pass their own reason.abort()without a reason also gets faster (static abort benchmark: about 569k to 3.4M ops/s). Passing a reason isn't affected.Fixes: #66192
Disclosure: prepared with assistance from Claude Code. I reproduced the leak, checked the fix against the source, and ran the new and existing abort tests on a local build.