| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
Replaces the finalizer-resurrection lifetime with reachability: the wrapper's persistent goes strong once the thread starts and is released only by the thread-exit notification, posted from the worker's teardown to the parent's event loop — terminate() initiates the wind-down but never drops the root early, so no GC can condemn a wrapper whose thread is still draining. ObjectManager's refuse-and-re-weaken branch stays as a defensive fallback but is unreachable for workers. The motivation is a reproduced heap corruption: the patched collector's kFinalizer resurrection handles ephemeron keys in the atomic pause but not under concurrent marking — a resurrected WeakMap key whose values are reachable only through the entry leaves a dangling value slot that crashes ConcurrentMarkingVisitor::RecordSlot on a later cycle. Strong lifetime takes Worker off that path entirely; the collector bug is tracked separately for the other resurrectable wrapper types. The thread-exit notification also dispatches the internal nsworkerended event on the Worker object, so node:worker_threads' Worker shim now emits 'exit' exactly once for self-close as well as terminate(). Suite: 1663/0 incl. new WorkerLifetimeTests (WeakMap-key repro that crashed before this change, collectability after terminate and self-close, delivery to an unreferenced live worker).
…ndence, not a live crash The wrapper-keyed-WeakMap corruption was a collector bug fixed in the v8-14.9.207.39-6 prebuilts; the rule stays because own-instance state is Node's design for handler attributes and keeps the builtins off the resurrection/ephemeron interplay the kFinalizer patch must re-cover on every V8 upgrade.
| Back | FazBrowse Home | New Git URL |
Stacked on #454 (feat/worker-threads). Merge that first.
What this fixes
Worker JS wrappers previously lived by finalizer resurrection: registered weak immediately, condemned by GC while the thread ran, then revived by ObjectManager::DisposeValue refusing disposal and re-arming the handle (sanctioned by our custom V8 kFinalizer patch). We reproduced real heap corruption from that pattern: the patch handles resurrected ephemeron keys in the atomic mark-compact pause, but not under concurrent marking — a resurrected WeakMap key whose values are reachable only through the entry leaves a dangling value slot, crashing ConcurrentMarkingVisitor::RecordSlot on a later cycle:
Reproducing required a task-posted GC (no conservative stack scan), values held only through the ephemeron entries, and a two-level chain — which is why it survived unnoticed: plain __collect() never hits it. Any app putting a Worker in a WeakMap could crash this way on current releases.
The change
Reachability-based lifetime, matching browsers and Node: the wrapper's persistent goes strong when the thread starts and is released only by a thread-exit notification posted from the worker's teardown to the parent's event loop. terminate() initiates wind-down but never drops the root early — the wrapper is strong for exactly the thread's lifetime, so the resurrection fallback is unreachable for workers (kept as a commented defensive branch). Teardown cascade verified: strong persistents flow through DisposeAllRegistered correctly.
Bonus from the same notification: an internal nsworkerended event on the Worker object lets the node:worker_threads shim emit 'exit' on self-close (previously only on terminate()), exactly once either way.
Tests
WorkerLifetimeTests.js (deliberately not in the shared suite — the repro would crash the Android runtime's CI until it gets the same treatment):
Suite: 1663 / 0.
Related