-
Notifications
You must be signed in to change notification settings - Fork 4.6k
process: implement process._rawDebug #30937
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
Open
robobun
wants to merge
3
commits into
main
Choose a base branch
from
farm/50751022/fix-rawdebug
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Minor Node-compat divergence: Node's
_rawDebugis backed by nativeFPrintF(stderr, ...), which silently no-ops when fd 2 is closed or the pipe is broken — butfs.writeSync(2, ...)will throwEBADF/EPIPEin those cases. Since this is meant as a last-resort logger for shutdown/crash paths where stderr may already be in a degraded state, consider wrapping thewriteSyncintry { ... } catch {}so a failed debug write can't mask the original error or change exit behavior.Extended reasoning...
What the bug is
Node implements
process._rawDebuginlib/internal/process/per_thread.jsas a thin wrapper around the native binding_rawDebug(RawDebug()insrc/node_process_methods.cc), which callsFPrintF(stderr, ...)followed byfflush(stderr). C stdio writes to a closed or brokenFILE*set the stream's error indicator and return a negative value, but never propagate a JavaScript exception — the call simply does nothing visible from JS.Bun's new implementation at
src/js/builtins/ProcessObjectInternals.ts:471-473usesfs.writeSync(2, ...):fs.writeSyncthrows on any underlyingwrite(2)error. So if fd 2 has been closed (EBADF) or stderr is a pipe whose reader has exited (EPIPE, sinceSIGPIPEis ignored),_rawDebugwill throw where Node would silently continue.Code path that triggers it
fs.closeSync(2), or the process was launched asbun script.js 2>&1 | headandheadexits after reading enough lines.pino/thread-stream, per the PR description) callsprocess._rawDebug('diagnostic message').writeSync(2, ...)invokeswrite(2), which returns-1witherrno = EBADForEPIPE.writeSyncconverts that into a thrown JS error, which propagates out of_rawDebug.Why nothing prevents it
There is no
try/catcharound thewriteSynccall, and theconstructRawDebugC++ wrapper inBunProcess.cpponly catches exceptions thrown while constructing the closure (the one-timegetRawDebug()call), not exceptions thrown when the returned_rawDebugfunction is later invoked.Step-by-step proof
FPrintF(stderr, "x\n")writes to a closed fd,fprintfreturns an error indicator, nothing is thrown into JS.'survived'is printed to stdout. Exit code 0.writeSync(2, "x\n")→write(2, ...) = -1, errno=EBADF→ throwsError: EBADF: bad file descriptor, write. The uncaught exception sets exit code 1 and'survived'is never printed.The
EPIPEvariant is more realistic in practice:bun -e "for(let i=0;i<1e5;i++) process._rawDebug(i)" 2>&1 | head -1— onceheadexits, the nextwriteSync(2, ...)throwsEPIPEin Bun but is silently dropped in Node.Impact
The PR's own comment describes
_rawDebugas something that "works even while the event loop is blocked or during shutdown" — i.e., it's a last-resort logger meant to be called from already-failing code paths. If it throws, it can:EBADF/EPIPEbecomes the visible failure instead of whatever was being logged).beforeExit/exitnow exits non-zero).uncaughtExceptionhandlers in surprising ways.That said, this is a rare edge case: most programs never close fd 2, and stderr's reader dying mid-process is uncommon. The previous Bun behavior was a no-op stub, so this PR is still a strict improvement. And
_rawDebugis an undocumented API.Fix
One-line change — swallow write errors to match Node's C-stdio semantics: