Repository navigation
fix(worker): decide who deletes a WorkerWrapper with one atomic state - #479
Conversation
The worker thread and the parent's teardown each checked one flag and set another, in opposite orders, so a parent that tore down while a child worker was ending could delete the wrapper under the child's ~Runtime, or leave it to nobody. One Holders state replaces both flags: each side lets go with a compare-and-swap, and whoever finds itself last deletes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesWorker lifetime ownership
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Parent
participant ObjectManager
participant WorkerWrapper
participant BackgroundLooper
participant Runtime
Parent->>ObjectManager: Dispose worker wrapper
ObjectManager->>WorkerWrapper: ReleaseFromParent
BackgroundLooper->>Runtime: Tear down worker runtime
BackgroundLooper->>WorkerWrapper: ReleaseFromWorkerThread
WorkerWrapper->>Parent: Post worker-ended notification
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change reworks how the native worker wrapper's memory is released between the parent thread and the worker's background thread, replacing a simpler weak-reference flag with a coordinated ownership handoff, and adds a stress test that repeatedly terminates and closes a worker with a running child. Verification of the core ownership handoff, the object finalizer re-registration, and the shared worker registry's thread-safety found no double-free, leak, or race introduced by this change, so it appears safe to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (3 skipped: 3 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit guards the holder state, Comment |
* ci: run the test suite under AddressSanitizer The Test job becomes a two-entry matrix: the existing run, still named Test, and Test (ASan), the same xcodebuild invocation with -enableAddressSanitizer YES. Use-after-free and double-free bugs in the runtime are usually silent in a plain run, so a spec written for one (the nested-worker teardown spec from #479) passes there with or without its fix. Artifact names are unique per workflow run, so the ASan entry uploads test-results-asan and test-diagnostics-asan, and collect-test-diagnostics takes the artifact name as an input. Five worker quality-of-service specs cannot hold in an instrumented build: a worker asked for default or utility reads back user-initiated. They are reported as skipped there, keyed on a new TNSIsAddressSanitizerEnabled() test fixture, and run unchanged everywhere else. * test: let the system assign the test server's port The XCTest harness served the junit report and the HTTP module fixtures on a fixed loopback port. Simulators share the host's loopback interface, so two suite runs on one machine, even on different simulators, could deliver one run's report to the other's listener. The listener now binds a port chosen by the system and hands it to the app through REPORT_BASEURL, which is already the only way the app learns the address. start() blocks until the listener is bound, since the port is only known then, and throws when binding fails instead of leaving the failure unobserved. * test: log the test server's port The port is assigned by the system at bind time, so the run's output is the only place to find it when poking at the server by hand. * test: report test server startup errors as test failures Setup moves to setUpWithError() so a listener that fails to bind, or does not become ready in time, is recorded by XCTest as a failure of the test instead of crashing the runner through try!. tearDown() tolerates a server that was never created, since it still runs after a throwing setup.
2 participants
A worker that ends while a child worker of its own is still running (terminated by its parent, or calling
close()) reads freed memory on the child's thread. With AddressSanitizer on the TestRunner the new spec aborts onmain:The same applies to an embedder that shuts the main runtime down while workers run. Without a sanitizer it is usually silent; when the freed byte happens to read non-zero it becomes a double free.
The parent's teardown terminates its children and, a few lines later, disposes their Worker objects, so both threads reach the end of a child's
WorkerWrappertogether. Who deletes it was decided by two flags checked in opposite orders: the worker publishedisDisposed_and later readisWeak_, the parent readisDisposed_and then either deleted or setisWeak_. That allows:isDisposed_, while the child's~Runtimestill reaches the wrapper through itsCaches::Workersentry (any other runtime's~Runtimewalking the registry can hit the stale entry too);This replaces both flags with one atomic
Holders { Parent, Both, WorkerThread }. It starts asParent, andStart()makes itBoth. The worker thread lets go as its last touch of the wrapper, after its registry entry is removed (both branches): CASBoth -> Parent, or delete if it findsWorkerThread. The parent's final disposal does CASBoth -> WorkerThread, or deletes if it findsParent. The finalizer only deletes inParent. Exactly one CAS out ofBothsucceeds, so exactly one side ends up last. Registry readers stay safe becauseForEachholds the map mutex across its callback and the worker'sRemovehas to get that mutex before the wrapper can become deletable by anyone. A wrapper that never reachedStart()staysParent, so its finalizer can free it, which the old flags never allowed.isDisposed_remains only as the worker thread's own "teardown has begun" flag.The new spec in
WorkerLifetimeTests.jsruns 24 parent/child rounds, alternatingterminate()andclose(). It needs ASan to fail onmain; with the change the full suite is clean under ASan apart from the thread QoS specs, which read a different QoS class back when the sanitizer interposes thread creation.Summary by CodeRabbit
Bug Fixes
Tests