| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configuration
Reviewing files that changed from the base of the PR and between 42a8bcf and 59edaa4. 📒 Files selected for processing (15)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. 📝 Walkthrough WalkthroughThe runtime now retains worker objects until their threads finish and reports completion through an internal event. The Worker API uses that event to emit one zero-code exit event and resolve pending terminate() promises. New tests and documentation cover worker lifetime and exit behavior. ChangesWorker lifecycle
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant WorkerThread
participant WorkerWrapper
participant WorkerEvents
participant Worker
participant NodeWorkerThreads
WorkerThread->>WorkerWrapper: Post NotifyThreadEndedOnParent
WorkerWrapper->>WorkerEvents: EmitEnded(worker)
WorkerEvents->>Worker: Dispatch nsworkerended
Worker->>NodeWorkerThreads: Handle nsworkerended
NodeWorkerThreads->>NodeWorkerThreads: Emit exit(0) and resolve waiters
WorkerWrapper->>WorkerWrapper: Clear worker registry entry and persistent handle
Merge Risk: ⚪ Minimal · up to 59eda The change keeps Worker objects alive until their threads end and reports exit and terminate() results at that point. No merge-blocking issue was identified from the supplied review material. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to 59eda The lifecycle change improves Worker retention and completion ordering without an identified expansion of worker authority. One failure-containment issue remains: a throwing exit listener can leave earlier termination promises permanently pending, even though native cleanup proceeds. Retained concerns
Security Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
❌ Failed checks (1 warning)
Explanation Docstring coverage is 34.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 13 files. (2 skipped: 2 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. ❤️ ShareA rabbit watched the worker run, Comment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
…the end as nsworkerended terminate() reset the Worker object's persistent and dropped the registry entry the moment it was called, while the thread was still winding down: the wrapper stopped being reachable from native before it had finished, and anything the worker had already queued on the parent's loop was discarded on arrival. The root now survives terminate(); it is released by the worker thread's own last act, which posts the end back to the parent's event loop. That post no longer only clears. On the parent's thread it dispatches the internal `nsworkerended` event on the Worker object and only then releases the persistent and the registry entry, so the end of a worker is observable from JS for the first time. The node:worker_threads shim listens for it, which is what lets 'exit' be emitted exactly once for a worker's own close() as much as for terminate(), and lets terminate() resolve at that point rather than off a microtask — after every message and error the worker had already sent. A parent that is itself tearing down clears its children directly and never delivers the notification, matching iOS. Android needed neither half of the iOS change's lifetime rework: the wrapper is shared_ptr-owned by the registry and by the detached thread itself, its poWorker_ has been a strong Persistent since construction, and the Worker object is a plain FunctionTemplate instance ObjectManager never sees — so there was no finalizer resurrection to take it off, and worker-thread posts already reached the parent through a weak_ptr to its event loop rather than through its isolate. Mirrors NativeScript/ios#456.
| Back | FazBrowse Home | New Git URL |
Stacked on #2043 (feat/worker-threads) — merge that first.
What this fixes
terminate() reset the Worker object's Persistent and dropped the registry entry synchronously, while the worker thread was still winding down. From that moment the wrapper was unreachable from native, the end of the worker was unobservable from JS, and any message the worker had already queued on the parent's loop was discarded when it arrived.
Android vs iOS lifetime — what was verified before porting
The iOS change is largely about replacing finalizer resurrection with reachability. Android never had that problem, so most of it does not apply:
The change
Consumer audit for the removed clear. Nothing relied on the persistent being empty after terminate() for correctness: PostMessageToParent already returns early on isTerminating_; every error source is gated at the point of forwarding (CallWorkerScopeOnErrorHandle returns early on IsTerminating(), the unhandled-rejection path in NativeScriptException.cpp checks IsTerminating()/IsDisposed(), and BackgroundLooper's catch checks !isTerminating_), so no error from a terminated worker reaches the parent; the isTerminated private only guards a double terminate(); FireMessageOnParentWorkerObject/FireErrorOnParentWorkerObject keep their empty-persistent guards for the teardown paths that still clear early. The one observable change is the intended one: a message the worker queued before terminate() is now delivered instead of being dropped on arrival, and it is delivered before nsworkerended.
Tests
New Android-only specs in test-app/app/src/main/assets/app/tests/testWorkerLifetime.js (wired in mainpage.js), with workerLifetimeCloseWorker.js and messaging/deadlockChild.js / messaging/deadlockParent.js:
Full device suite on a Pixel_3a_API_36 arm64 emulator: 1398 specs, 0 failures, 4 skipped (baseline on feat/worker-threads was 1391/0/4; the 7 new specs all ran and passed). npm run lint clean.
Deviations from iOS (ab72efc2)
Mirrors NativeScript/ios#456.
Summary by CodeRabbit