node: fix the rest of test-process - #16026
Merged
Merged
Conversation
Collaborator
|
Updated 9:12 PM PT - Jan 3rd, 2025
❌ @nektro, your commit 83ce207 has 2 failures in
🧪 try this PR locally: bunx bun-pr 16026 |
Jarred-Sumner
requested changes
Jan 4, 2025
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
Collaborator
There was a problem hiding this comment.
We shouldn't make error: print as Error:
- There are very likely 3rd-party scripts depending on this behavior
- It's aesthetically worse
We can make whatever tests that specifically check for the string "Error:" instead check for "error:", or we can make sure it's using util.inspect isntead of Bun.inspect / console.log in the appropriate places.
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 27, 2026
…ocess as this (#44057) ### Problem - `process.exit()` calls `process.reallyExit(code)`, as Node.js does. When `process.reallyExit` is not callable, Bun throws a `TypeError` with an empty message. Node.js throws `TypeError: process.reallyExit is not a function`. - A build with assertions aborts: `ASSERTION FAILED: !message.isEmpty()` at `vendor/WebKit/Source/JavaScriptCore/runtime/Error.cpp(71)` in `JSC::createTypeError`. - The cause is `JSC::call(globalObject, reallyExitVal, args, ""_s)` at `src/jsc/bindings/BunProcess.cpp:913`. The last argument is the message for a value that is not callable. This overload also passes the function as its own `this`. Node.js passes `process`. ### Fix - The call passes `process` as `this` and the message `process.reallyExit is not a function`. - Correct because Node.js runs `process.reallyExit(process.exitCode || 0)` in JavaScript. That is a method call on `process`, and V8 gives that message. - Verified: two new tests in `test/js/node/process/process.test.js`. Both fail on release 1.4.3-canary.1. Also ran the other `process.exit()` tests and 12 exit tests from `test/js/node/test/parallel/`. ### Background - `process.reallyExit` is the raw exit of Node.js: it emits no `'exit'` event. Libraries such as signal-exit replace it. - `JSC::call` is the JavaScriptCore helper that calls a JS function from C++. - No other design applies: the two wrong values are arguments of one call. No other `JSC::call` in `src/` has an empty message. ### Downsides - A function that replaces `process.reallyExit` now sees `this === process` when `process.exit()` calls it. Before, `this` was the function itself. - Size: +53 bytes in the `BunProcess.cpp` object (`-O3`, no LTO): 37 for the message, 16 of code. Per call: no new allocation, branch, or syscall. <details><summary>Notes</summary> Repro: ```js process.reallyExit = "str"; process.exit(0); ``` | | stderr | exit code | |---|---|---| | Bun 1.4.3-canary.1+367d939d9 (release) | `TypeError: ` | 1 | | Bun 1.4.3-debug+367d939d9 | `ASSERTION FAILED: !message.isEmpty()` | 134 | | this branch (debug) | `TypeError: process.reallyExit is not a function` | 1 | | Node.js v26.3.0 | `TypeError: process.reallyExit is not a function` | 0 | The same result for each value tried: `"str"`, `undefined`, `null`, `1`, `{}`, `Symbol("x")`, and `delete process.reallyExit`. The `this` value, with `process.reallyExit = function () { console.log(this === process) }` and `process.exit(3)`: `false` before, `true` on this branch and on Node.js v26.3.0. Not changed by this PR: when nothing catches the `TypeError`, Node.js exits with the code given to `process.exit()` (0 above, 5 for `process.exit(5)`). Bun exits 1. A replaced `process.reallyExit` that throws shows the same difference. For an entry point that throws, `uncaught_exception` (`src/jsc/VirtualMachine.rs:2161`) and `exit_with_unhandled_note` (`src/runtime/cli/run_command.rs:1638`) both set exit code 1. Related to #42032, which changes the first place for an `'exit'` listener that throws. The new test catches the error in the child, so it does not depend on that exit code. History: #16026 added the call with the comment `// process.reallyExit(exitCode);`. Search for other call sites: `JSC::call`, `call`, `profiledCall` and `construct` with an empty message literal in `src/` and `packages/`. `BunProcess.cpp:913` is the only one. Size measurement: compiled `BunProcess.cpp` from `origin/main` and from this branch with the release flags of the build (`-O3 -march=nehalem`), without `-flto=thin` so that the object is native code. `size -A`: `.text` of `Bun::Process_functionExit` 712 to 728 bytes, `.rodata.str1.1` 10594 to 10631 bytes, `.data` and `.bss` unchanged. On a debug build without the fix, the first new test fails: the child exits with code 134 and the assertion is in its stderr. Tests run on the debug build of this branch: - `test/js/node/process/process.test.js -t "process.exit\(\)"`: 6 pass. - `test/js/node/process/process.test.js`, whole file: 179 pass, 5 skip, 3 fail. `process` fails because the container sets no `USER`. `signal > simple case works` and `signal > process.emit will call signal events` time out at 5 s in the whole-file run, with and without the fix. Each passes in 2.5 s when it runs alone. - `test/js/node/test/parallel/`: `test-process-exit-code-validation.js`, `test-process-exit-code.js`, `test-process-exit-from-before-exit.js`, `test-process-exit-handler.js`, `test-process-exit-recursive.js`, `test-process-exit.js`, `test-process-really-exit.js`, `test-worker-exit-code.js`, `test-worker-nested-on-process-exit.js`, `test-worker-on-process-exit.js`, `test-worker-process-exit-async-module.js`, `test-crypto-op-during-process-exit.js`: all exit 0. Not run: macOS, Windows. The changed line has no platform-specific code. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/process/process.test.js <!-- robobun:evidence:end -->
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
supercedes #14621
fixes all the tests that don't depend on missing behavior in
node:child_processornode:v8a few missing for
process.envthat will get a separate followupnot done, pushing now so the robobun comment is at the top