fix(worker): let terminate() stop a worker that called close()

close() only ends the worker once its running callback returns, and terminate() returned early for a closing worker, so a worker that kept running after close() could not be stopped. terminate() now interrupts it like any other running worker.

Author: adrian-niculescu

test-app/app/src/main/assets/app/mainpage.js

@@ -27,6 +27,7 @@ require("./tests/testWebAssembly");
 require("./tests/testEventLoop");
 require("./tests/testMultithreadedJavascript");
 require("./tests/testWorkerTerminateDuringLoad");
+require("./tests/testWorkerTerminateAfterClose");
 require("./tests/testWorkerOptions");
 require("./tests/testWorkerResourceLimits");
 require("./tests/testInterfaceDefaultMethods");

test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js

@@ -0,0 +1,30 @@
+describe("Worker terminate after close", function () {
+	var SETTLE_AFTER = 300;
+
+	it("stops a worker that keeps running after it called close()", function (done) {
+		var counter = new Int32Array(new SharedArrayBuffer(4));
+		var worker = new Worker("./workerCloseThenSpinWorker.js");
+		worker.postMessage(counter.buffer);
+		var started = Date.now();
+
+		(function waitForSpin() {
+			if (Atomics.load(counter, 0) === 0) {
+				if (Date.now() - started > 5000) {
+					fail("the worker never started running");
+					done();
+					return;
+				}
+				setTimeout(waitForSpin, 20);
+				return;
+			}
+			worker.terminate();
+			setTimeout(function () {
+				var afterTerminate = Atomics.load(counter, 0);
+				setTimeout(function () {
+					expect(Atomics.load(counter, 0)).toBe(afterTerminate);
+					done();
+				}, SETTLE_AFTER);
+			}, SETTLE_AFTER);
+		})();
+	});
+});

test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js

@@ -0,0 +1,9 @@
+// close() lets the running callback finish, so this one never returns unless
+// terminate() interrupts it.
+onmessage = function (event) {
+	var counter = new Int32Array(event.data);
+	close();
+	for (;;) {
+		Atomics.add(counter, 0, 1);
+	}
+};

test-app/runtime/src/main/cpp/WorkerWrapper.cpp

@@ -142,8 +142,7 @@ void WorkerWrapper::PostMessageToParent(std::shared_ptr<worker::Message> message
 }
 
 void WorkerWrapper::Terminate() {
-    if (isClosing_ || isDisposed_) {
-        // The worker is already shutting down on its own; nothing to do.
+    if (isDisposed_) {
         return;
     }
 
@@ -160,8 +159,8 @@ void WorkerWrapper::Terminate() {
         std::lock_guard<std::mutex> lock(workerIsolateMutex_);
         Isolate* isolate = workerIsolate_.load();
         if (isolate != nullptr) {
-            // The only v8 call that is legal from another thread - interrupts any
-            // JS currently running on the worker (e.g. a busy loop).
+            // Legal from any thread: interrupts any JS currently running on the
+            // worker (e.g. a busy loop, including one that follows close()).
             isolate->TerminateExecution();
             // A pump parked with nothing queued runs no JS, so the interrupt
             // above never materializes for it - the loop's own flag ends it.