diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index b0c04abf..792cddb8 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -616,7 +616,6 @@ class WorkerWrapper : public BaseDataWrapper { const WrapperType Type(); const int Id(); - const inline bool isDisposed() { return isDisposed_; } const bool IsRunning(); const bool IsClosing(); const int WorkerId(); @@ -624,17 +623,38 @@ class WorkerWrapper : public BaseDataWrapper { // The only route from the worker thread to the parent: see mainLoop_. std::weak_ptr MainLoop() const { return mainLoop_; } const inline v8::Isolate* GetWorkerIsolate() { return workerIsolate_; } - const inline void MakeWeak() { isWeak_ = true; } - const inline bool IsWeak() { return isWeak_; } + + // Deletion is decided by who still holds the wrapper. The parent's Worker + // object holds it from construction; Start() adds the worker thread. Each + // side lets go exactly once, with a compare-and-swap out of Holders::Both, + // so exactly one of them finds itself the last holder and deletes. + // + // Parent's thread, during the final disposal of the parent isolate. Returns + // true when the worker thread is already done with the wrapper (or never + // had it), which leaves the delete to the caller; false when the worker + // thread still uses it and deletes it when it ends. + bool ReleaseFromParent(); + // Parent's thread, from the Worker object's finalizer. True when the worker + // thread is done with the wrapper (or never had it), so the caller may + // delete; nothing changes when false. + bool HeldByParentOnly() const; + // Worker thread, as its last touch of the wrapper and after it removed the + // wrapper's Caches::Workers entry. Deletes the wrapper when the parent has + // already let go; otherwise the parent may delete it from here on. + void ReleaseFromWorkerThread(); private: + enum class Holders : uint8_t { Parent, Both, WorkerThread }; + v8::Isolate* mainIsolate_; v8::Isolate* workerIsolate_; std::atomic isRunning_; std::atomic isClosing_; std::atomic isTerminating_; + // The worker thread has started tearing down. Worker thread only; who + // deletes the wrapper is holders_, not this. std::atomic isDisposed_; - std::atomic isWeak_; + std::atomic holders_; // False until the entry script has finished evaluating (EnableMessageQueue); // DrainPendingTasks leaves the queue untouched while disabled. std::atomic messagesEnabled_; @@ -674,8 +694,10 @@ class WorkerWrapper : public BaseDataWrapper { bool workerObjectRooted_ = false; // Cleared by the destructor, so a task posted from the worker thread can tell // whether this wrapper still exists once it reaches the main isolate. The - // wrapper is only ever destroyed with that isolate locked, which is what the - // task takes before reading this. + // parent deletes the wrapper with that isolate locked, which is what the task + // takes before reading this. The worker thread only deletes it after the + // parent's final disposal, which follows the shutdown of the loop such a + // task would run on. std::shared_ptr> selfRef_; void BackgroundLooper(std::function func); diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 5787fe66..013cf18f 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -303,20 +303,21 @@ void DisposeHandle(v8::Isolate* isolate, } case WrapperType::Worker: { WorkerWrapper* worker = static_cast(wrapper); - if (!worker->isDisposed()) { - // A running worker's Worker object is rooted (WorkerWrapper:: - // RootWorkerObject), so a weak callback should not reach a live worker - // at all. This refusal stays as the floor under that: re-arming keeps - // the wrapper alive for another cycle, which is safe, whereas freeing - // it while the thread still posts through it is not. Reaching it is not - // free either -- a re-armed handle that is also a weak-collection key - // can corrupt the collector's ephemeron bookkeeping -- so it is a - // fallback, not a mechanism to rely on. - // - // During final disposal, inform the worker it should delete itself. - if (isFinalDisposal) { - worker->MakeWeak(); - } + // Final disposal lets go of the wrapper for good, and only deletes it + // when the worker thread is done with it; otherwise that thread deletes + // it when it ends. + // + // A weak callback deletes the wrapper only when the worker thread is + // done with it, and changes nothing otherwise. A running worker's Worker + // object is rooted (WorkerWrapper::RootWorkerObject), so a weak callback + // should not reach a live worker at all. The refusal stays as the floor + // under that: re-arming keeps the wrapper alive for another cycle, which + // is safe, whereas freeing it while the thread still posts through it is + // not. Reaching it is not free either -- a re-armed handle that is also a + // weak-collection key can corrupt the collector's ephemeron bookkeeping + // -- so it is a fallback, not a mechanism to rely on. + bool lastHolder = isFinalDisposal ? worker->ReleaseFromParent() : worker->HeldByParentOnly(); + if (!lastHolder) { return false; } break; diff --git a/NativeScript/runtime/Runtime.mm b/NativeScript/runtime/Runtime.mm index d4325a9a..bcd37978 100644 --- a/NativeScript/runtime/Runtime.mm +++ b/NativeScript/runtime/Runtime.mm @@ -292,14 +292,9 @@ void DisposeIsolateWhenPossible(Isolate* isolate) { messaging::CloseAllPorts(isolate_); if (IsRuntimeWorker()) { - std::shared_ptr workerState = Caches::Workers->Get(this->workerId_); - WorkerWrapper* currentWorker = - workerState == nullptr ? nullptr : static_cast(workerState->UserData()); + // Only the registry entry: the wrapper outlives this runtime, and the + // worker thread lets go of it once this destructor has returned. Caches::Workers->Remove(this->workerId_); - // if the parent isolate is dead then deleting the wrapper is our responsibility - if (currentWorker != nullptr && currentWorker->IsWeak()) { - delete currentWorker; - } } Caches::Remove(this->isolate_); diff --git a/NativeScript/runtime/WorkerWrapper.mm b/NativeScript/runtime/WorkerWrapper.mm index 90ddcb57..02234c18 100644 --- a/NativeScript/runtime/WorkerWrapper.mm +++ b/NativeScript/runtime/WorkerWrapper.mm @@ -52,7 +52,7 @@ static void PostToLoop(const std::shared_ptr& loop, std::functionGetEventLoop()), @@ -84,6 +84,8 @@ static void PostToLoop(const std::shared_ptr& loop, std::functionisRunning_ = true; + // Also before queueing: the operation's last act is to let go of the wrapper. + this->holders_.store(Holders::Both, std::memory_order_release); NSBlockOperation* op = [NSBlockOperation blockOperationWithBlock:^{ this->BackgroundLooper(func); @@ -246,41 +248,62 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrDestroyInspector(); - // The callback closes over this wrapper, which ~Runtime may delete below, - // while V8 keeps the registration until the isolate is disposed - and - // disposal can be deferred past that point. + // The callback closes over this wrapper, which may be deleted at the end of + // this function, while V8 keeps the registration until the isolate is + // disposed - and disposal can be deferred past that point. if (this->heapLimitIsolate_ != nullptr) { v8::Locker locker(this->heapLimitIsolate_); this->heapLimitIsolate_->RemoveNearHeapLimitCallback(WorkerWrapper::OnNearHeapLimit, 0); this->heapLimitIsolate_ = nullptr; } - // Everything needed below is read first: publishing isDisposed_ is the last - // permitted touch of `this`. From that store on, a parent that is tearing - // down may delete this wrapper concurrently, and ~Runtime deletes it on this - // thread when the parent already handed ownership over. + // Read before ReleaseFromWorkerThread below, after which `this` may be gone. Isolate* mainIsolate = this->mainIsolate_; std::weak_ptr mainLoop = this->mainLoop_; std::shared_ptr> selfRef = this->selfRef_; - int workerId = this->workerId_; this->isDisposed_ = true; Runtime* runtime = Runtime::GetCurrentRuntime(); if (runtime != nullptr) { + // Removes this worker's Caches::Workers entry. delete runtime; } else { - // Runtime was never created (worker terminated before initialization). - // The runtime destructor normally handles this cleanup, so do it here. - bool found; - auto state = Caches::Workers->Get(workerId, found); - if (found) { - Caches::Workers->Remove(workerId); - } + // Runtime was never created (worker terminated before initialization), so + // the entry its destructor removes is removed here. + Caches::Workers->Remove(this->workerId_); } + // The registry entry is gone, so no other runtime's teardown can reach the + // wrapper through it any more, and nothing on this thread needs it again. + this->ReleaseFromWorkerThread(); + PostThreadEndedNotification(mainIsolate, mainLoop, selfRef); } +bool WorkerWrapper::ReleaseFromParent() { + Holders expected = Holders::Both; + if (this->holders_.compare_exchange_strong(expected, Holders::WorkerThread, + std::memory_order_acq_rel)) { + return false; + } + // A failed exchange leaves the value found in `expected`. Anything but + // Parent means the parent let go earlier and the worker thread still has it. + return expected == Holders::Parent; +} + +bool WorkerWrapper::HeldByParentOnly() const { + return this->holders_.load(std::memory_order_acquire) == Holders::Parent; +} + +void WorkerWrapper::ReleaseFromWorkerThread() { + Holders expected = Holders::Both; + if (!this->holders_.compare_exchange_strong(expected, Holders::Parent, + std::memory_order_acq_rel) && + expected == Holders::WorkerThread) { + delete this; + } +} + void WorkerWrapper::EnableMessageQueue() { this->messagesEnabled_.store(true, std::memory_order_release); this->queue_.Signal(); diff --git a/TestRunner/app/tests/WorkerLifetimeTests.js b/TestRunner/app/tests/WorkerLifetimeTests.js index f1f73d80..8bc41115 100644 --- a/TestRunner/app/tests/WorkerLifetimeTests.js +++ b/TestRunner/app/tests/WorkerLifetimeTests.js @@ -256,3 +256,85 @@ describe("Worker teardown with a transferred port in flight", function () { }; }); }); + +// A worker that ends with a child still running terminates the child and then +// disposes the child's Worker object, while the child's own thread is tearing +// down: both threads are at the end of the child's native wrapper at once. +describe("Worker teardown with a running child worker", function () { + const ROUNDS = 24; + let originalTimeout; + // The worker of the round in progress, and whether the spec is over: a + // spec that failed or timed out must not leave a worker running or start + // another round from a late event. + let active = null; + let finished = false; + beforeEach(function () { + originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; + jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000; + finished = false; + }); + afterEach(function () { + jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout; + finished = true; + if (active !== null) { + active.terminate(); + active = null; + } + }); + + it("ends the parent whether it is terminated or closes itself", function (done) { + function finish() { + if (!finished) { + finished = true; + done(); + } + } + (function round(index) { + if (index === ROUNDS) { + finish(); + return; + } + let childUp = false; + const worker = new Worker("./workerLifetimeNestedParent.js"); + active = worker; + worker.onerror = function (event) { + if (finished) { + return true; + } + expect("worker error: " + event.message).toBeNull(); + finish(); + return true; + }; + worker.onmessage = function (event) { + if (finished) { + return; + } + // Anything else is the fixture reporting that its child failed. + expect(event.data).toBe("child up"); + if (event.data !== "child up") { + finish(); + return; + } + childUp = true; + if (index % 2 === 0) { + worker.terminate(); + } else { + worker.postMessage("close"); + } + }; + worker.addEventListener("nsworkerended", function () { + if (finished) { + return; + } + active = null; + // An end before the child was up did not exercise the teardown. + expect(childUp).toBe(true); + if (!childUp) { + finish(); + return; + } + round(index + 1); + }); + })(0); + }); +}); diff --git a/TestRunner/app/tests/workerLifetimeNestedParent.js b/TestRunner/app/tests/workerLifetimeNestedParent.js new file mode 100644 index 00000000..cdc759e5 --- /dev/null +++ b/TestRunner/app/tests/workerLifetimeNestedParent.js @@ -0,0 +1,17 @@ +// Reports once its child worker is running, so the test can end this worker +// while the child is alive; "close" ends it from the inside instead. +var child = new Worker("./eventLoopEchoWorker.js"); +child.onmessage = function () { + postMessage("child up"); +}; +child.onerror = function (event) { + postMessage("child error: " + event.message); + return true; +}; +child.postMessage("ping"); + +onmessage = function (event) { + if (event.data === "close") { + close(); + } +};