Repository navigation
Run the promise jobs of 'exit' listeners and report their rejections when the event loop runs dry #46758
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
base: main
Are you sure you want to change the base?
Run the promise jobs of 'exit' listeners and report their rejections when the event loop runs dry #46758
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -352,6 +352,9 @@ pub struct VirtualMachine { | |
| pub on_print_error_zig_exception: Option<fn(*mut c_void, &ZigException)>, | ||
| pub on_print_error_zig_exception_ctx: *mut c_void, | ||
| pub(crate) is_handling_uncaught_exception: bool, | ||
| /// Advances on each error `uncaught_exception` takes, handled or not. `on_exit` compares it | ||
| /// around the 'exit' listeners: an error they threw ends the event. | ||
| uncaught_exception_epoch: u32, | ||
| pub(crate) exit_on_uncaught_exception: bool, | ||
| /// An earlier `--hot` run ended with an unhandled error. Its `beforeExit` listeners are still registered. | ||
| hot_run_failed: bool, | ||
|
|
@@ -500,6 +503,8 @@ unsafe extern "C" { | |
|
|
||
| safe fn Process__dispatchOnBeforeExit(global: &JSGlobalObject, code: u8); | ||
| safe fn Process__dispatchOnExit(global: &JSGlobalObject, code: u8); | ||
| safe fn Bun__Process__hasExitCode(global: &JSGlobalObject) -> bool; | ||
| safe fn Bun__Process__discardNextTicks(global: &JSGlobalObject); | ||
| safe fn Bun__closeAllSQLiteDatabasesForTermination(global: &JSGlobalObject); | ||
| safe fn Bun__closeAllNodeSqliteDatabasesForTermination(global: &JSGlobalObject); | ||
| safe fn Bun__WebView__closeAllForTermination(); | ||
|
|
@@ -776,6 +781,8 @@ pub struct ExitHandler { | |
| /// `process.exit()`, a fatal error or the end of a `bun test` run, as opposed to the event loop running dry. | ||
| /// See `VirtualMachine::exit_tears_down_napi_envs`. | ||
| pub requested: bool, | ||
| /// `on_before_exit()` returned with the loop dry and no unhandled error. | ||
| pub loop_ran_dry: bool, | ||
| } | ||
|
|
||
| impl ExitHandler { | ||
|
|
@@ -823,14 +830,22 @@ impl ExitHandler { | |
| /// parent via `container_of` would escape the provenance of `&mut self` | ||
| /// (which only covers the `ExitHandler` field). Callers pass the VM | ||
| /// reference instead; the body re-enters JS so no `&mut` is held. | ||
| pub(crate) fn dispatch_on_exit(vm: &VirtualMachine) { | ||
| let exit_code = vm.exit_handler.exit_code; | ||
| /// | ||
| /// Returns whether the event was emitted and nothing stopped its listeners. | ||
| pub(crate) fn dispatch_on_exit(vm: &VirtualMachine) -> bool { | ||
| // `process.on('exit')` handlers are user script (see `on_exit`). | ||
| if vm.script_allowed() && !vm.exit_handler.skip_exit_listeners { | ||
| Process__dispatchOnExit(vm.global(), exit_code); | ||
| if !vm.script_allowed() || vm.exit_handler.skip_exit_listeners { | ||
| return false; | ||
| } | ||
| if vm.worker.is_none() { | ||
| Bun__WebView__closeAllForTermination(); | ||
| let exit_code = vm.exit_handler.exit_code; | ||
| let global = vm.global(); | ||
| match jsc::from_js_host_call_generic(global, || Process__dispatchOnExit(global, exit_code)) | ||
| { | ||
| Ok(()) => true, | ||
| Err(err) => { | ||
| let _ = crate::task::report_error_or_terminate(global, err); | ||
| false | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -2145,6 +2160,7 @@ impl VirtualMachine { | |
| if self.is_shutting_down() || !self.script_allowed() || err.is_termination_exception() { | ||
| return true; | ||
| } | ||
| self.uncaught_exception_epoch = self.uncaught_exception_epoch.wrapping_add(1); | ||
|
|
||
| // An exception thrown by a Bun.ModuleGraph's module code is that graph's to | ||
| // handle, ahead of the test runner and the thread-wide path. (A rejection | ||
|
|
@@ -2227,6 +2243,7 @@ impl VirtualMachine { | |
| } | ||
|
|
||
| pub fn on_before_exit(&mut self) { | ||
| self.exit_handler.loop_ran_dry = false; | ||
| // Node only emits 'beforeExit' on a natural drain; a fatal uncaught | ||
| // exception that brought us here is an implicit process.exit(1). Arm | ||
| // the hard-exit flag ourselves since we're skipping the dispatch that | ||
|
|
@@ -2269,6 +2286,7 @@ impl VirtualMachine { | |
|
|
||
| break; | ||
| } | ||
| self.exit_handler.loop_ran_dry = self.unhandled_error_counter == 0; | ||
| } | ||
|
|
||
| /// Writes each configured profile once. The sampling report is written first because | ||
|
|
@@ -2327,7 +2345,22 @@ impl VirtualMachine { | |
| // Write profiles before any shutdown begins. | ||
| self.write_profiles(); | ||
|
|
||
| ExitHandler::dispatch_on_exit(self); | ||
| // The loop ran dry: node emits this 'exit' through MakeCallback, which then runs the | ||
| // listeners' promise jobs and reports what they rejected, unless a listener threw. | ||
| // `process.exit()` and a fatal error emit it with a plain call. Not in a worker: | ||
| // `WebWorker::shutdown` has unpublished its VM, so `terminate()` could not stop a job. | ||
| let ran_dry = | ||
| self.is_main_thread() && self.exit_handler.loop_ran_dry && !self.exit_handler.requested; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Under Why this was flaggedTrigger: Verification: Trigger:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. One observation for the test. Under |
||
| let uncaught_exception_epoch = self.uncaught_exception_epoch; | ||
| { | ||
| let _entered = ran_dry.then(|| self.enter_event_loop_scope_without_checkpoint()); | ||
| if ExitHandler::dispatch_on_exit(self) && ran_dry { | ||
| self.run_exit_listeners_jobs(uncaught_exception_epoch); | ||
| } | ||
| } | ||
| if self.is_main_thread() { | ||
| Bun__WebView__closeAllForTermination(); | ||
| } | ||
|
|
||
| // process.exit() never reaches drain_microtasks; flush AutoFlusher sinks here. | ||
| if !self.is_inside_deferred_task_queue.get() { | ||
|
|
@@ -2360,6 +2393,34 @@ impl VirtualMachine { | |
| } | ||
| } | ||
|
|
||
| /// What node's `InternalCallbackScope::Close` (src/api/callback.cc) runs after the 'exit' | ||
| /// listeners of a loop that ran dry, unless one threw: the microtask checkpoint, then the | ||
| /// rejection pass. `&self` and never inlined: the listeners and the jobs change the fields | ||
| /// it compares through other references. | ||
| #[cold] | ||
| #[inline(never)] | ||
| fn run_exit_listeners_jobs(&self, uncaught_exception_epoch: u32) { | ||
| if self.uncaught_exception_epoch != uncaught_exception_epoch { | ||
|
robobun marked this conversation as resolved.
Resolved
|
||
| return; | ||
| } | ||
| let global = self.global(); | ||
| Bun__Process__discardNextTicks(global); | ||
| if self.event_loop_mut().drain_microtasks().is_err() { | ||
| return; | ||
| } | ||
| let unhandled_error_counter = self.unhandled_error_counter; | ||
|
Comment on lines
+2408
to
+2411
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Under Why this was flaggedA node:test file run as Verification: nit — triggered when a node:test file run under
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. The helper will take its mark of It is not pushed yet. The PR is a draft until it can be rebased on #45575, #45597 and #45582, and this change comes with that push. |
||
| let _ = global.handle_rejected_promises(); | ||
|
robobun marked this conversation as resolved.
Resolved
|
||
| if self.unhandled_error_counter != unhandled_error_counter { | ||
| // A fatal error in node: it exits with the assigned `process.exitCode` or with 1, | ||
| // and frees no environment. | ||
| let exit_handler = &mut self.as_mut().exit_handler; | ||
| exit_handler.requested = true; | ||
| if !Bun__Process__hasExitCode(global) { | ||
| exit_handler.exit_code = 1; | ||
| } | ||
|
Comment on lines
+2416
to
+2420
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Under Why this was flaggedA CommonJS or ESM script run with Verification: Then
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed. The store that replaces the code is the one in So this PR does not change
Comment on lines
+2413
to
+2420
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Users who assign process.exitCode and whose 'exit' listener leaves a job that throws now exit 1 instead of the assigned code (base: assigned code, silent; Node: assigned code, error printed). The "assigned process.exitCode, or 1" rule at src/jsc/VirtualMachine.rs:2418 only covers the rejection pass. A throwing microtask in drain_microtasks (2408), or a rejection under --unhandled-rejections=throw or strict, goes through uncaught_exception, which calls process_exit(global_object, 1) at 2219 and overwrites exit_code with 1 at src/runtime/node/node_process.rs:71. Fix: once 'exit' is being emitted, every fatal error reported after the listeners must keep the assigned exitCode, e.g. have the fatal path at 2219 pass exit_handler.exit_code when Bun__Process__hasExitCode is true. Why this was flaggedTrigger: Verification: src/jsc/VirtualMachine.rs:2408
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed. The exit code is 1 where Node exits 2, for a job that throws and for a rejection under I built the fix at the fatal arm of #42032 carries this rule for all three stores, and it needs #45575 first. The rule is not part of this PR anymore. Until it lands, a job that throws exits 1, as a listener that throws does on main. The description lists the row. |
||
| } | ||
| } | ||
|
|
||
| /// Whether `on_exit()` runs `RareData::cleanup_hooks`, i.e. `NapiEnv::cleanup` for | ||
| /// each addon. Node does this when it frees an environment: a worker's on any exit, | ||
| /// the main thread's only once its loop ran dry (`process.exit()` and a fatal error | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.