Replace the RuntimeHooks fn-pointer table with extern "Rust" functions - #33215
Replace the RuntimeHooks fn-pointer table with extern "Rust" functions#33215robobun wants to merge 5 commits into
Conversation
WalkthroughThis PR renames internal runtime hook dispatch functions from a ChangesRuntimeHooks to bun_runtime_* Rename
Sequence Diagram(s)sequenceDiagram
participant VirtualMachine
participant jsc_hooks
VirtualMachine->>jsc_hooks: bun_runtime_init_runtime_state(vm, opts)
jsc_hooks-->>VirtualMachine: Result<RuntimeState, Error>
VirtualMachine->>jsc_hooks: bun_runtime_ensure_debugger(vm, block)
VirtualMachine->>jsc_hooks: bun_runtime_auto_tick(vm)
VirtualMachine->>jsc_hooks: bun_runtime_print_exception(vm, value, list)
VirtualMachine->>jsc_hooks: bun_runtime_process_exit(global, code)
VirtualMachine->>jsc_hooks: bun_runtime_cancel_all_timers(vm)
VirtualMachine->>jsc_hooks: bun_runtime_terminate_all_workers_and_wait(timeout)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:29 AM PT - Jul 9th, 2026
❌ @robobun, your commit 2c58b2b has 4 failures in
🧪 To try this PR locally: bunx bun-pr 33215That installs a local version of the PR into your bun-33215 --bun |
There was a problem hiding this comment.
I didn't find any bugs — the fn-pointer→extern conversion looks mechanically correct and the removed fallbacks were provably dead — but this touches core VM/event-loop/worker/timer dispatch across 17 files and sets architectural direction (aligning with #33152), so it's worth a human look.
Extended reasoning...
Overview
This PR replaces the 28-slot RuntimeHooks fn-pointer struct with individual extern "Rust" functions resolved at link time. The change spans 17 files: VirtualMachine.rs converts the struct to an unsafe extern "Rust" block, jsc_hooks.rs renames each body to bun_runtime_* with #[no_mangle] and deletes the static table, and ~13 call-site files swap (hooks.foo)(...) for direct bun_runtime_foo(...) calls while removing the always-taken if let Some(hooks) = runtime_hooks() wrappers. Two new worker --no-addons tests are added.
Security risks
None identified. This is a dispatch-mechanism refactor with no changes to validation, auth, crypto, or trust boundaries. The safe/unsafe qualifiers on each extern fn match the original fn-pointer types.
Level of scrutiny
High. While the transformation is mechanical and I verified the extern signatures match the old fn-pointer types (including safe/unsafe qualifiers and lifetimes), this touches the core runtime dispatch surface: VM init/destroy, event loop auto-tick, worker lifecycle/shutdown, timer heap insert/remove, IPC, debugger, exception printing, and console formatting. The removed fallback code paths (auto_tick/auto_tick_active/run_error_handler when runtime_hooks() returned None) were provably dead since runtime_hooks() wrapped a link-time static and always returned Some, but their removal still changes the shape of critical paths.
Other factors
- This is explicitly the
RuntimeHooksslice of a larger refactor (#33152), so it carries architectural direction that a maintainer should ratify. - The PR description shows thorough verification (cargo check/clippy across 4 crates,
bun bdlink, and targeted test runs), and adds coverage for the previously-untested workerexecArgv --no-addonspath. - No CODEOWNERS or prior human review comments on the timeline yet.
There was a problem hiding this comment.
I didn't find any bugs — the fn-pointer-table → extern-fn conversion looks mechanically correct and the deleted fallbacks were provably dead — but this touches core VM init/teardown, the event loop, worker lifecycle, and error handling across 17 files, and it's setting the bun_runtime_* naming that #33152 converges on, so it's worth a human look.
Extended reasoning...
Overview
This PR replaces the RuntimeHooks fn-pointer struct (28 slots, defined as a #[no_mangle] static in bun_runtime and declared extern "Rust" in bun_jsc) with individual extern "Rust" functions, each #[no_mangle]-defined in bun_runtime::jsc_hooks under a bun_runtime_* prefix. Call sites change from (hooks.foo)(...) / runtime_hooks().unwrap() to direct bun_runtime_foo(...) calls. Since runtime_hooks() wrapped a link-time static and always returned Some, the if let Some(hooks) wrappers and their dead else-branches (in auto_tick, auto_tick_active, run_error_handler, etc.) are deleted. The bulk of the diff is comment updates renaming RuntimeHooks::foo → bun_runtime_foo. Two new tests cover the previously-untested worker execArgv: ["--no-addons"] path.
Security risks
None. This is a dispatch-mechanism refactor with no change to input handling, validation, permissions, crypto, or trust boundaries. The safe/unsafe split in the new unsafe extern "Rust" block preserves the safety coloring of each original fn-pointer slot.
Level of scrutiny
High. While the transformation is mechanical and I traced each of the 28 slots (declaration signature ↔ #[no_mangle] definition ↔ call sites) without finding a mismatch, the touched code paths are the runtime's spine: VirtualMachine::init/destroy/global_exit, auto_tick/auto_tick_active, run_error_handler, worker start_vm/shutdown, IPC message/close handling, hot reload, and the console formatter dispatch. The deleted fallback branches were dead (link-time-resolved static ⇒ runtime_hooks() never returned None), but their removal in run_error_handler deletes ~25 lines of degraded-print logic that a reviewer should confirm was truly unreachable.
Other factors
The PR is explicitly a scoped-down piece of #33152 and establishes the bun_runtime_* naming both PRs share, so a maintainer should confirm the direction and naming convention before it lands. The author reports cargo check/clippy clean, bun bd links, and the relevant test suites pass (with a handful of pre-existing failures reproduced on main). No prior human review exists on this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/no-addons.test.ts`:
- Around line 34-48: The two worker-spawning cases in no-addons.test.ts are
independent, so switch the existing test definitions to test.concurrent. Update
both dlopenInWorker-related tests to use test.concurrent while keeping the same
assertions and helper usage, and ensure no shared state is introduced between
them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 386327c9-ea6a-485b-a2f0-ff732dec5eda
📒 Files selected for processing (17)
src/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/js_parser_jsc/Macro.rssrc/jsc/AbortSignal.rssrc/jsc/ConsoleObject.rssrc/jsc/Debugger.rssrc/jsc/VirtualMachine.rssrc/jsc/event_loop.rssrc/jsc/web_worker.rssrc/resolver/resolver.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/cli/run_command.rssrc/runtime/jsc_hooks.rssrc/runtime/test_runner/pretty_format.rssrc/runtime/timer/mod.rssrc/runtime/valkey_jsc/js_valkey.rstest/js/node/no-addons-worker-fixture.jstest/js/node/no-addons.test.ts
There was a problem hiding this comment.
No issues found, but this reworks the cross-crate dispatch for VM init/teardown, event-loop ticking, error handling, and worker lifecycle (and deletes the previously-unreachable no-hooks fallbacks), so it's worth a human pass to confirm nothing was relying on those branches.
Extended reasoning...
Overview
This PR replaces the RuntimeHooks fn-pointer table (28 slots, declared in bun_jsc::virtual_machine and defined as a #[no_mangle] static in bun_runtime::jsc_hooks) with individual extern "Rust" functions, each #[no_mangle]-defined in jsc_hooks.rs and declared in an unsafe extern "Rust" block in VirtualMachine.rs. Call sites across 15 source files change from (hooks.foo)(...) (behind runtime_hooks() / if let Some(hooks)) to direct bun_runtime_foo(...) calls. It also deletes the "no hooks installed" fallback bodies in run_error_handler, auto_tick, and auto_tick_active, and adds two worker --no-addons tests plus a fixture.
Security risks
None identified. No auth, crypto, permission, or untrusted-input parsing paths are touched; the change is a dispatch-mechanism refactor with no new data flow.
Level of scrutiny
Medium-high. The mechanical shape of each hunk is straightforward (rename + unwrap), and it follows an established in-tree pattern (__bun_tick_queue_with_count, __bun_stdio_blob_store_new). However, the touched surface is core runtime infrastructure — VM init/destroy, global_exit, auto_tick/auto_tick_active, run_error_handler, worker start_vm/shutdown, IPC close, timer insert/remove, and process.exit. The removed fallbacks were justified as dead (runtime_hooks() always returned Some because it wrapped a link-time static), which looks correct from reading the deleted code, but that's exactly the kind of "provably dead" claim a maintainer should confirm — especially since the old comments explicitly called out "pure bun_jsc unit tests" as the reason for the Option shape.
Other factors
- The PR is explicitly a scoped-down piece of a larger refactor (#33152) using the same
bun_runtime_*names for convergence, so a maintainer may want to weigh sequencing/naming against that PR. - The one CodeRabbit nit (use
test.concurrent) has been addressed and resolved. - The lifetime-parameter change on
console_print_runtime_object(dropping the explicitfor<'a, 'f>HRTB in favor of elided lifetimes on a free fn) looks equivalent but is a subtle signature change. - Verification in the description is thorough (cargo check/clippy across the affected crates,
bun bdlink, targeted test runs), and the bug-hunting system found nothing.
e2a15ef to
1d1d3c0
Compare
There was a problem hiding this comment.
No issues found and the stale doc references from my earlier comment are addressed, but this rewires the VM↔runtime dispatch boundary across 22 files (VM init/teardown, worker lifecycle, error handling, process exit) so it's worth a human sign-off.
Extended reasoning...
Overview
This PR replaces the RuntimeHooks fn-pointer table (a 28-slot struct + __BUN_RUNTIME_HOOKS static + runtime_hooks() accessor) with direct extern "Rust" fn declarations, each defined #[no_mangle] in bun_runtime::jsc_hooks. Call sites go from (hooks.foo)(args) to bun_runtime_foo(args). Since runtime_hooks() always returned Some(&__BUN_RUNTIME_HOOKS) (a link-time static), the if let Some(hooks) wrappers and their dead fallback branches (run_error_handler's degraded print, auto_tick/auto_tick_active's tick() fallback) are deleted along with them. Also adds worker execArgv: ["--no-addons"] test coverage that was missing.
Files touched span the whole JSC↔runtime boundary: VirtualMachine.rs, web_worker.rs, jsc_hooks.rs, event_loop.rs, Debugger.rs, ConsoleObject.rs, plus comment-only updates in ~10 other files.
Security risks
None identified. This is a dispatch-mechanism refactor with no change to inputs, validation, or trust boundaries. The SSL_CTX cache, IPC, and process-exit hooks are renamed but their bodies and call ordering are unchanged.
Level of scrutiny
High. While each individual change is mechanical (rename + dead-branch removal), the affected code paths are load-bearing: VM init/teardown, worker shutdown ordering, uncaught-exception → process.exit, timer-heap insert/remove, cron teardown, and the standalone-sourcemap load. The removed if let Some(hooks) branches were provably dead (the accessor was Some(&static)), and the PR description's disassembly shows release LTO was already devirtualizing these to direct calls — so the argument for "no behavior change" is convincing. But a mistake here (a missed #[no_mangle], a dropped call, a signature mismatch that only trips at link time on one platform) would break VM startup or worker teardown, so a maintainer should confirm the sweep is complete. The PR also notes it converges with #33152's naming, which is an architectural coordination point a human should be aware of.
Other factors
- The bug-hunting system found nothing this run.
- My earlier inline comment (stale
init_runtime_state/deinit_runtime_statedoc references) was addressed in 681da74, along with four more sites the author found in a follow-up grep. - CodeRabbit's
test.concurrentnitpick was addressed. - CI build #70724 is in progress per the robobun comment; I did not verify its final state.
- The added worker
--no-addonstest looks correct: it wiresonerrorto reject, awaits the actual message, and terminates infinally.
bun_jsc declared a 28-slot struct of fn pointers plus an extern static (__BUN_RUNTIME_HOOKS) that bun_runtime defined; every call loaded a fn pointer out of the static. The table has exactly one implementation and is already resolved at link time, so declare each hook as its own extern "Rust" fn (bun_runtime_*) and define it #[no_mangle] in bun_runtime::jsc_hooks, matching the existing __bun_* link-time externs. Delete RuntimeHooks, __BUN_RUNTIME_HOOKS, runtime_hooks(), and the dead "no hooks installed" fallbacks its Option return implied (the static was always present, so those branches never ran).
The worker execArgv parsing path (bun_runtime_parse_worker_exec_argv_allow_addons) had no coverage: test/js/node/no-addons.test.ts only covers the CLI flag on the main process. Assert that a worker created with execArgv: ["--no-addons"] rejects process.dlopen with the addons-disabled error while a worker without the flag can still call dlopen.
test/js/web/workers/worker.test.ts has two pre-existing debug-build timing failures (the 1s "doesn't close event loop" guards), so host the new coverage in test/js/node/no-addons.test.ts instead, which is where the --no-addons behavior is already tested.
The rename left a few comments pointing at the old short names
(init_runtime_state / deinit_runtime_state) in files the sweep didn't touch:
bundler/transpiler.rs, dns_jsc/dns.rs, socket/{Listener,socket_body}.rs,
api/bun/SecureContext.rs. Update those, and collapse the jsc_hooks.rs:312
comment that named the same function under both the old and new names.
681da74 to
2c58b2b
Compare
What
bun_jsc::virtual_machine::RuntimeHookswas a 28-slot struct of fn pointers.bun_jscdeclared it plus anextern "Rust"static (__BUN_RUNTIME_HOOKS),bun_runtime::jsc_hooksdefined the static, and every call site went throughruntime_hooks()and a fn-pointer load.The table has exactly one implementation and is already resolved at link time, so the struct adds nothing. This deletes
RuntimeHooks,__BUN_RUNTIME_HOOKS, andruntime_hooks(), and declares each hook as its ownextern "Rust"fn (bun_runtime_*prefix), defined#[no_mangle]inbun_runtime::jsc_hooks. This is the same pattern the codebase already uses for__bun_tick_queue_with_count,__bun_stdio_blob_store_new, and the other cross-tier externs.Call sites go from
(hooks.auto_tick)(vm)tobun_runtime_auto_tick(vm). Becauseruntime_hooks()always returnedSome(it wrapped a link-time static), theif let Some(hooks)wrappers and the dead "no hooks installed" fallbacks inauto_tick,auto_tick_active, andrun_error_handlerare deleted with it.No behavior change. This is the
RuntimeHookspiece of what #33152 does inside a much larger refactor, scoped to just the dispatch mechanism (no type or field changes), and it uses the samebun_runtime_*names so the two converge.Verification
cargo checkandcargo clippyare clean onbun_jsc,bun_runtime,bun_http_jsc, andbun_js_parser_jsc;cargo fmt --checkpasses.bun bdlinks, which is what actually resolves theextern "Rust"symbols.bun build --compile --sourcemapsmoke test (standalone sourcemap + runtime-flag hooks). The only failures (two 1s-timeout worker tests,setInterval doesn't leak memory, and twoinspect-errorsnapshot tests) reproduce identically on an unmodified debug build of main.execArgv: ["--no-addons"]path (bun_runtime_parse_worker_exec_argv_allow_addons) had no coverage (test/js/node/no-addons.test.tsonly covered the CLI flag on the main process), so this adds two cases there: a worker spawned with the flag rejectsprocess.dlopenwith the addons-disabled error, and a worker without the flag can still call it. The other converted paths are already exercised by the existing timer, worker, console, preload, IPC, and standalone suites; the conversion itself changes no behavior, so there is no before/after-distinguishing test to write.Codegen
Release builds (fat LTO,
codegen-units = 1) of main and of this branch, disassembling the same caller,EventLoop::wait_for_promise, which dispatchesauto_tickin its wait loop:Before (main):
After (this branch):
Release codegen was already a direct call before this change: fat LTO devirtualized the fn-pointer table and
__BUN_RUNTIME_HOOKSdoes not exist in the shipped binary at all (it appears in neithernmnor the linker map of the main build). The new form produces the same direct call by construction, without relying on LTO, so debug and non-LTO builds stop loading and indirect-calling through the table too. In the new release binary, 22 of the 28 hooks remain as standalone direct-call targets and the other 6 (timer_remove,ssl_ctx_cache_get_or_create,create_node_fs,ipc_child_singleton_deinit,console_on_before_print,apply_standalone_runtime_flags) were inlined into their callers entirely. No function pointers remain on this path in any build mode.Rebase
Rebased onto main at b05b4fa. Conflicts resolved:
exit_on_uncaught_exceptionblock inside the!handledbranch ofVirtualMachine::uncaught_exception; kept main's new structure and converted its new(hooks.process_exit)(global_object.as_ptr(), 1)call tobun_runtime_process_exit(global_object.as_ptr(), 1). ThebeforeExit/exit/uncaughtExceptiontests added by that PR still pass on the rebased build.Listener.rsSSL-ctx-cache accessor from returning a raw pointer to a closure-passingwith_ssl_ctx_cache; took main's new body and re-applied the comment'sinit_runtime_state→bun_runtime_init_runtime_staterename.CI status
Build 70844: every lane green except debian-13 x64-asan, which has two unrelated failures:
test/js/bun/http/proxy-stress-concurrent.test.ts: LeakSanitizer reports a 9-byte leak (aBox<[u8]>DNS name) allocated atsrc/runtime/dns_jsc/dns.rs:4697inResolver::get_or_put_into_pending_cache, far from anything this PR touches; this PR's only change to that file is two doc-comment lines.test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js: SIGABRT with the JSC validator assertion!scope.exception() || !hasSlotinJSValue::get. Did not reproduce in 10 back-to-back local runs withBUN_JSC_validateExceptionChecks=1on this branch's debug build at2c58b2b6a.Main's build 70779 at the same base commit (
b05b4fab0e) passes both. Not pushing a retrigger, since a CI re-roll was already used earlier in this branch's history.