From 59edaa4a5da8465e6d43a1c7a1715d2a91bd99f5 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 11 Sep 2026 18:25:09 -0300 Subject: [PATCH] fix: keep the Worker object rooted until its thread ends, and report the end as nsworkerended MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/worker-threads.md | 53 ++- test-app/app/src/main/assets/app/mainpage.js | 2 + .../app/tests/messaging/deadlockChild.js | 5 + .../app/tests/messaging/deadlockParent.js | 13 + .../assets/app/tests/testWorkerLifetime.js | 301 ++++++++++++++++++ .../app/tests/workerLifetimeCloseWorker.js | 6 + .../runtime/src/main/cpp/CallbackHandlers.cpp | 7 +- .../runtime/src/main/cpp/WorkerEvents.cpp | 22 +- test-app/runtime/src/main/cpp/WorkerEvents.h | 10 + .../runtime/src/main/cpp/WorkerWrapper.cpp | 42 ++- test-app/runtime/src/main/cpp/WorkerWrapper.h | 10 + test-app/runtime/src/main/cpp/js/README.md | 8 +- test-app/runtime/src/main/cpp/js/events.js | 12 +- .../src/main/cpp/js/node-worker-threads.js | 44 ++- .../runtime/src/main/cpp/js/worker-events.js | 12 +- 15 files changed, 516 insertions(+), 31 deletions(-) create mode 100644 test-app/app/src/main/assets/app/tests/messaging/deadlockChild.js create mode 100644 test-app/app/src/main/assets/app/tests/messaging/deadlockParent.js create mode 100644 test-app/app/src/main/assets/app/tests/testWorkerLifetime.js create mode 100644 test-app/app/src/main/assets/app/tests/workerLifetimeCloseWorker.js diff --git a/docs/worker-threads.md b/docs/worker-threads.md index 35078728b..defd7692b 100644 --- a/docs/worker-threads.md +++ b/docs/worker-threads.md @@ -54,7 +54,7 @@ means deliberately unsupported. | `threadName` | shim | Always `undefined`. | | `workerData` | shim | Always `null` — see below. | | `parentPort` | shim | `null` on the main isolate. Inside a worker, a `MessagePort`-shaped `EventTarget` over the worker's existing parent channel: `postMessage` forwards to the global `postMessage`, `message`/`messageerror` are re-dispatched from the worker global scope, `start()` and `close()` are no-ops. It is **not** a real port: not transferable, no queue of its own. | -| `Worker` | shim | A class over the runtime's global `Worker` with a small Node-style emitter (`on`/`once`/`off`/`removeListener`) for `message`, `messageerror`, `error`, `online` and `exit`. `postMessage(value, transfer)` and `terminate()` forward. `online` is emitted off a microtask after construction, not from the thread. Unsupported options throw a `TypeError` naming the option: `workerData`, `env`, `eval`, `transferList`, and `stdin`/`stdout`/`stderr` when explicitly truthy. The runtime's own `Worker` options (`androidPriority`) ride along untouched — the native constructor ignores keys it does not know. | +| `Worker` | shim | A class over the runtime's global `Worker` with a small Node-style emitter (`on`/`once`/`off`/`removeListener`) for `message`, `messageerror`, `error`, `online` and `exit`. `postMessage(value, transfer)` and `terminate()` forward. `online` is emitted off a microtask after construction, not from the thread. `exit` (always code `0`) fires exactly once, when the thread has ended, whether the worker was terminated or ended by its own `close()`; `terminate()` resolves at the same point. Unsupported options throw a `TypeError` naming the option: `workerData`, `env`, `eval`, `transferList`, and `stdin`/`stdout`/`stderr` when explicitly truthy. The runtime's own `Worker` options (`androidPriority`) ride along untouched — the native constructor ignores keys it does not know. | | `postMessageToThread` | throws | `Error: postMessageToThread is not supported in this runtime`. | | `moveMessagePortToContext` | throws | `Error: moveMessagePortToContext is not supported in this runtime`. | | `locks` | absent | Web Locks are not implemented; the property does not exist. | @@ -72,12 +72,16 @@ Values are cloned on the way in and deserialized fresh on each read, so mutating the object you passed does not reach a reader, and two readers never share one object. -### `exit` comes only from `terminate()` +### `exit` fires when the thread has ended, always with code `0` -The runtime has no thread-exit signal — nothing reports that a worker's isolate -finished. `terminate()` therefore resolves with `0` and emits `exit` with code -`0` on the way, and that is the only path that emits it. A worker that ends by -its own `close()` produces no `exit`. +`exit` is emitted once, from the runtime's end-of-worker notification, so every +`message` and `error` the worker produced before it ended has been delivered +first. Node reports the thread's exit code; this runtime has none to report, so +the code is `0` whichever way the worker ended — `terminate()`, its own +`close()`, an uncaught error or a missing entry. `terminate()` resolves with +`0` at the same moment `exit` fires. A parent that is itself tearing down never +delivers the notification, so a `terminate()` awaited from a dying isolate +stays pending, as it does in Node when the parent process exits. ### A worker error carries no `error` object, and the worker scope's `onerror` is not an event @@ -264,3 +268,40 @@ rather than raising a `DataCloneError`, which is long-standing behaviour app code relies on. Transfer is not part of that leniency — a port in a worker transfer list is validated exactly as it is everywhere else, since degrading a transfer would strand the port's sibling. + +## Worker lifetime + +**A `Worker` is held strongly by the runtime from the moment it is constructed +until its thread ends**, the way a browser keeps a running worker's handle +alive. Dropping every reference to one does not stop it: it keeps running, and +it keeps dispatching `message` and `error` events at the handlers installed on +it. + +```js +(function () { + const worker = new Worker("./worker.js"); + worker.onmessage = handle; // still fires; nothing here holds `worker` + worker.postMessage("go"); +})(); +``` + +Being a GC root also means a `Worker` is a well-behaved key: put one in a +`WeakMap`, `WeakSet` or `WeakRef` and the entry survives for as long as the +worker runs. + +The root is released when the worker ends — `terminate()`, or the worker's own +`close()`. `terminate()` only starts the wind-down: the object stays rooted +until the worker thread has actually finished and reported that to the parent. +From then on the object is collectable like any other, and the runtime drops +the native side with it. Nothing about a *finished* worker is kept alive. + +### `nsworkerended` + +When the worker's thread has finished, the runtime dispatches a plain `Event` +named `nsworkerended` on the `Worker` object. It is **internal and +non-standard** — the web has no end-of-worker event, and the name is +deliberately outside the standard namespace. It exists so that +`node:worker_threads` can report `'exit'` for a worker that ended by its own +`close()`; app code should not rely on it. The event is best effort: a worker +whose parent is already tearing down never delivers it, because the parent's +own teardown disposes the worker anyway. diff --git a/test-app/app/src/main/assets/app/mainpage.js b/test-app/app/src/main/assets/app/mainpage.js index eef723e59..d773f3cfd 100644 --- a/test-app/app/src/main/assets/app/mainpage.js +++ b/test-app/app/src/main/assets/app/mainpage.js @@ -110,6 +110,8 @@ require('./tests/testCreateRequire'); require('./tests/testNodeUrlModule'); require('./tests/testImportMetaResolution'); require('./tests/testWorkerEsmEntry'); +// Worker wrapper reachability across GC (strong while running, collectable after) +require('./tests/testWorkerLifetime'); // Fetches from the in-app loopback fixture server, so it goes last require('./tests/testEsmHttpLoader'); // Node-API addon surface diff --git a/test-app/app/src/main/assets/app/tests/messaging/deadlockChild.js b/test-app/app/src/main/assets/app/tests/messaging/deadlockChild.js new file mode 100644 index 000000000..9f3098847 --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/messaging/deadlockChild.js @@ -0,0 +1,5 @@ +onmessage = function (event) { + var port = event.data.port; + postMessage(port, [port]); + Atomics.store(event.data.flag, 0, 1); +}; diff --git a/test-app/app/src/main/assets/app/tests/messaging/deadlockParent.js b/test-app/app/src/main/assets/app/tests/messaging/deadlockParent.js new file mode 100644 index 000000000..c60c1d596 --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/messaging/deadlockParent.js @@ -0,0 +1,13 @@ +// Leaves a message that carries a port on this worker's own loop, undrained, +// at the moment the parent terminates it: the port's sibling is port1, owned +// by this worker. Spinning inside a timer callback keeps the loop from +// draining while still letting terminate() interrupt the JS. +var channel = new MessageChannel(); +var child = new Worker("./deadlockChild.js"); +var flag = new Int32Array(new SharedArrayBuffer(4)); +child.postMessage({ port: channel.port2, flag: flag }, [channel.port2]); +setTimeout(function () { + while (Atomics.load(flag, 0) === 0) {} + postMessage("ready"); + for (;;) {} +}, 0); diff --git a/test-app/app/src/main/assets/app/tests/testWorkerLifetime.js b/test-app/app/src/main/assets/app/tests/testWorkerLifetime.js new file mode 100644 index 000000000..3e83f02a5 --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/testWorkerLifetime.js @@ -0,0 +1,301 @@ +// Worker lifetime under GC. A running worker's JS wrapper is a GC root, so it +// behaves like any other strongly held object: weak collections keyed on it +// keep their entries, and it keeps answering messages nobody holds a reference +// to it for. Once the worker ends — terminate() or its own close() — the root +// is dropped and the wrapper becomes collectable. + +describe("Worker lifetime", function () { + // GC polling and worker teardown are far slower on a device than the + // jasmine default allows for. + var originalTimeout; + beforeEach(function () { + originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; + jasmine.DEFAULT_TIMEOUT_INTERVAL = 30000; + }); + afterEach(function () { + jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout; + }); + + var WORKER_COUNT = 4; + var PAYLOAD_SIZE = 64; + + // A collection per loop turn: weak-collection clearing needs turns after + // the collect, so nothing here asserts synchronously after __collect(). + function pollGC(predicate, cb) { + var turns = 0; + (function poll() { + __collect(); + if (predicate() || turns >= 100) { + cb(); + return; + } + turns++; + setTimeout(poll, 20); + })(); + } + + // Reached through a call rather than a closure, so the worker it derefs + // cannot end up in a scope the caller's later callbacks keep alive. + function terminateWorker(ref) { + var worker = ref.deref(); + if (worker !== undefined) { + worker.terminate(); + } + } + + function postToWorker(ref, message) { + var worker = ref.deref(); + if (worker !== undefined) { + worker.postMessage(message); + } + } + + // Enough allocation to put V8 part-way through an incremental/concurrent + // mark, so the collection that follows finishes a mark that was already + // running rather than starting an atomic one. + function churn() { + var sink = null; + for (var i = 0; i < 24; i++) { + var block = new Array(8192); + for (var j = 0; j < 8192; j++) { + block[j] = { j: j, s: "churn-" + j }; + } + sink = block; + } + return sink !== null; + } + + function makePayload(id) { + var payload = new Array(PAYLOAD_SIZE); + for (var i = 0; i < PAYLOAD_SIZE; i++) { + payload[i] = "payload-" + id + "-" + i; + } + return payload; + } + + it("a live Worker survives GC as a WeakMap key", function (done) { + // Nothing outside this map holds the values: an entry whose key stays + // alive while its value is not marked is what leaves a dangling value + // slot behind. + var sideTable = new WeakMap(); + var refs = []; + var replies = 0; + + for (var i = 0; i < WORKER_COUNT; i++) { + refs.push((function (index) { + var worker = new Worker("./eventLoopEchoWorker.js"); + // A second entry reachable only through the first one's value, + // so resolving these takes more than one ephemeron pass. + var link = { id: index }; + sideTable.set(link, { deep: index, payload: makePayload("deep" + index) }); + sideTable.set(worker, { id: index, link: link, payload: makePayload(index) }); + worker.onmessage = function () { replies++; }; + worker.postMessage("ping"); + return new WeakRef(worker); + })(i)); + } + + var round = 0; + function spin() { + churn(); + // async execution runs the collection from a task, so V8 treats the + // stack as pointer-free and the workers are genuinely unreachable + // for it — a conservative scan of this frame would not let them be. + __collect({ execution: "async" }).then(function () { + __collect(); + + // Only some turns touch the workers: a turn that does not leaves + // them dead for a whole mark cycle. + if (round % 3 === 0) { + for (var i = 0; i < refs.length; i++) { + postToWorker(refs[i], "ping-" + round); + } + } + + round++; + if (round < 15) { + setTimeout(spin, 20); + return; + } + + for (var k = 0; k < refs.length; k++) { + var survivor = refs[k].deref(); + expect(survivor).not.toBeUndefined(); + if (survivor === undefined) { + continue; + } + var entry = sideTable.get(survivor); + expect(entry).not.toBeUndefined(); + if (entry !== undefined) { + expect(entry.id).toBe(k); + expect(entry.payload.length).toBe(PAYLOAD_SIZE); + expect(entry.payload[PAYLOAD_SIZE - 1]).toBe( + "payload-" + k + "-" + (PAYLOAD_SIZE - 1)); + var deep = sideTable.get(entry.link); + expect(deep).not.toBeUndefined(); + if (deep !== undefined) { + expect(deep.deep).toBe(k); + expect(deep.payload.length).toBe(PAYLOAD_SIZE); + } + } + } + expect(replies).toBeGreaterThan(0); + + for (var t = 0; t < refs.length; t++) { + terminateWorker(refs[t]); + } + done(); + }); + } + spin(); + }); + + it("an unreferenced live Worker still answers messages", function (done) { + var reply = null; + var ref = (function () { + var worker = new Worker("./eventLoopEchoWorker.js"); + worker.onmessage = function (event) { reply = event.data; }; + worker.postMessage("hello"); + return new WeakRef(worker); + })(); + + pollGC(function () { return reply !== null; }, function () { + expect(reply).toBe("hello"); + expect(ref.deref()).not.toBeUndefined(); + terminateWorker(ref); + done(); + }); + }); + + it("a terminated Worker becomes collectable", function (done) { + var ended = false; + var ref = (function () { + var worker = new Worker("./eventLoopEchoWorker.js"); + worker.addEventListener("nsworkerended", function () { ended = true; }); + worker.postMessage("ping"); + return new WeakRef(worker); + })(); + + setTimeout(function () { + terminateWorker(ref); + // The root outlives terminate() by design: it goes only when the + // thread reports its end, so collectability is observable no + // earlier than that. + pollGC(function () { return ended; }, function () { + expect(ended).toBe(true); + pollGC(function () { return ref.deref() === undefined; }, function () { + expect(ref.deref()).toBeUndefined(); + done(); + }); + }); + }, 150); + }); + + it("a Worker that closed itself becomes collectable", function (done) { + var ended = false; + var ref = (function () { + var worker = new Worker("./workerLifetimeCloseWorker.js"); + worker.addEventListener("nsworkerended", function () { ended = true; }); + worker.postMessage("close"); + return new WeakRef(worker); + })(); + + pollGC(function () { return ended; }, function () { + expect(ended).toBe(true); + pollGC(function () { return ref.deref() === undefined; }, function () { + expect(ref.deref()).toBeUndefined(); + done(); + }); + }); + }); +}); + +describe("node:worker_threads Worker exit", function () { + // GC polling and worker teardown are far slower on a device than the + // jasmine default allows for. + var originalTimeout; + beforeEach(function () { + originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; + jasmine.DEFAULT_TIMEOUT_INTERVAL = 30000; + }); + afterEach(function () { + jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout; + }); + + var wt = require("node:worker_threads"); + + it("emits 'exit' once when the worker closes itself", function (done) { + var worker = new wt.Worker("~/tests/workerLifetimeCloseWorker.js"); + var codes = []; + worker.on("exit", function (code) { codes.push(code); }); + worker.postMessage("go"); + + setTimeout(function () { + expect(codes).toEqual([0]); + done(); + }, 2000); + }); + + it("emits 'exit' once on terminate(), after the thread ended, and resolves then", function (done) { + var worker = new wt.Worker("~/tests/eventLoopEchoWorker.js"); + var codes = []; + worker.on("exit", function (code) { codes.push(code); }); + + setTimeout(function () { + var resolved = null; + worker.terminate().then(function (code) { + resolved = code; + // 'exit' precedes the promise settling. + expect(codes).toEqual([0]); + }); + setTimeout(function () { + expect(resolved).toBe(0); + expect(codes).toEqual([0]); + worker.terminate().then(function (code) { + expect(code).toBe(0); + expect(codes).toEqual([0]); + done(); + }); + }, 2000); + }, 150); + }); +}); + +describe("Worker teardown with a transferred port in flight", function () { + // GC polling and worker teardown are far slower on a device than the + // jasmine default allows for. + var originalTimeout; + beforeEach(function () { + originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; + jasmine.DEFAULT_TIMEOUT_INTERVAL = 30000; + }); + afterEach(function () { + jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout; + }); + + // The parent worker's loop still holds a message carrying a port whose + // sibling that worker owns; dropping it during shutdown posts the sibling's + // close sentinel back into the loop being shut down. + it("ends a terminated worker whose dropped message sentinels a port it owns", function (done) { + var worker = new Worker("./messaging/deadlockParent.js"); + var ended = false; + worker.addEventListener("nsworkerended", function () { ended = true; }); + worker.onerror = function (event) { + fail("worker error: " + event.message); + return true; + }; + worker.onmessage = function (event) { + expect(event.data).toBe("ready"); + worker.terminate(); + var deadline = Date.now() + 10000; + (function poll() { + if (ended || Date.now() > deadline) { + expect(ended).toBe(true); + done(); + return; + } + setTimeout(poll, 50); + })(); + }; + }); +}); diff --git a/test-app/app/src/main/assets/app/tests/workerLifetimeCloseWorker.js b/test-app/app/src/main/assets/app/tests/workerLifetimeCloseWorker.js new file mode 100644 index 000000000..ed90b183a --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/workerLifetimeCloseWorker.js @@ -0,0 +1,6 @@ +// Ends itself on request, so the parent can observe the end-of-worker path +// that does not go through terminate(). +onmessage = function () { + postMessage("closing"); + close(); +}; diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp index 27e031c07..65226985e 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp @@ -1732,11 +1732,12 @@ CallbackHandlers::WorkerObjectTerminateCallback(const v8::FunctionCallbackInfoTerminate(); } - - // Reset the persistent Worker object handle and drop the registry entry - WorkerWrapper::ClearWorkerOnParent(id); } catch (NativeScriptException &ex) { ex.ReThrowToV8(); } catch (std::exception e) { diff --git a/test-app/runtime/src/main/cpp/WorkerEvents.cpp b/test-app/runtime/src/main/cpp/WorkerEvents.cpp index aea42802a..978aeb014 100644 --- a/test-app/runtime/src/main/cpp/WorkerEvents.cpp +++ b/test-app/runtime/src/main/cpp/WorkerEvents.cpp @@ -14,12 +14,13 @@ namespace { /* * The worker-events builtin's delivery callouts for this isolate. Both message - * directions share emitMessage; only the receiver differs. emitError is - * parent-side only. + * directions share emitMessage; only the receiver differs. emitError and + * emitEnded are parent-side only. */ struct WorkerEventsState { Global emitMessage; Global emitError; + Global emitEnded; }; Local CalloutOf(Local exports, Isolate* isolate, const char* name) { @@ -48,6 +49,7 @@ void WorkerEvents::Init(Local context) { Local emitMessage = CalloutOf(exports, isolate, "emitMessage"); Local emitError = CalloutOf(exports, isolate, "emitError"); + Local emitEnded = CalloutOf(exports, isolate, "emitEnded"); auto* state = RuntimeState::For(isolate); if (state == nullptr) { @@ -55,6 +57,7 @@ void WorkerEvents::Init(Local context) { } state->emitMessage.Reset(isolate, emitMessage); state->emitError.Reset(isolate, emitError); + state->emitEnded.Reset(isolate, emitEnded); } void WorkerEvents::EmitMessage(Isolate* isolate, Local receiver, @@ -122,4 +125,19 @@ bool WorkerEvents::EmitError(Isolate* isolate, Local receiver, return result->BooleanValue(isolate); } +void WorkerEvents::EmitEnded(Isolate* isolate, Local receiver) { + auto* state = RuntimeState::For(isolate); + if (state == nullptr || state->emitEnded.IsEmpty()) { + return; + } + Runtime* runtime = Runtime::TryGetRuntime(isolate); + if (runtime == nullptr) { + return; + } + Local context = runtime->GetContext(); + + Local result; + (void)state->emitEnded.Get(isolate)->Call(context, receiver, 0, nullptr).ToLocal(&result); +} + } // namespace tns diff --git a/test-app/runtime/src/main/cpp/WorkerEvents.h b/test-app/runtime/src/main/cpp/WorkerEvents.h index 9a011df69..1149f9732 100644 --- a/test-app/runtime/src/main/cpp/WorkerEvents.h +++ b/test-app/runtime/src/main/cpp/WorkerEvents.h @@ -49,6 +49,16 @@ class WorkerEvents { static bool EmitError(v8::Isolate* isolate, v8::Local receiver, const std::string& message, const std::string& source, const std::string& stackTrace, int lineNumber); + + /* + * Dispatches `nsworkerended` on `receiver` (the Worker object, on the + * parent isolate) once the worker's thread has finished. Internal and + * non-standard: the web has no end-of-worker event, and the + * node:worker_threads shim is what turns this into an 'exit'. A listener + * that throws leaves the exception pending for the caller's TryCatch. + * No-op before Init has run. + */ + static void EmitEnded(v8::Isolate* isolate, v8::Local receiver); }; } // namespace tns diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp index 7bc272b24..7fea571f6 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp @@ -707,13 +707,13 @@ void WorkerWrapper::BackgroundLooper(std::shared_ptr self) { isDisposed_ = true; - // Notify the parent thread so the Worker object's persistent handle and - // the registry entry are released (no-op if terminate() or the parent's + // Notify the parent thread so the end reaches the Worker object and its + // persistent handle and registry entry are released (no-op if the parent's // own shutdown already cleared them). if (auto parentTasks = parentTasks_.lock()) { int workerId = workerId_; parentTasks->PostInternal([workerId]() { - WorkerWrapper::ClearWorkerOnParent(workerId); + WorkerWrapper::NotifyThreadEndedOnParent(workerId); }); } @@ -757,6 +757,42 @@ void WorkerWrapper::ClearWorkerOnParent(int workerId) { } } +void WorkerWrapper::NotifyThreadEndedOnParent(int workerId) { + auto wrapper = WorkerWrapper::GetById(workerId); + if (wrapper == nullptr) { + return; + } + + Isolate* isolate = wrapper->parentIsolate_; + { + v8::Locker locker(isolate); + Isolate::Scope isolate_scope(isolate); + HandleScope handle_scope(isolate); + + if (wrapper->poWorker_ != nullptr && !wrapper->poWorker_->IsEmpty()) { + auto worker = Local::New(isolate, *wrapper->poWorker_); + auto context = Runtime::GetRuntime(isolate)->GetContext(); + Context::Scope context_scope(context); + + try { + // A listener that throws has no JS frame below it to unwind + // into, so it is reported here the way a timer callback's + // exception is. + TryCatch tc(isolate); + WorkerEvents::EmitEnded(isolate, worker); + if (tc.HasCaught() && + !NativeScriptException::ContainUncaughtCallbackException(isolate, tc)) { + ReportFromEventLoopEntry(isolate, tc); + } + } catch (NativeScriptException& ex) { + ex.ReThrowToV8(); + } + } + } + + ClearWorkerOnParent(workerId); +} + void WorkerWrapper::TerminateChildren(Isolate* parentIsolate) { std::vector> children; { diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.h b/test-app/runtime/src/main/cpp/WorkerWrapper.h index 5dd3b2fa8..683131bba 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.h +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.h @@ -129,6 +129,16 @@ class WorkerWrapper : public std::enable_shared_from_this { */ static void ClearWorkerOnParent(int workerId); + /* + * Parent thread only, from the worker thread's last act: dispatches + * `nsworkerended` on the Worker object and only then releases it. The + * Worker object stays rooted from construction until this runs, so a + * terminate() that is still draining can never lose its wrapper. A parent + * that is itself tearing down clears its children directly and never gets + * here, so the notification is best effort. + */ + static void NotifyThreadEndedOnParent(int workerId); + /* * Terminates and clears all workers whose parent is the given isolate. * Must run on the parent's thread, before the parent isolate is disposed diff --git a/test-app/runtime/src/main/cpp/js/README.md b/test-app/runtime/src/main/cpp/js/README.md index 580ce108c..5c4665d45 100644 --- a/test-app/runtime/src/main/cpp/js/README.md +++ b/test-app/runtime/src/main/cpp/js/README.md @@ -122,10 +122,10 @@ The two extra rules a lazy builtin lives by: are whatever user code left behind, so it should not reach for them at all. - The per-instance wrappers `defineEventHandler` creates live on the target's **own listener bag**, under a private symbol — never in a WeakMap keyed by - the target. An ObjectManager-registered object (a `Worker`) can be - resurrected by its finalizer while its thread is alive, and a resurrected - object's weak-collection entries are already gone, so a WeakMap would hand - the revived object a fresh, empty handler map. + the target. Own-instance state is Node's own design for handler attributes, + and it keeps the builtins independent of how the collector treats the + weak-collection entries of objects native code keeps alive — a `Worker` is + rooted by the runtime for as long as its thread runs. - No `import`/`export` — these are classic function bodies, not modules. - ESLint (`eslint.config.mjs` at the repo root, `npm run lint`) declares `exports`, `require`, `module`, `binding`, `primordials` and the reachable diff --git a/test-app/runtime/src/main/cpp/js/events.js b/test-app/runtime/src/main/cpp/js/events.js index 22ba21cc4..d2a706f53 100644 --- a/test-app/runtime/src/main/cpp/js/events.js +++ b/test-app/runtime/src/main/cpp/js/events.js @@ -44,12 +44,12 @@ function setListenerErrorReporter(fn) { // Event name -> handler-attribute wrapper (see defineEventHandler), stored on // the target's own listener bag under a symbol so it cannot collide with an -// event type. Deliberately NOT a WeakMap keyed by the target: a Worker is an -// ObjectManager-registered object whose finalizer resurrects it while its -// thread is alive, and a resurrected object's weak-collection entries are -// already gone. Each wrapper carries a `delta` that the listener count is -// corrected by: the wrapper occupies one slot in the listener list from its -// first assignment onwards, but a cleared handler is not a listener. +// event type. Deliberately NOT a WeakMap keyed by the target: the wrappers +// live with the target, as Node keeps them, and stay independent of how the +// collector treats weak-collection entries of objects that native code keeps +// alive. Each wrapper carries a `delta` that the listener count is corrected +// by: the wrapper occupies one slot in the listener list from its first +// assignment onwards, but a cleared handler is not a listener. var kHandlers = Symbol("handlers"); function handlersOf(target) { diff --git a/test-app/runtime/src/main/cpp/js/node-worker-threads.js b/test-app/runtime/src/main/cpp/js/node-worker-threads.js index da60893b3..fac70cbb8 100644 --- a/test-app/runtime/src/main/cpp/js/node-worker-threads.js +++ b/test-app/runtime/src/main/cpp/js/node-worker-threads.js @@ -28,6 +28,7 @@ const { ObjectCreate, ObjectDefineProperty, ObjectFreeze, + Promise, PromisePrototypeThen, PromiseResolve, SymbolFor, @@ -145,6 +146,8 @@ class WorkerEmitter { class Worker extends WorkerEmitter { #worker; #exited = false; + // Every terminate() promise settles when the thread's end is reported. + #exitWaiters = []; constructor(filename, options) { super(); @@ -182,24 +185,53 @@ class Worker extends WorkerEmitter { worker.onerror = function (error) { self.emit("error", error); }; + // The runtime's end-of-worker event: the one place 'exit' comes from, for + // a worker's own close() and for terminate() alike, so nothing the worker + // sent before it ended can follow 'exit'. + FunctionPrototypeCall( + addEventListener, + worker, + "nsworkerended", + function () { + self.#reportExit(); + } + ); soon(function () { self.emit("online", undefined); }); } + // Node emits 'exit' once and settles terminate() after it. The code is + // always 0: this runtime has no thread exit status to report, and the + // cross-runtime suite pins that for every end a worker can take. + #reportExit() { + if (this.#exited) { + return; + } + this.#exited = true; + this.emit("exit", 0); + const waiters = this.#exitWaiters; + this.#exitWaiters = []; + for (let i = 0; i < waiters.length; i++) { + waiters[i](0); + } + } + postMessage(value, transfer) { this.#worker.postMessage(value, transfer); } + // Resolves with the exit code once the thread has actually ended. A parent + // that is itself tearing down never delivers that signal, so the promise + // stays pending there, as it does in Node when the parent dies. terminate() { + if (this.#exited) { + return PromiseResolve(0); + } this.#worker.terminate(); const self = this; - return PromisePrototypeThen(PromiseResolve(), function () { - if (!self.#exited) { - self.#exited = true; - self.emit("exit", 0); - } - return 0; + return new Promise(function (resolve) { + ArrayPrototypePush(self.#exitWaiters, resolve); }); } } diff --git a/test-app/runtime/src/main/cpp/js/worker-events.js b/test-app/runtime/src/main/cpp/js/worker-events.js index 73536fc44..e3283c173 100644 --- a/test-app/runtime/src/main/cpp/js/worker-events.js +++ b/test-app/runtime/src/main/cpp/js/worker-events.js @@ -12,6 +12,7 @@ const { ObjectDefineProperty, ObjectSetPrototypeOf } = primordials; const { + Event, EventTarget, defineEventHandler, dispatchEventRethrowing, @@ -73,6 +74,15 @@ function emitError(message, filename, lineno, stackTrace) { return event.defaultPrevented; } +// The parent-side end-of-worker callout, invoked by native with the Worker +// object as `this` once the worker's thread has finished — its own close() as +// much as a terminate(). `nsworkerended` is internal and non-standard: the web +// has no end-of-worker event, and the node:worker_threads shim is what turns +// this into an 'exit'. +function emitEnded() { + dispatchEventRethrowing(this, new Event("nsworkerended")); +} + ObjectSetPrototypeOf(g.Worker.prototype, EventTarget.prototype); defineEventHandler(g.Worker.prototype, "message"); defineEventHandler(g.Worker.prototype, "messageerror"); @@ -99,4 +109,4 @@ for (const name of ["onmessage", "onmessageerror"]) { }); } -module.exports = { emitMessage, emitError }; +module.exports = { emitMessage, emitError, emitEnded };