Repository navigation
Conversation
Worker::create, WorkerMessagingProxy and WebWorker__create validated the options and spawned the thread in one step. Split each into a create step that validates and allocates, and a start step that spawns. The serialized workerData, its ports and the shared env store become arguments of start. The constructor calls create and start back to back, where it called create. No behaviour change.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reshapes worker construction across the C++/Rust FFI boundary (refcounts, keep-alive, child_workers registration, thread spawn) and ships no test for the new spawn-failure path, a human look would still be worthwhile.
What was reviewed:
- Spawn-failure path: thread ref and parent ticket drop with the closure,
startunrefs the poll ref, thenreleaseWorkerThread()unrefs again (KeepAlive::unref is a no-op when already inactive), removes the child_workers entry, joins nothing, and derefs the proxy's ref — balanced with the old null-return path. child_workers.pushmoved before spawn: create and start run back-to-back on the parent thread inside the constructor, so no exit path can observe the entry between them;release_parent_poll_refstill prunes it.- Options stored on
m_optionsbeforeWebWorker__start, so the worker thread's read in Worker.cpp still sees them before the thread exists; stopped-at-birth early return preserved. - JSWorker.cpp:
toJSNewlyCreated3-arg overload exists; manual ExceptionOr propagation matches the old throwScope overload; single caller ofWorker::create/startWorkerGlobalScope, no stale call sites.
Extended reasoning...
The change splits Worker construction into create (validate + allocate) and start (copy parent state, keep-alive, spawn) across six files in src/jsc/bindings/webcore and src/jsc/web_worker.rs, moving workerData/ports/sharedEnvStore from WorkerOptions into a new start() argument list and the child_workers registration from after spawn to before it. It touches no injection, auth, or data-exposure surface; the sensitive surface is cross-FFI refcount and thread-lifecycle correctness. Refcounts and the keep-alive were traced as balanced on success, spawn failure, and stopped-at-birth, and the OOM arm now routes through bun_core::handle_oom per repo convention. Deferring rather than approving because the diff rewires worker lifecycle plumbing that is hard to exercise in tests (the PR adds none), and the author describes a second PR stacking on it that depends on this ordering. No CODEOWNERS entry covers the changed files and there are no third-party review events.
|
Status
|
Behaviour change: none
#44942 stacks on this PR.
Problem
Worker::create,WorkerMessagingProxy::startWorkerGlobalScopeandWebWorker__create(src/jsc/web_worker.rs) each reject options and spawn the thread in one step.Fix
workerData, its ports and the shared env store become arguments of start.JSWorkerDOMConstructor::constructcalls create then start where it called create. No statement changes its order.Background
WorkerMessagingProxyis shared by the parent thread and the worker thread.WebWorkeris the Rust object that owns the OS thread.child_workers, so the existing exit path releases one that never started.Downsides
new Worker(). Instructions in these functions for a plain construct: 7,844 to 7,728. Allocator calls: 110 to 110. Release text: +768 bytes.size_of::<WebWorker>()(200 bytes before and after) and the test files above.Notes
What moved
WebWorker::createkeeps the preload resolution and theexecArgvflag check, allocates theWebWorkerand registers it as a child. It stores the two flags thatexecArgvsets (--no-addons,--no-ffi-cc) for start.WebWorker::startholds the rest of the old body unchanged: the copy of the transform options and of the env, the keep-alive, the parent ticket and the spawn. It returnsfalseif the OS refuses the thread.Out of memoryarm aroundLoader::clone_for_worker. That function returns noErr(ArrayHashMap::clonecannot fail), so the arm was unreachable. Start has one failure left, the OS refusing the thread.WorkerMessagingProxy::prepareWorkerGlobalScopeis the oldstartWorkerGlobalScopeup toWebWorker__create. The newstartWorkerGlobalScopestores the three arguments inm_options, where the worker thread reads them, and callsWebWorker__start.WebWorker::createare untouched, so Report the resolve error when a Worker preload does not resolve #41496 merges with no conflict.Checks that it behaves as main (release builds of main
e655c58032and of this commit, before the rebase onto71d0d439973, which touches none of these files)new.targetwith a throwingprototypegetterCost (gdb counters on the constructing thread.
pthread_createis stepped over.)new Worker(f){ data, transferList }{ preload: [p] }{ execArgv: [] }node:worker_threadsnew Worker(f)WebWorker__create(+WebWorker__start),new Worker(f)new Worker(f), constructs 4 and 5size_of::<WebWorker>(), release / debugTests
test/js/node/worker_threads,test/js/web/workersandtest/js/bun/module-graph, 2,811 pass, 0 fail.module-graph-workers.test.tsran with--max-concurrency=2: at the default some of its tests time out on my machine, for main too. Node'stest-worker-*.js: 105 of 106, as on main.cargo clippy -p bun_jsc,cargo fmt --checkandclang-formatare clean.prlimit --nprocasnobody, there is no portable test): the constructor throwsTypeError: Failed to spawn worker thread, the transferred port closes at once, no'worker'event fires, and the process exits 0. Main does the same. Worker: throw ERR_WORKER_INIT_FAILED when the OS refuses the thread #41675 adds a Linux test for this path.Open PRs that touch the same lines (
git merge-treeagainst this branch)no test proof · iteration 0 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check