diff --git a/NativeScript/runtime/ConcurrentQueue.cpp b/NativeScript/runtime/ConcurrentQueue.cpp index 24721114..ee55723a 100644 --- a/NativeScript/runtime/ConcurrentQueue.cpp +++ b/NativeScript/runtime/ConcurrentQueue.cpp @@ -54,14 +54,19 @@ void ConcurrentQueue::SignalAndWakeUp() { void ConcurrentQueue::Terminate() { std::unique_lock lock(initializationMutex_); terminated = true; - if (this->runLoop_) { - CFRunLoopStop(this->runLoop_); + CFRunLoopRef runLoop = this->runLoop_; + CFRunLoopSourceRef source = this->runLoopTasksSource_; + this->runLoopTasksSource_ = nullptr; + this->runLoop_ = nullptr; + + if (runLoop) { + CFRunLoopStop(runLoop); } - if (this->runLoopTasksSource_) { - CFRunLoopRemoveSource(this->runLoop_, this->runLoopTasksSource_, kCFRunLoopCommonModes); - CFRunLoopSourceInvalidate(this->runLoopTasksSource_); - CFRelease(this->runLoopTasksSource_); + if (source) { + CFRunLoopRemoveSource(runLoop, source, kCFRunLoopCommonModes); + CFRunLoopSourceInvalidate(source); + CFRelease(source); } } diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index ceb47b17..c0e7c585 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -690,11 +690,11 @@ class WorkerWrapper: public BaseDataWrapper { private: v8::Isolate* mainIsolate_; v8::Isolate* workerIsolate_; - bool isRunning_; - bool isClosing_; + std::atomic isRunning_; + std::atomic isClosing_; std::atomic isTerminating_; - bool isDisposed_; - bool isWeak_; + std::atomic isDisposed_; + std::atomic isWeak_; std::function thiz, std::shared_ptr)> onMessage_; std::shared_ptr> poWorker_; ConcurrentQueue queue_; diff --git a/NativeScript/runtime/Runtime.mm b/NativeScript/runtime/Runtime.mm index e98f56f4..2de309cc 100644 --- a/NativeScript/runtime/Runtime.mm +++ b/NativeScript/runtime/Runtime.mm @@ -198,8 +198,6 @@ void DisposeIsolateWhenPossible(Isolate* isolate) { } this->isolate_->TerminateExecution(); - // TODO: fix race condition on workers where a queue can leak (maybe calling Terminate before - // Initialize?) Caches::Workers->ForEach([currentIsolate](int& key, std::shared_ptr& value) { auto childWorkerWrapper = static_cast(value->UserData()); if (childWorkerWrapper->GetMainIsolate() == currentIsolate) { diff --git a/NativeScript/runtime/WorkerWrapper.mm b/NativeScript/runtime/WorkerWrapper.mm index f0f23fcb..70ea6e40 100644 --- a/NativeScript/runtime/WorkerWrapper.mm +++ b/NativeScript/runtime/WorkerWrapper.mm @@ -40,7 +40,7 @@ const int WorkerWrapper::WorkerId() { return this->workerId_; } void WorkerWrapper::PostMessage(std::shared_ptr message) { - if (!this->isTerminating_) { + if (!this->isTerminating_ && !this->isClosing_) { this->queue_.Push(message); } } @@ -66,7 +66,7 @@ Local global = context->Global(); for (std::shared_ptr message : messages) { - if (this->isTerminating_) { + if (this->isTerminating_ || this->isClosing_) { break; } TryCatch tc(this->workerIsolate_); @@ -76,6 +76,14 @@ this->CallOnErrorHandlers(tc); } } + + if (this->isClosing_) { + bool wasTerminating = this->isTerminating_.exchange(true); + if (!wasTerminating) { + this->queue_.Terminate(); + this->isRunning_ = false; + } + } } void WorkerWrapper::BackgroundLooper(std::function func) { @@ -101,7 +109,18 @@ this->isDisposed_ = true; Runtime* runtime = Runtime::GetCurrentRuntime(); - delete runtime; + if (runtime != nullptr) { + delete runtime; + } else { + // Runtime was never created (worker terminated before initialization). + // The runtime destructor normally handles this cleanup, so do it here. + int workerId = this->workerId_; + bool found; + auto state = Caches::Workers->Get(workerId, found); + if (found) { + Caches::Workers->Remove(workerId); + } + } } void WorkerWrapper::Close() { this->isClosing_ = true; } diff --git a/TestRunner/app/tests/shared/Workers/index.js b/TestRunner/app/tests/shared/Workers/index.js index 31e3dc0e..1a94f021 100644 --- a/TestRunner/app/tests/shared/Workers/index.js +++ b/TestRunner/app/tests/shared/Workers/index.js @@ -381,6 +381,28 @@ describe("TNS Workers", () => { }, DEFAULT_TIMEOUT_BEFORE_ASSERT); }); + it("Worker should fully shut down after close() without needing terminate()", (done) => { + var worker = new Worker("./tests/shared/Workers/EvalWorker.js"); + + worker.postMessage({ + eval: "close(); postMessage('closing');" + }); + + var responseCounter = 0; + worker.onmessage = (msg) => { + responseCounter++; + }; + + setTimeout(() => { + worker.postMessage({ eval: "postMessage('should not arrive');" }); + }, 500); + + setTimeout(() => { + expect(responseCounter).toBe(1); + done(); + }, DEFAULT_TIMEOUT_BEFORE_ASSERT + 1000); + }); + it("Test onerror invoked for a script that has invalid syntax", (done) => { var worker = new Worker("./tests/shared/Workers/WorkerInvalidSyntax.js");