process: restore default signal handling and unref IPC after removeAllListeners() with no args - #35982
process: restore default signal handling and unref IPC after removeAllListeners() with no args#35982robobun wants to merge 4 commits into
Conversation
|
Updated 8:05 AM PT - Jul 29th, 2026
❌ @robobun, your commit 8c7e30a has some failures in 🧪 To try this PR locally: bunx bun-pr 35982That installs a local version of the PR into your bun-35982 --bun |
WalkthroughChangesremoveAllListeners lifecycle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the no-arg removeAllListeners() overload now fires onDidChangeListener per event type, matching the named overload and EventTarget::removeAllEventListeners().
What was reviewed:
map.eventTypes()returnsVector<Identifier>by value (refcounted), so the captured names survivemap.clear().onDidChangeListenersinBunProcess.cppgates removal onlistenerCount == 0/signalToContextIdsMappresence andKeepAlive::unrefchecksstatus != Active, so per-type calls after a bulk clear are idempotent even when bothmessageanddisconnectwere registered.eventListenersDidChange()is a no-op onEventEmitter; adding it here just matches sibling overloads.m_thisObject.clear()ordering and the return value are preserved (sole caller inJSEventEmitter.cppdiscards it).- Tests follow existing patterns in the file, drain pipes concurrently, and cover restore-default, reinstall-after-remove, and IPC unref.
Extended reasoning...
Overview
The PR fixes EventEmitter::removeAllListeners() (no-arg) in src/jsc/bindings/webcore/EventEmitter.cpp to invoke the onDidChangeListener hook for each cleared event type, mirroring what removeAllListeners(eventType) and EventTarget::removeAllEventListeners() already do. Without this, process.removeAllListeners() cleared JS listeners but left sigaction pointing at Bun's dispatcher and left the IPC KeepAlive reffed. Three subprocess tests are added to test/js/node/process/process-signal-listener-count.test.ts.
Security risks
None. This only affects when an internal C++ notification hook fires; the hook is a static function pointer set once in Process::finishCreation and does not run user JS.
Level of scrutiny
Moderate — it's native C++ in the EventEmitter core, but the change is a small, mechanical alignment with sibling code paths. I checked lifetime of the captured event-type list (IdentifierEventListenerMap::eventTypes() returns a fresh Vector<JSC::Identifier>, and Identifier refcounts its impl, so it outlives map.clear()). I checked that clearing the map before invoking the hook is the right order for this emitter: Process::onDidChangeListeners reads listenerCount(eventName) on the removal branch and needs it to be 0. I verified the hook's removal branches are idempotent (KeepAlive::unref early-returns when not Active; signal reset is gated on signalToContextIdsMap membership; memory-pressure uninstall is gated on listenerCount == 0), so firing once per cleared type — including both message and disconnect after a bulk clear — cannot double-decrement.
Other factors
eventListenersDidChange() is now called (previously wasn't) — it's an empty inline on EventEmitter, so no behavior change, just consistency with addListener/removeListener/removeAllListeners(type). The bool return value and m_thisObject.clear() behavior are unchanged for both empty and non-empty cases; the only caller (JSEventEmitter.cpp:409) ignores the return anyway. The new tests follow the exact conventions of the three existing tests in the file (subprocess with -e, Promise.all on stdout/stderr/exited, normalizeBunSnapshot, skipIf(isWindows) for signal tests). The PR description states the first and third new tests fail on canary and all pass with the fix.
|
Overlap with #33495: that PR includes this same |
There was a problem hiding this comment.
LGTM — small, well-scoped fix that brings the no-arg removeAllListeners() overload in line with its named sibling.
What was reviewed:
onDidChangeListenersin BunProcess.cpp — the removal branches for signals, IPC, and memoryPressure all gate onlistenerCount(eventName) == 0, so clearing the map before firing the hook (opposite order fromEventTarget::removeAllEventListeners) is correct here.map.eventTypes()returnsVector<Identifier>by value, so the captured identifiers survivemap.clear().- The only
EventEmitter::onDidChangeListenerconsumer isProcess; no other emitters are affected. Re-entrancy is fine — the hook body is C-level (sigaction/unref/uninstall) and the map is already empty. - Tests follow the existing patterns in both files (concurrent pipe drain, stdout asserted before exitCode, SIGUSR2 with
skipIf(isWindows)).
Extended reasoning...
Overview
The no-arg overload of EventEmitter::removeAllListeners() cleared the listener map without invoking the onDidChangeListener hook, so Process::onDidChangeListeners never ran to reset sigaction to SIG_DFL, unref the IPC channel, or uninstall the memory-pressure watcher. The named overload removeAllListeners(eventType) already did this correctly. The fix captures the event-type list before clearing, clears, calls eventListenersDidChange() (previously missing from this overload but present in every sibling), then fires the hook once per removed type — the same net effect as calling the named overload for each type.
Correctness
- Hook ordering: The hook is fired after
map.clear(), which differs fromEventTarget::removeAllEventListeners()(fires before). This is deliberate and required: BunProcess.cpp:1418/1442/1587 all checkeventEmitter.listenerCount(eventName) == 0before uninstalling, so the map must already be empty when the hook runs. - Lifetime:
map.eventTypes()returnsVector<Identifier>by value;Identifieris refcounted, so the loop is safe post-clear(). - Blast radius:
onDidChangeListeneronEventEmitteris only assigned inProcess::finishCreation(BunProcess.cpp:4549); other consumers (MessagePort, WorkerGlobalScope) useEventTarget's hook, so this change only affectsprocess. - Re-entrancy / m_thisObject: The hook body only calls native helpers (sigaction,
Bun__unrefChannelUnlessOverridden,Bun__MemoryPressure__uninstall) — no user JS runs, andm_thisObjectis not read by the hook, so clearing it after the loop is fine. The empty-map early return preserves the prior behavior of clearingm_thisObjectand returningfalse.
Security risks
None. No untrusted input parsing; the change tightens teardown so signals aren't silently swallowed.
Level of scrutiny
Moderate — native C++ in the JSC bindings, but the change is ~15 lines confined to one function and mirrors the pattern already used in the named overload and EventTarget. The mechanism (missing hook call) is clearly the root cause, not a symptom patch.
Other factors
- Four new tests cover all three symptoms (signal restoration, signal re-install after removal, IPC unref, memory-pressure disarm) plus the named-overload path as a control. Tests reuse existing helpers/patterns in their files and drain subprocess pipes concurrently.
- Overlap with #33495 is acknowledged in the thread; this is the minimal standalone slice and either can land first.
- The 2000ms
setTimeoutin the SIGUSR2 test is a bounded fallback deadline (process should die immediately viaSIG_DFL), matching the existing 1000ms pattern in the same file — not a sleep-then-check.
There was a problem hiding this comment.
LGTM — small, well-scoped fix that brings the no-arg removeAllListeners() overload in line with the named overload and EventTarget::removeAllEventListeners().
What was reviewed:
- Verified
map.eventTypes()returnsVector<Identifier>by value, so iterating aftermap.clear()is safe; hook fires post-clear solistenerCount()==0and the SIG_DFL / IPC-unref / memory-pressure paths inonDidChangeListenerstrigger correctly. - Checked the only caller (
JSEventEmitter.cppbinding) and that the hook body cannot re-enter in a way that touches the cleared map. - Tests follow existing patterns in the same files (subprocess spawn, concurrent pipe drain, signal fallback timeout mirrors the neighboring test).
Extended reasoning...
Overview
The PR fixes a single 12-line C++ function, EventEmitter::removeAllListeners() (no-arg overload), so it invokes the onDidChangeListener hook once per cleared event type — the same contract already honored by removeAllListeners(eventType), removeListener, addListener, and EventTarget::removeAllEventListeners(). Without this, process.removeAllListeners() clears the JS listener list but never resets sigaction to SIG_DFL, never unrefs the IPC channel, and never uninstalls the memory-pressure watcher. Four new tests cover each symptom plus the re-add path.
Security risks
None. The change restores default signal disposition and drops references — it does not add any new privileged operations, and the hook it now calls is the same one already invoked by every other add/remove path on process.
Level of scrutiny
Low-to-medium. The native change is tiny and mechanical: capture event types by value, clear the map, call eventListenersDidChange(), iterate the captured types through the existing hook, then clear m_thisObject. I confirmed IdentifierEventListenerMap::eventTypes() returns a fresh Vector<Identifier>, so the iteration is not over freed memory. The hook is invoked after the map is cleared, which is required — Process::onDidChangeListeners gates every teardown branch on listenerCount(name) == 0. The hook body (BunProcess.cpp:1409-1604) calls only sigaction/signal, Bun__unrefChannelUnlessOverridden, Bun__UVSignalHandle__close, and Bun__MemoryPressure__uninstall; none re-enter the emitter, and even if they did the map is already empty so the early-return guard holds.
Other factors
- The only C++ caller of the no-arg overload is the JS binding in
JSEventEmitter.cpp, so behavior change is scoped toemitter.removeAllListeners()from JS. m_thisObject.clear()now runs after the hook loop instead of before the return; the hook does not readm_thisObject(it goes throughscriptExecutionContext()->jsGlobalObject()), so ordering is immaterial.- Tests are placed alongside existing coverage in the correct files, use
bunExe/bunEnv/await using, drain stdout/stderr/exited concurrently, and the 2ssetTimeoutfallback in the SIGUSR2 test mirrors the existing 1s fallback in the neighboring "removing all signal listeners uninstalls the handler" test. - Overlap with #33495 is acknowledged in-thread; this is the minimal standalone slice and is safe to land independently.
|
CI status on the rebased branch (8c7e30a, build #85095 finished): the diff is green on every lane that ran. The new tests in The only red lane is All other annotations are parallel-batch flakes that passed when run alone or on retry; none touch the files in this PR and none are marked Ready for review. |
…removeAllListeners() The no-arg overload of EventEmitter::removeAllListeners() cleared the listener map without invoking the onDidChangeListener hook, so side effects tied to listener presence were never torn down. On the process object this left the sigaction for any registered signal pointing at forwardSignal while the listener list was empty, so SIGTERM/SIGINT/SIGHUP/SIGUSR2 were silently swallowed and the default disposition never came back. An IPC-forked child that called process.removeAllListeners() similarly never unref'd its channel and could not exit. The named overload removeAllListeners(eventType) already calls the hook; this change brings the no-arg overload in line with it and with EventTarget::removeAllEventListeners(): capture the event types, clear the map, then invoke onDidChangeListener(*this, type, false) for each removed type.
…pressure watcher The same no-arg removeAllListeners() hook skip that leaked signal handlers and the IPC channel ref also left the memory-pressure PSI watcher installed. The fix in EventEmitter.cpp already covers it; this adds the direct assertion via isMemoryPressureWatcherInstalled().
Matches the existing pattern in this file. The default-disposition assertion is already covered by the stdout and exitCode checks; the specific signal name reported by Subprocess.signalCode differs across platforms.
2e6d21b to
b12ed6a
Compare
There was a problem hiding this comment.
LGTM — small, well-scoped fix that mirrors the named removeAllListeners(eventType) overload.
What was reviewed:
map.eventTypes()returnsVector<Identifier>by value, so it's safe aftermap.clear(); hook fires post-clear solistenerCount()==0on every branch ofonDidChangeListeners.- Only
ProcesssetsEventEmitter::onDidChangeListener; that hook calls no user JS, so no reentrancy hazard onm_thisObjector the map during the loop. - Double-fire for
message+disconnectis harmless —KeepAlive::unrefearly-returns when already inactive; signal and memory-pressure paths gate on their own installed-state maps. - Tests follow the neighboring patterns in both files and cover restore-default, re-arm, IPC unref, and watcher disarm.
Extended reasoning...
Overview
The no-arg EventEmitter::removeAllListeners() overload previously cleared the listener map without invoking the onDidChangeListener hook, so Process::onDidChangeListeners never ran and OS-level side effects (sigaction, IPC channel ref, memory-pressure PSI watcher) were left installed. The fix captures event types, clears the map, then calls the hook once per removed type — the same contract the named removeAllListeners(eventType) overload already honors. Four new tests exercise the three symptoms (signal disposition, IPC keep-alive, memory-pressure watcher) plus the re-arm path.
Security risks
None. No user input parsing, no new syscall surface; the change only routes an existing internal callback through an additional call site. The signal path is teardown (restoring SIG_DFL), not installation.
Level of scrutiny
Medium — this is core EventEmitter C++ shared by every native emitter, but the hook is only ever set by Process (BunProcess.cpp:4549 is the sole assignment to EventEmitter::onDidChangeListener). I traced each branch of onDidChangeListeners for the isAdded=false case: memory-pressure and signal paths guard on listenerCount(eventName)==0, and the IPC path guards on totalListenerCount==0; because the map is cleared before the hook fires, all three see zero and take the teardown branch. eventTypes() returns a fresh Vector<Identifier> (IdentifierEventListenerMap.cpp:32), so iterating it after map.clear() is safe. eventListenersDidChange() is a no-op on EventEmitter (EventEmitter.h:106) but is now consistent with the sibling overloads. The empty-map early return preserves the old m_thisObject.clear() + return false behavior.
I checked the one interesting edge: if both "message" and "disconnect" listeners are registered, the hook fires twice with both counts already at zero, so Bun__unrefChannelUnlessOverridden is called twice. KeepAlive::unref (src/io/keep_alive.rs:43) is a status flip guarded by status != Active, so the second call is a no-op. The signal path similarly guards on signalToContextIdsMap->find(...) != end(), and memory-pressure uninstall is state-gated on the Rust side.
Other factors
The tests are placed in the existing files for signal-listener-count and memory-pressure coverage, use bunEnv/bunExe()/await using, drain pipes concurrently, and assert observable outcomes (signalCode !== null, child exit, isMemoryPressureWatcherInstalled() sequence). CI (build #82737) is green on all lanes for the modified test files; the remaining red is the binary-size check comparing against a stale canary and a known-flaky darwin no-orphans timeout, neither of which this ~15-line C++ change touches. No prior human review comments to address.
There was a problem hiding this comment.
LGTM — the no-arg removeAllListeners() now mirrors the named overload and EventTarget::removeAllEventListeners() by firing onDidChangeListener per cleared type.
What was reviewed:
map.eventTypes()returns a by-valueVector<Identifier>, somap.clear()afterward is safe; identifiers stay refcounted.Process::onDidChangeListenersre-readslistenerCount()(now 0 post-clear) and is plain C++ with no user-JS re-entry, so the per-type loop can't re-enter or observe stale state.- Empty-map path still clears
m_thisObjectand returns false, matching prior behavior;eventListenersDidChange()is a no-op onEventEmitter. - New tests cover signal SIG_DFL restore, re-install after clear, IPC unref, and memory-pressure disarm; each fails on canary per the PR body.
Extended reasoning...
Overview
The PR changes one function in src/jsc/bindings/webcore/EventEmitter.cpp: the no-argument overload of EventEmitter::removeAllListeners(). Previously it cleared the listener map and m_thisObject but never invoked eventListenersDidChange() or the onDidChangeListener hook. The fix captures map.eventTypes() (a by-value Vector<Identifier>), clears the map, calls eventListenersDidChange(), then loops the captured types calling onDidChangeListener(*this, type, false). Four new tests are added across process-signal-listener-count.test.ts and process-memory-pressure.test.ts.
Security risks
None. This is listener-lifecycle bookkeeping. The hook it now fires (Process::onDidChangeListeners in BunProcess.cpp) resets sigaction to SIG_DFL, unrefs the IPC channel, and uninstalls the memory-pressure watcher — restoring default OS behavior rather than granting anything new. No user-controlled data flows into the changed code path.
Level of scrutiny
Moderate. The C++ change is ~15 lines and structurally identical to two existing siblings: EventEmitter::removeAllListeners(const Identifier&) in the same file (which already calls both eventListenersDidChange() and onDidChangeListener) and EventTarget::removeAllEventListeners() (which loops eventTypes() firing the hook per type). I verified IdentifierEventListenerMap::eventTypes() returns a copied vector so map.clear() cannot invalidate the captured identifiers, and that eventListenersDidChange() on EventEmitter is an empty body. The only setter of onDidChangeListener on an EventEmitter is Process::finishCreation, and that hook is pure C++ (reads listenerCount(), calls Rust FFI) with no path back into user JS, so there is no re-entrancy or GC hazard from calling it in a loop after the map is cleared. The empty-map early-return preserves the prior m_thisObject.clear() + return false semantics.
Other factors
The tests follow existing conventions in both files (subprocess spawn, Promise.all for pipe drain, snapshot assertions before exit-code assertions, test.skipIf(isWindows) for signal tests). They exercise the three concrete symptoms named in the PR (SIG_DFL restore, IPC unref, memory-pressure disarm) plus the re-install-after-clear case. CI on build #82737 is green for both changed test files across all lanes; remaining red is documented as unrelated (binary-size baseline drift, an unrelated darwin flake). The overlap with #33495 is acknowledged and this PR is the minimal standalone slice. The bug hunting system found no issues.
|
Closing: the same fix landed on main in #34660 (EventEmitter::removeAllListeners() with no event name now collects the event types, clears the map, and runs eventListenersDidChange() plus onDidChangeListener for each type, so signal handlers, the memoryPressure watcher and the IPC ref are torn down). The four tests this PR adds to test/js/node/process/process-signal-listener-count.test.ts and process-memory-pressure.test.ts pass unmodified against a debug build of main at 04148c8, twice in a row. |
2 participants
Problem
process.removeAllListeners()called with no event name clears every listener but leaves the underlying OS-level side effects in place. Three user-visible symptoms:Signals are swallowed forever. After registering a signal listener and then calling
process.removeAllListeners(),listenerCountcorrectly reports 0 but thesigactionis still pointing at Bun's internal dispatcher, which now emits to an empty listener list. The default signal disposition never comes back, so a process that does this becomes unkillable by SIGTERM/SIGINT/SIGHUP/SIGUSR2 until SIGKILL. Node restoresSIG_DFLin the same scenario.IPC children never exit. A
fork()ed child that callsprocess.removeAllListeners()after adding a"message"listener never unrefs the IPC channel, so it stays alive with no work to do. Node exits 0.The memory-pressure watcher leaks. A
"memoryPressure"listener followed byprocess.removeAllListeners()leaves the PSI trigger fd open and the watcher armed for the rest of the process. Minor compared to the other two, but the same root.removeAllListeners("<name>")with the event name already worked correctly for all three.Cause
EventEmitter::removeAllListeners()(the no-arg overload,src/jsc/bindings/webcore/EventEmitter.cpp) cleared the listener map without calling theonDidChangeListenerhook.Process::onDidChangeListenersis the only place that resetssigactiontoSIG_DFL, unrefs the IPC channel, and uninstalls the memory-pressure watcher; skipping it leaves all of those installed. The named overloadremoveAllListeners(eventType)andEventTarget::removeAllEventListeners()already fire the hook per event type.Fix
Capture the registered event types before clearing, clear the map, then invoke
onDidChangeListener(*this, type, false)for each removed type. This is the same patternEventTarget::removeAllEventListeners()uses, and is equivalent to callingremoveAllListeners(type)for every type. Each of the process hooks (signal reset, IPC unref, memory-pressure uninstall) re-readslistenerCount()and is idempotent, so firing once per cleared event name is safe.Tests
test/js/node/process/process-signal-listener-count.test.ts:process.removeAllListeners()with no args restores default signal handling (process is terminated by SIGUSR2 rather than swallowing it)process.removeAllListeners()with no args then re-adding a listener reinstalls the OS handlerprocess.removeAllListeners()with no args unrefs the IPC channel so the child exitstest/js/node/process/process-memory-pressure.test.ts:process.removeAllListeners()with no args uninstalls the memory-pressure watcher (asserted viaisMemoryPressureWatcherInstalled())On canary the signal, IPC, and memory-pressure tests fail; all twelve tests across both files pass with this change.
no test proof · iteration 8 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/process/process-signal-listener-count.test.ts