process: register the default 'warning' listener at process creation, like Node - #37344
Conversation
…ted, like Node
The default warning printer was installed lazily on the first emitWarning(),
so process.removeAllListeners('warning') at startup (the usual way to opt out
of the default print) had no effect, listenerCount('warning') was 0, and a bare
process.emit('warning', err) printed nothing. Node registers onWarning during
bootstrap, so all three behave the other way there.
Register a native "onWarning" stub as a real listener in Process::finishCreation
(before onDidChangeListener is wired, so it does not load the signal tables);
the JS printer is still built on the first warning and the stub forwards to it.
--no-warnings / NODE_NO_WARNINGS=1 register nothing, and a runtime
process.noProcessWarnings assignment is now honored inside the printer, which
the vendored Node test harness relies on.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe warning handler is now created by Process warning listener
macOS CI targets
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it moves listener registration into Process::finishCreation (every process/worker startup) and adds new GC-visible C++ state, a human look would still be worthwhile.
What was reviewed:
m_onWarningWriteBarrier is visited invisitChildrenImpl; the nativeonWarningJSFunction is rooted via the EventEmitter's listener map.- ThrowScope discipline in
ensureOnWarningand the trampoline (RETURN_IF_EXCEPTIONafter the factory call,RELEASE_AND_RETURNon the tail call). installDefaultWarningListenerruns beforeonDidChangeListeneris wired, so the add doesn't load signal tables; thesetThisObject(this)skipped under--no-warningsis harmless because the trampoline readsglobalObject->processObject()directly and the JS-binding paths set it on first.on()/.emit()anyway.- The
process.noProcessWarningscheck moving from install-time to per-call keeps the vendored Node harness working and doesn't diverge from Node (which only ever seeds it read-only from--no-warnings).
Extended reasoning...
Overview
Moves the default 'warning' printer from lazy install (on first emitWarning()) to eager registration in Process::finishCreation, matching Node's setupWarningHandler. What's registered up front is a native host-function stub named onWarning; on first invocation it builds the real JS printer via createOnWarning (the renamed installOnWarningListener, now returning the closure instead of calling prependListener) and caches it in a new WriteBarrier<JSObject> m_onWarning. The lazy-install guard bool and the install call in emitWarningErrorInstance are removed. Tests add an 8-case subprocess block; the vendored test/common/index.js comment is updated to match the new per-call noProcessWarnings read.
Security risks
None. No untrusted input parsing, no auth/crypto, no filesystem writes on the new startup path (the --redirect-warnings require('node:fs') still happens only on the first actual warning).
Level of scrutiny
Medium-high: this runs during Process::finishCreation for every VM (main thread + every worker_threads Worker), and adds GC-visible C++ state. The change is small and follows established patterns (WriteBarrier + visitChildrenImpl, JSEventListener::create for native listeners, ThrowScope + RELEASE_AND_RETURN on the forwarding call), but startup-path changes to process deserve a second pair of eyes.
Other factors
- The trampoline doesn't depend on
callFrame->thisValue()— it fetchesglobalObject->processObject()— so thesetThisObject(this)being skipped under--no-warnings(early return) has no effect on correctness; pre-PRfinishCreationnever called it either. - Test coverage is thorough: startup
listenerCount, listener.name, remove-by-reference,removeAllListenersbefore first warning, print-before-user-listener ordering, bareprocess.emit('warning'),--no-warnings/NODE_NO_WARNINGS=1, and per-Worker registration. All subprocess-isolated with concurrent execution. - No prior reviews on the timeline.
dylan-conway
left a comment
dylan-conway
left a comment
There was a problem hiding this comment.
Direction looks right and matches Node's setupWarningHandler (eager registration, lazy printer). Startup cost is one host JSFunction + one JSEventListener + the listener-map entry, only when process is materialized — not a concern. A few notes inline.
| return; | ||
| auto* globalObject = defaultGlobalObject(this->globalObject()); | ||
| auto* onWarning = JSFunction::create(vm, globalObject, 1, "onWarning"_s, Process_functionDefaultOnWarning, ImplementationVisibility::Public); | ||
| wrapped().addListener(builtinNames(vm).warningPublicName(), WebCore::JSEventListener::create(*onWarning, *this, false, globalObject->world()), false, false); |
There was a problem hiding this comment.
This needs vm.writeBarrier(this, onWarning); after the add, same as the JS path does in JSEventEmitter::addListener (JSEventEmitter.cpp:288). JSEventListener::m_jsFunction is a Weak<> that's only kept alive through visitAdditionalChildren → visitJSEventListeners, and being inside finishCreation doesn't make the barrier unnecessary: JSFunction::create above is an allocation/safepoint between Process's own allocation and this store, and LazyProperty::callFunc only holds DeferTermination, not DeferGC. If an eden collection lands in that window, Process is old by the time the new function is stored into it unbarriered, and the next eden GC clears the Weak.
There was a problem hiding this comment.
Done in d21bdcc — vm.writeBarrier(this, onWarning) right after the add, mirroring JSEventEmitter::addListener.
| return function onWarning(warning) { | ||
| // Node only seeds this from --no-warnings; also honoring a runtime assignment lets | ||
| // test/js/node/test/common apply a test's `--no-warnings` flag without re-spawning. | ||
| if (process.noProcessWarnings) return; |
There was a problem hiding this comment.
I'd drop this. Node's onWarning never reads noProcessWarnings — it's a read-only alias seeded from the flag, and the only gate is whether the listener got registered at bootstrap. This per-emit check exists solely so test/js/node/test/common/index.js can fake --no-warnings in-process, but this PR is exactly what makes the real idiom work there: common loads before any test listener exists, so it can do
process.noProcessWarnings = true;
process.removeAllListeners('warning');which is byte-for-byte the state --no-warnings produces in Node. Then the printer matches Node exactly and doesn't carry a branch (and a justifying comment) for a test shim.
There was a problem hiding this comment.
Agreed, dropped in d21bdcc; the printer no longer reads it. The harness now removes the bootstrap listener instead (next thread), and noProcessWarnings = true alone prints again, same as node v26.3.0.
| // process.noProcessWarnings alias read at that point, so set it here. | ||
| // printer re-reads Node's process.noProcessWarnings alias on every | ||
| // warning, so setting it here silences the print like the flag does. | ||
| process.noProcessWarnings = true; |
There was a problem hiding this comment.
(see the comment on createOnWarning) — add process.removeAllListeners('warning'); here and the runtime noProcessWarnings read in the printer can go away.
There was a problem hiding this comment.
Done in d21bdcc — alias plus removeAllListeners('warning'). Spot-checked 10 --no-warnings vendored tests (incl. the expectWarning ones) through this branch, all pass.
| auto* globalObject = defaultGlobalObject(lexicalGlobalObject); | ||
| auto& vm = JSC::getVM(globalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| auto* process = globalObject->processObject(); |
There was a problem hiding this comment.
minor: this re-resolves the target via defaultGlobalObject(lexicalGlobalObject)->processObject() rather than the Process the stub was registered on. Equivalent in practice since the function is created per-global, but jsDynamicCast<Process*>(callFrame->thisValue()) (the emitter always invokes with m_thisObject, and you setThisObject(this) at install) would be the direct route and also does the right thing if someone plucks it out of listeners('warning') and .call()s it on a worker's process.
There was a problem hiding this comment.
Done in d21bdcc — dynamicDowncast(thisValue), printer built against that process's global; falls back to the realm's process only when called bare (f.call({}, err) still prints).
…ia this, drop the runtime noProcessWarnings read Review follow-ups: the listener map holds the stub weakly and is marked through the Process object, so the store needs the same barrier the JS addListener path issues; the stub now prints for the Process it was invoked on (falling back to the realm's process when called bare); and the printer no longer reads process.noProcessWarnings per warning, since Node's does not. The vendored test harness gets the state --no-warnings produces by removing the bootstrap listener instead.
darwin-aarch64 / darwin-x64 still build (they cross-compile on the Linux hosts) and still ship in canary; only the three macOS test entries and the darwin trace-order step are dropped, since those are the only steps that need a native macOS agent and they otherwise sit in the queue until the build times out. No-Verification-Needed: CI pipeline config only
…7364) ### What does this PR do? #37344 commented out the three darwin `testPlatforms` entries and the darwin `trace-order` target while the mac fleet was offline. #37354 (merged after it, but branched before it) routes darwin tests to Buildkite-hosted agents instead; with the entries gone there is nothing left to route, so **main currently runs no darwin tests at all** (see build 91730: 0 darwin test steps). This puts the entries back. #37354's filter still drops the two lanes hosted agents can't run (aarch64 `previous`, x64) and gates the rest to `main` / `[macos tests]`, so the effective result is one aarch64 macOS 26 lane on `test-darwin-hosted`, which build 91720 already showed passing in ~13 min. Reverting #37354 when the fleet is back brings all three lanes back with no further edit here. ### How did you verify your code works? `node .buildkite/ci.mjs --dry-run` on top of current main: - `main`: `darwin-aarch64-26-test-bun` + `darwin-aarch64-trace-order`, both `queue: test-darwin-hosted` - PR, plain subject: no darwin test steps - PR with `[macos tests]`: the single darwin test step
2 participants
What does this PR do?
Registers the default
'warning'printer whenprocessis created, the way Node's bootstrap does (lib/internal/process/pre_execution.jssetupWarningHandler), instead of on the firstemitWarning().#31831 made the printer a real
'warning'listener but installed it lazily, which its description called out as a divergence:process.removeAllListeners('warning')at startup did not silence it. That idiom is how programs (CLIs rendering a TUI, test runners) opt out of the default print, and it silenced warnings on every Bun release before #31831 (a user listener suppressed the native print), so this is a regression for them:(node:PID) Warning: hello+(Use \bun --trace-warnings ...`)`The fix is
Process::installDefaultWarningListener, called fromProcess::finishCreation. To keep the printer's setup (--redirect-warnings,--disable-warning,require("node:fs")) off the startup path, what gets registered up front is a nativeonWarningstub;Process::ensureOnWarningbuilds the JS printer (createOnWarning, the formerinstallOnWarningListenerminus theprependListenercall) on the first warning and the stub forwards to it. Startup cost is one nativeJSFunctionand one listener-map entry perprocessobject; the listener is added beforeonDidChangeListeneris wired so it does not trigger the signal-table loading that hook does on every add.Observable results, all matching Node (every script in the tests, plus the extra probes listed under verification, was run under node v26.3.0 and v24.18.0 and prints byte-identical output modulo pid/argv0):
process.listenerCount('warning')is1at startup,process.listeners('warning')[0].name === 'onWarning', and removing it by reference or viaremoveAllListenerssilences the print.--no-warnings/NODE_NO_WARNINGS=1register nothing (listenerCount === 0); user listeners still fire.worker_threadsWorker gets its own listener.process.emit('warning', err)with no prioremitWarning()now prints, as in Node (the other divergence process: port Node.js v26.3.0 process compatibility tests and fix the gaps they surface (env exotic-object/TZ semantics, warnings pipeline + CLI flags, uncaught origin/exit codes, execve throw, threadCpuUsage/finalization/loadEnvFile, native-module identity; +26 tests) #31831's description listed).The printer no longer reads
process.noProcessWarningsat all (Node's doesn't; it is only a read-only alias of the flag).test/js/node/test/common, which used that read to apply a test's--no-warningsflag in-process, now removes the bootstrap listener instead — it loads before any test listener exists, so that is the same state the flag produces.How did you verify your code works?
New
describe("default 'warning' listener is registered at startup")block intest/js/node/process/process.test.js(8 cases). On the unfixed build theremoveAllListeners-before-first-warning cases print the warning,listenerCountis0, the worker case reports0:1, and the bare-emitcase prints nothing; all pass withbun bd test. Node's own suite has no test for the startup listener (which is how #31831 missed it), so these are hand-written; the existing warning-pipeline tests (test-process-warning*.js,test-process-warnings.mjs,test-process-emitwarning.js,test-env-var-no-warnings.js,test-process-redirect-warnings*.js,test-common-expect-warning.js, the max-listeners and timers warning tests) still pass.Beyond the test scripts, these were also diffed against node v26.3.0 and v24.18.0 on the built binary and came out identical: no-arg
removeAllListeners(), the--trace-warningshint printed once across two warnings, callingprocess.listeners('warning')[0]directly with an Error and with a non-Error,--trace-warningsstacks,--redirect-warnings/NODE_REDIRECT_WARNINGS, and--disable-warning. The new native path was also exercised underBUN_JSC_validateExceptionChecks=1.