Worker: implement static Worker.data (Bun's workerData alias) - #35572
Worker: implement static Worker.data (Bun's workerData alias)#35572robobun wants to merge 4 commits into
Conversation
The TypeScript declaration for Worker.data ('the cloned value of the
data property passed to new Worker()') has existed since #4052 but
the runtime never exposed it; reading Worker.data returned undefined.
The data option itself was already being read and routed to
node:worker_threads' workerData. This adds the missing surface: a
DontEnum custom-value getter on the Worker constructor that returns
the same deserialized value workerData sees, triggering the
deserialization once if node:worker_threads hasn't been loaded yet.
createNodeWorkerThreadsBinding now caches workerData on the global
and reads it back on re-entry so both access orders resolve to the
same JS object.
Also declares the data option on Bun.WorkerOptions and documents it
in runtime/workers.mdx.
Fixes #9330
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds Worker data options and documentation, stores worker data in the global object, synchronizes Node worker data during bootstrap, exposes a read-only ChangesWorker.data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:27 AM PT - Jul 25th, 2026
❌ @robobun, your commit e5343d7 has 1 failures in
The baseline build contains instructions not available on Static scan violations
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Inside a node:worker_threads Worker, workerData is wrapped on the parent (stdio/messaging ports ride along) and unwrapped JS-side in worker_threads.ts. createNodeWorkerThreadsBinding was caching the wrapper, so Worker.data leaked the internal MessagePorts instead of the user's value. Expose a _setWorkerData binding and call it from worker_threads.ts after unpacking/unwrapping so m_nodeWorkerData stays identical to the exported workerData. The getter is now CustomAccessor (not CustomValue) so that write-back is observable. Also declare workerData/transferList on Bun.WorkerOptions since the native option parser already reads them and the new data JSDoc references transferList. Tests now assert object identity and cover the node:worker_threads Worker path.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
5f64571 addresses both earlier findings — _setWorkerData writes the unwrapped/unpackJSTransferables-processed value back into m_nodeWorkerData, and the new "is the unwrapped workerData inside a node:worker_threads Worker" test covers the wt.Worker path with an object-identity + Object.keys assertion (no @@bun… leak). Tests 1 and 2 now assert same: true on object payloads. Deferring to a human for the final sign-off given this adds new user-facing API surface on the Worker constructor and a new WriteBarrier on ZigGlobalObject; the comment-cop flags are also still open.
Extended reasoning...
Overview
Follow-up commit 5f64571d on top of f32c5825 implements static Worker.data (Bun's alias for workerData). Touches: ZigGlobalObject.{h,cpp} (new m_nodeWorkerData WriteBarrier<Unknown> in the FOR_EACH_GLOBALOBJECT_GC_MEMBER list + accessor), Worker.cpp (createNodeWorkerThreadsBinding now caches/re-reads the deserialized value and exports a setWorkerData host function at index 11), JSWorker.cpp (a DontEnum|ReadOnly|CustomAccessor data getter on the constructor that lazy-runs the binding), src/js/node/worker_threads.ts (destructures index 11 and calls _setWorkerData(workerData) when the module-local value diverges from the raw deserialized one), plus docs, bun.d.ts, and four new spawned tests.
Prior findings — resolved
My earlier 🔴 (wt.Worker leaks the internal @@bunWorkerThreadsMessaging/@@bunWorkerThreadsStdio transport wrapper via Worker.data) is fixed by option (b) from that comment: jsFunctionSetWorkerData lets the JS module push the final unwrapped value back into the native cache after workerData = workerData.data. Traced the guard if (workerData !== _workerData): unpackJSTransferables mutates plain objects in place (returns the same reference), so for a global-Worker payload with no JSTransferable markers the native cache already holds the right object and no write-back is needed; for wt.Worker the .data unwrap reassigns and triggers the write-back; if the top-level value is itself a marker, unpackJSTransferables returns a new instance and the write-back also fires. The 🟡 test-strength nit is fixed: test 1 now asserts same: before === workerData && Worker.data === workerData on an object, test 2 uses { v: 1 }, and the new third test asserts same: true and keys: ["greeting"] inside a require('node:worker_threads').Worker.
Security risks
None identified. _setWorkerData is only reachable via the $cpp(...) builtin intrinsic (not user-exposed). Worker.data reads per-VM state on ZigGlobalObject, so no cross-worker leakage.
Level of scrutiny
Moderate–high: new user-facing API on a global constructor, a new GC-visited field on ZigGlobalObject, and a re-entrancy contract between the Worker.data getter and the node:worker_threads binding factory. The WriteBarrier<Unknown> is declared inside FOR_EACH_GLOBALOBJECT_GC_MEMBER, so it is visited automatically. On re-entry, nodeWorkerData() returns jsNull()/jsUndefined() (both non-empty JSValues), so the if (JSValue cached = ...) guard correctly preserves them.
Other factors
The comment-cop action has five open flags on multi-line comments added in this diff. Those need to be addressed or dismissed before merge. Given the API-surface addition and the open bot comments, a human should sign off.
There was a problem hiding this comment.
Both earlier findings are addressed — the _setWorkerData binding now syncs the unwrapped/unpackJSTransferables-processed value back into m_nodeWorkerData, the new wt.Worker test covers that path, and tests 1/2 now assert object identity with object payloads. I didn't find anything further, but this adds user-facing API surface (Worker.data, WorkerOptions.data/workerData/transferList) and a new re-entrant init path through createNodeWorkerThreadsBinding, so it's worth a maintainer sign-off.
What was reviewed:
- Re-entry ordering (Worker.data-first vs require-first) and the moved-from
workerDataAndEnvironmentData/dataMessagePortson the second call — both guarded. - GC rooting:
m_nodeWorkerDatais aWriteBarrier<Unknown>insideFOR_EACH_GLOBALOBJECT_GC_MEMBER, so it's visited. if (JSValue cached = ...)with a cachedjsUndefined()—JSValue::operator boolis non-empty-check, so the no-data-worker case reuses the cache correctly.unpackJSTransferablesmutates containers in place, soworkerData === _workerDataholds for the global-Worker path and_setWorkerDataisn't spuriously called.
Extended reasoning...
Overview
Implements the long-declared-but-never-wired Worker.data static getter (Bun's alias for workerData). Adds m_nodeWorkerData (WriteBarrier<Unknown>) on ZigGlobalObject, a CustomAccessor getter on the Worker constructor that forces createNodeWorkerThreadsBinding if needed, a re-entry cache inside that binding, and a setWorkerData host function (index 11 of the binding array) so src/js/node/worker_threads.ts can push the unwrapped/unpackJSTransferables-processed value back into the native cache. Also adds data/workerData/transferList to Bun.WorkerOptions, a docs section, and four spawned-subprocess tests.
Findings addressed since the last review
- 🔴 (wt.Worker leaked the transport wrapper via
Worker.data): fixed by_setWorkerData+if (workerData !== _workerData) _setWorkerData(workerData)after the unwrap block, plus a dedicated test ("is the unwrapped workerData inside a node:worker_threads Worker") that assertssame: trueandkeys: ['greeting']. - 🟡 (identity not asserted on object payloads): tests 1 and 2 now both use object payloads and assert
same: Worker.data === workerData(test 1 also checksbefore === workerDataacross the re-entry boundary). - comment-cop's multi-line-comment flags were trimmed in dc5bbf3.
Security risks
None identified. The wrapper-leak concern (internal control/stdio MessagePorts surfacing via Worker.data) from the first review is closed by the _setWorkerData sync — the new wt.Worker test asserts Object.keys(Worker.data) contains only the user's key.
Level of scrutiny
Medium-high. This is a small feature but it threads through JSC bindings (custom accessor on a DOM constructor, a new WriteBarrier on the global object), makes createNodeWorkerThreadsBinding re-entrant, and adds public API/type surface. REVIEW.md calls out API-design changes and "no new fields on ZigGlobalObject" as things maintainers care about — this one is a WriteBarrier alongside the existing m_nodeWorkerEnvironmentData, which seems in-pattern, but that's a maintainer call.
Other factors
- Four new
test.concurrentsubprocess tests cover both access orderings, the wt.Worker wrapper path, and the null/undefined defaults; all wireonerror/errorto a rejecting exit and drain stdout/stderr concurrently. - The
entanglePorts(WTF::move(options.dataMessagePorts))on the second binding call operates on an already-moved (empty) vector — harmless no-op. if (JSValue cached = globalObject->nodeWorkerData())correctly treats a cachedjsUndefined()as present (JSValue's bool is emptiness, not JS-truthiness), so the no-data case doesn't fall through tojsNull().
Deferring to a maintainer for the API-surface/ZigGlobalObject-field sign-off rather than approving.
There was a problem hiding this comment.
The two issues from my earlier review are addressed: _setWorkerData (binding index 11) now writes the unwrapped/unpackJSTransferables-processed value back into m_nodeWorkerData, and a new test covers the node:worker_threads Worker path (asserting Worker.data === wt.workerData and keys: ["greeting"] — no transport-wrapper leak). Test 1 now asserts object identity via same: before === workerData && Worker.data === workerData, and test 2 uses an object payload.
I didn't find further bugs, but this adds user-facing API surface (a DontEnum custom getter on the Worker constructor), a new WriteBarrier<Unknown> on ZigGlobalObject, and a JS→native write-back path — worth a maintainer glance at the shape before landing.
What was reviewed
- Re-entry ordering:
Worker.data-first callscreateNodeWorkerThreadsBindingdirectly, then the module's$cppcall re-enters — thenodeWorkerData()/nodeWorkerEnvironmentData()cache reads make the second call idempotent (the moved-fromserializedis guarded). if (workerData !== _workerData)gate: fires exactly when the wt.Worker unwrap ran or_workerDatawas a top-level JSTransferable marker; nested markers mutate in place so the cached container reference stays coherent without a write-back.- GC:
m_nodeWorkerDatais inFOR_EACH_GLOBALOBJECT_GC_MEMBERalongsidem_nodeWorkerEnvironmentData, so it's visited;setNodeWorkerDatauses.set(vm(), this, data). - Empty/undefined/null: an unset
WriteBarrier<Unknown>decodes to an emptyJSValue(falsy in theif (JSValue cached = …)check), whilejsNull()/jsUndefined()are truthy JSValues, so the cache-hit test distinguishes "never set" from "set to null/undefined".
Extended reasoning...
Overview
Implements the long-declared-but-never-implemented Worker.data static getter (Bun's alias for require('node:worker_threads').workerData, per #4052 / #9330). Touches: ZigGlobalObject.{h,cpp} (new m_nodeWorkerData WriteBarrier + accessor pair), Worker.cpp (createNodeWorkerThreadsBinding now caches/reuses workerData on the global and exports a setWorkerData host function at binding index 11), JSWorker.cpp (jsWorkerConstructor_data custom getter installed on the constructor in initializeProperties), src/js/node/worker_threads.ts (writes the unwrapped/unpackJSTransferables-processed value back via _setWorkerData), plus type declarations (data/workerData/transferList on Bun.WorkerOptions), docs, and four spawned-subprocess tests in worker.test.ts.
Follow-up on prior review
My earlier 🔴 finding (wt.Worker leaks the internal { '@@bunWorkerThreadsMessaging', '@@bunWorkerThreadsStdio', data } transport wrapper via Worker.data) is fixed by the _setWorkerData write-back after the JS-side unwrap, and covered by the new third test which asserts identity with wt.workerData and Object.keys(Worker.data) == ['greeting']. My 🟡 finding (identity not asserted on an object payload) is fixed: test 1 now sends same: before === workerData && Worker.data === workerData and asserts same: true, and test 2's payload is now { v: 1 }. The comment-cop bot's multi-line-comment complaints were trimmed in dc5bbf3. All prior inline threads are marked resolved.
Security risks
None. The value cached in m_nodeWorkerData is the same object already exposed as workerData; the write-back setWorkerData host function is only reachable through the internal $cpp binding array (not on any user-visible object). The wrapper leak from the earlier revision — which exposed the internal stdio/messaging MessagePorts — is closed.
Level of scrutiny
Medium. The API surface itself was already committed to in the type declarations, so there's no new design to bikeshed, but the implementation adds a WriteBarrier field to ZigGlobalObject, a custom getter on a global constructor, and re-entrancy handling in createNodeWorkerThreadsBinding. I traced the four ordering combinations (main/worker × Worker.data-first / require-first) plus the wt.Worker preload path and they all resolve to the same object; the empty-vs-null/undefined WriteBarrier semantics check out; the field is visited via FOR_EACH_GLOBALOBJECT_GC_MEMBER.
Other factors
Test coverage is good: four test.concurrent spawned subprocesses cover both access orders, the wt.Worker unwrap path, and the no-data null/undefined defaults, all asserting reference identity inside the worker body before structured-clone erases it. The PR description reports the full worker.test.ts (29 pass) and worker_threads.test.ts (91 pass) suites green. Deferring rather than approving because a new constructor-level custom accessor + ZigGlobalObject field is the kind of thing a maintainer should sign off on even when the mechanics look right.
|
Diff is green on every lane that ran the worker tests (no
All review threads resolved. Ready for a maintainer to look at the API surface (new |
2 participants
Fixes #9330.
Repro
Cause
The TypeScript declaration for
Worker.data("the cloned value of thedataproperty passed tonew Worker()", Bun's equivalent ofworkerData) has existed since #4052 but the runtime never exposed adataproperty on theWorkerconstructor. Thedataoption was already being read inJSWorkerDOMConstructor::constructand routed torequire("node:worker_threads").workerData; only theWorker.datasurface was missing.Fix
ZigGlobalObjectgains am_nodeWorkerDatawrite barrier alongsidem_nodeWorkerEnvironmentData.createNodeWorkerThreadsBindingnow caches the deserializedworkerDataon the global and reads it back on re-entry, so both access orders (Worker.datafirst vsrequire("node:worker_threads")first) resolve to the same JS object. AsetWorkerDatahost function (binding index 11) letssrc/js/node/worker_threads.tspush the unwrapped/unpackJSTransferables-processed value back into the cache after it strips the internal stdio/messaging transport wrapper, soWorker.datanever exposes those ports.JSWorkerDOMConstructor::initializePropertiesinstalls aDontEnumCustomAccessorgetter fordatathat triggers the one-time deserialization ifnode:worker_threadshas not been loaded yet and then returns the cached value.Bun.WorkerOptionsinpackages/bun-types/bun.d.tsnow declaresdata,workerData, andtransferList(all three are already read by the native option parser), anddocs/runtime/workers.mdxgains a shortWorker.datasection.Why this is correct
Worker.datais a Bun-specific alias forworkerData, so it must be the same value (including object identity) thatnode:worker_threadssees. Routing throughcreateNodeWorkerThreadsBindingand caching the result on the global guarantees that without deserializing twice or changingworkerData's behaviour. The write-back fromworker_threads.tskeeps the cache aligned with the JS-side unwrapping that node-kind workers perform. On the main threadworkerDataisnull, and inside a worker with nodataoption it isundefined;Worker.datamirrors both, matching Node'sworkerDatasemantics.Verification