diff --git a/src/jsc/bindings/webcore/JSMessagePort.cpp b/src/jsc/bindings/webcore/JSMessagePort.cpp index 3b70232ef476..b6d6fd62f768 100644 --- a/src/jsc/bindings/webcore/JSMessagePort.cpp +++ b/src/jsc/bindings/webcore/JSMessagePort.cpp @@ -208,7 +208,7 @@ static inline bool setJSMessagePort_onmessageSetter(JSGlobalObject& lexicalGloba if (value.isCallable()) thisObject.wrapped().jsRef(&lexicalGlobalObject); else - thisObject.wrapped().jsUnref(&lexicalGlobalObject); + thisObject.wrapped().jsUnref(); return true; } @@ -354,7 +354,6 @@ static inline JSC::EncodedJSValue jsMessagePortPrototypeFunction_closeBody(JSC:: UNUSED_PARAM(throwScope); UNUSED_PARAM(callFrame); auto& impl = castedThis->wrapped(); - impl.jsUnref(lexicalGlobalObject); RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.close(); }))); } @@ -385,7 +384,7 @@ static inline JSC::EncodedJSValue jsMessagePortPrototypeFunction_unrefBody(JSC:: UNUSED_PARAM(throwScope); UNUSED_PARAM(callFrame); auto& impl = castedThis->wrapped(); - RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.jsUnref(lexicalGlobalObject); }))); + RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.jsUnref(); }))); } JSC_DEFINE_HOST_FUNCTION(jsMessagePortPrototypeFunction_unref, (JSGlobalObject * lexicalGlobalObject, CallFrame* callFrame)) diff --git a/src/jsc/bindings/webcore/MessagePort.cpp b/src/jsc/bindings/webcore/MessagePort.cpp index 1e1681c17d85..b252550bc977 100644 --- a/src/jsc/bindings/webcore/MessagePort.cpp +++ b/src/jsc/bindings/webcore/MessagePort.cpp @@ -224,31 +224,17 @@ void MessagePort::close() // it in hasPendingActivity()); marking our side Closed is sufficient. m_pipe->close(m_side, MessagePortPipe::CloseKind::Explicit); - // Release the self-reference taken by jsRef() (set when .onmessage is - // assigned or .ref() is called from JS). The JS .close() binding calls - // jsUnref() first; stop() and contextDestroyed() do not. - if (m_hasRef) { - m_hasRef = false; - if (auto* context = scriptExecutionContext()) - context->unrefEventLoop(); - deref(); - } - - // close() can run without a prior jsUnref() (warn-and-close, contextDestroyed()); - // clear the listener keepalive so a later listener add can't re-ref the loop. - if (m_isRefd) { - m_isRefd = false; - updateListenerEventLoopRef(); - } - - // Defer 'close' to a task (node fires it at uv close-callback timing, i.e. - // after sync code and microtasks), so a listener added after close() still - // observes it and close(cb) interleaves with other listeners. if (isContextStopped()) { + jsUnref(); removeAllEventListeners(); return; } + // Defer 'close' to a task (node fires it at uv close-callback timing, after + // sync code and microtasks) and release the loop refs there, matching node's + // handle which stays ref'd until its close callback runs. stop() releases + // them instead if teardown discards the task unrun. queueTaskKeepingObjectAlive(*this, TaskSource::PostedMessageQueue, [](MessagePort& port) { + port.jsUnref(); port.dispatchCloseEvent(); port.removeAllEventListeners(); }); @@ -282,13 +268,10 @@ void MessagePort::peerClosed() // drain is scheduled -- e.g. on('close') registered before on('message'). if (m_started && hasMessageEventListener()) flushQueuedMessagesBeforeClose(); - // Fire 'close' (guarded against a double dispatch) and release this side's loop refs - // so the loop can idle, matching node. + // Node's 'close' handler sees hasRef() false (its handle closes when the + // peer's close arrives): release this side's loop refs, then fire 'close'. + jsUnref(); dispatchCloseEvent(); - // jsUnref() clears both the listener loop-ref (m_isRefd) and the onmessage/ref() - // keepalive (m_hasRef), so a listening transferred port stops pinning the loop. - auto* globalObject = defaultGlobalObject(context->globalObject()); - jsUnref(globalObject); } TransferredMessagePort MessagePort::disentangle() @@ -301,24 +284,10 @@ TransferredMessagePort MessagePort::disentangle() removeAllEventListeners(); m_hasMessageEventListener = false; - // Release the self-reference taken by jsRef() on the sending side. After - // transfer this object is inert (the receiving side gets a fresh - // MessagePort for the same pipe endpoint) and is no longer a destruction - // observer, so nothing else will ever release a ref taken here. - // The caller (disentanglePorts) holds a RefPtr, so deref() is safe. - if (m_hasRef) { - m_hasRef = false; - if (auto* context = scriptExecutionContext()) - context->unrefEventLoop(); - deref(); - } - - // A transferred port is inert; clear the listener keepalive too so hasRef() - // reports false (the disentangle analogue of the close() reset above). - if (m_isRefd) { - m_isRefd = false; - updateListenerEventLoopRef(); - } + // A transferred port is inert (the receiving side gets a fresh MessagePort + // for the same pipe endpoint), so nothing later would release jsRef()'s + // refs. The caller (disentanglePorts) holds a RefPtr across the deref(). + jsUnref(); // Hand the pipe endpoint to its next owner. Messages that arrive while // in transit buffer in the pipe; the receiving context's entangle() @@ -396,11 +365,20 @@ void MessagePort::dispatchEvent(Event& event) EventTarget::dispatchEvent(event); } +void MessagePort::stop() +{ + close(); + // Teardown discards close()'s queued 'close' task unrun, so release the refs here. + jsUnref(); +} + void MessagePort::contextDestroyed() { ASSERT(scriptExecutionContext()); - close(); + // jsRef()'s self-ref may be the last reference to this port. + Ref protectedThis { *this }; + stop(); ActiveDOMObject::contextDestroyed(); } @@ -552,15 +530,12 @@ WebCoreOpaqueRoot root(MessagePort* port) void MessagePort::jsRef(JSGlobalObject* lexicalGlobalObject) { - // A closed or transferred-away port can never receive messages again, so - // taking a self-ref (and an event-loop ref) here would only leak: - // close()/disentangle() have already run and nothing will ever release a - // ref taken afterwards. Same once the peer has closed: peerClosed() already - // ran jsUnref(), and nothing releases a ref re-taken after it, so `.ref()` - // or a late `onmessage =` would pin the loop forever. Node no-ops both. - // Only an explicit peer close counts: node never closes a channel because a - // port was collected, so keying on Closed alone made this GC-dependent. - if (!isEntangled() || m_pipe->isOtherSideClosedByRequest(m_side)) + // Once the port is closed, transferred away, explicitly closed by its peer, + // or its 'close' event has fired (the only signal when a collected peer + // closed the channel), nothing would ever release a ref taken here, so a + // late `.ref()` or `onmessage =` would pin the loop forever. Node no-ops + // ref() on a closed port too. + if (!isEntangled() || m_pipe->isOtherSideClosedByRequest(m_side) || m_closeEventDispatched) return; // Re-acquire the message-listener loop-ref (if a listener is present) that .unref() released. @@ -576,7 +551,7 @@ void MessagePort::jsRef(JSGlobalObject* lexicalGlobalObject) } } -void MessagePort::jsUnref(JSGlobalObject* lexicalGlobalObject) +void MessagePort::jsUnref() { // Also release the listener loop-ref; otherwise an always-listening transferred // port (a postMessageToThread control port) would pin the event loop forever. @@ -584,10 +559,13 @@ void MessagePort::jsUnref(JSGlobalObject* lexicalGlobalObject) m_isRefd = false; updateListenerEventLoopRef(); } + // unrefEventLoop() balances jsRef()'s refKeepAlive; every caller holds a + // reference across the deref(). if (m_hasRef) { m_hasRef = false; + if (auto* context = scriptExecutionContext()) + context->unrefEventLoop(); deref(); - Bun__eventLoop__refKeepAlive(WebCore::clientData(lexicalGlobalObject->vm())->bunVM, -1); } } diff --git a/src/jsc/bindings/webcore/MessagePort.h b/src/jsc/bindings/webcore/MessagePort.h index c08911798f25..50156c8b8fb7 100644 --- a/src/jsc/bindings/webcore/MessagePort.h +++ b/src/jsc/bindings/webcore/MessagePort.h @@ -79,8 +79,8 @@ class MessagePort final : public ActiveDOMObject, public EventTarget, public Thr // The worker's entry module finished evaluating: a start() requested before that takes effect now. void entrySettled(); void close(); - // Called on the entangled peer when this side closes: dispatches a - // 'close' event and releases the event-loop ref so the loop can idle. + // Called on the entangled peer when this side closes: drops the peer's + // loop refs, then fires its 'close' event. void peerClosed(); void dispatchCloseEvent(); @@ -114,8 +114,10 @@ class MessagePort final : public ActiveDOMObject, public EventTarget, public Thr JSValue tryTakeMessage(JSGlobalObject*, bool& hadMessage); void jsRef(JSGlobalObject*); - void jsUnref(JSGlobalObject*); - // Report the actual loop-ref state (matches Node's uv_has_ref), not the intent flag. + // .unref(); also drops every loop ref on close, transfer, or context stop. Idempotent. + void jsUnref(); + // The actual loop-ref state, not the intent flag: node's hasRef() stays true + // until the handle closes, right before its 'close' event fires. bool jsHasRef() { return m_hasRef || m_listenerLoopRefActive; } private: @@ -126,7 +128,7 @@ class MessagePort final : public ActiveDOMObject, public EventTarget, public Thr // ActiveDOMObject. void contextDestroyed() final; - void stop() final { close(); } + void stop() final; bool virtualHasPendingActivity() const final; // Deliver messages already queued when close() is called, before teardown. diff --git a/test/js/node/worker_threads/worker_threads.test.ts b/test/js/node/worker_threads/worker_threads.test.ts index 023ea833ffa1..2f449a3a31bd 100644 --- a/test/js/node/worker_threads/worker_threads.test.ts +++ b/test/js/node/worker_threads/worker_threads.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, setDefaultTimeout, test } from "bun:test"; -import { bunEnv, bunExe, isDebug, tempDir, tmpdirSync } from "harness"; +import { bunEnv, bunExe, isASAN, isDebug, isLinux, tempDir, tmpdirSync } from "harness"; import { once } from "node:events"; import fs from "node:fs"; import { join, relative, resolve } from "node:path"; @@ -947,6 +947,183 @@ test("MessagePort.hasRef() reports actual loop-ref state", () => { port1.close(); }); +// node: close() only starts closing the handle. hasRef() keeps reporting the handle's +// ref flag until the close callback fires 'close' (HandleWrap::OnClose marks the handle +// closed before emitting), so it is still true right after close() and false inside +// the handler. A port learns of its peer's close the same way, so it follows the same +// timing. Both ways of holding a ref are covered: a 'message' listener and ref(). +describe.each([ + ["a 'message' listener", (port: MessagePort) => port.on("message", () => {})], + ["ref()", (port: MessagePort) => port.ref()], +])("hasRef() around close() on a port kept alive by %s", (_, keepAlive) => { + test("own close(): true until its 'close' event, false inside and after it", async () => { + const { port1 } = new MessageChannel(); + keepAlive(port1); + const { promise, resolve } = Promise.withResolvers(); + port1.on("close", () => resolve(port1.hasRef())); + port1.close(); + const afterClose = port1.hasRef(); + const inCloseEvent = await promise; + expect({ afterClose, inCloseEvent, afterCloseEvent: port1.hasRef() }).toEqual({ + afterClose: true, + inCloseEvent: false, + afterCloseEvent: false, + }); + }); + + test("peer close(): true until this side's 'close' event, false inside and after it", async () => { + const { port1, port2 } = new MessageChannel(); + keepAlive(port1); + const { promise, resolve } = Promise.withResolvers(); + port1.on("close", () => resolve(port1.hasRef())); + port2.close(); + const afterPeerClose = port1.hasRef(); + const inCloseEvent = await promise; + expect({ afterPeerClose, inCloseEvent, afterCloseEvent: port1.hasRef() }).toEqual({ + afterPeerClose: true, + inCloseEvent: false, + afterCloseEvent: false, + }); + }); +}); + +// The closing half of node's test/parallel/test-messageport-hasref.js: closing one side +// leaves both sides reporting true until their 'close' events; the closing side's own +// 'close' fires first, so by the time the peer's handler runs both report false. +test("closing one side: both ports report true until their 'close' events, then false", async () => { + const { port1, port2 } = new MessageChannel(); + port1.on("message", () => {}); + port2.on("message", () => {}); + const events: string[] = []; + const { promise, resolve } = Promise.withResolvers(); + port2.on("close", () => events.push(`port2 close: ${port1.hasRef()} ${port2.hasRef()}`)); + port1.on("close", () => { + events.push(`port1 close: ${port1.hasRef()} ${port2.hasRef()}`); + resolve(); + }); + port2.close(); + const rightAfterClose = [port1.hasRef(), port2.hasRef()]; + await promise; + expect({ rightAfterClose, events }).toEqual({ + rightAfterClose: [true, true], + events: ["port2 close: true false", "port1 close: false false"], + }); +}); + +// Between close() and 'close' the handle is still alive in node, so ref changes made +// in that window show up in hasRef(), and 'close' still ends with it false. +test("hasRef() tracks unref() and listener changes made between close() and 'close'", async () => { + const { port1: unrefd } = new MessageChannel(); + unrefd.on("message", () => {}); + const unrefdClosed = Promise.withResolvers(); + unrefd.on("close", () => unrefdClosed.resolve(unrefd.hasRef())); + unrefd.close(); + unrefd.unref(); + const afterUnref = unrefd.hasRef(); + + const { port1: listened } = new MessageChannel(); + const listenedClosed = Promise.withResolvers(); + listened.on("close", () => listenedClosed.resolve(listened.hasRef())); + listened.close(); + listened.on("message", () => {}); + const afterListenerAdd = listened.hasRef(); + + expect({ + afterUnref, + unrefdInCloseEvent: await unrefdClosed.promise, + afterListenerAdd, + listenedInCloseEvent: await listenedClosed.promise, + }).toEqual({ + afterUnref: false, + unrefdInCloseEvent: false, + afterListenerAdd: true, + listenedInCloseEvent: false, + }); +}); + +// The refs close() leaves in place for hasRef() are dropped when 'close' fires, so a +// closed port cannot keep the process alive. Spawned: the failure mode is a hang. +test("a closed port stops keeping the process alive once 'close' has fired", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `const { MessageChannel } = require("worker_threads"); + const { port1 } = new MessageChannel(); + port1.on("message", () => {}); + port1.ref(); + port1.on("close", () => console.log("in close event: " + port1.hasRef())); + port1.close(); + console.log("after close(): " + port1.hasRef());`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ lines: stdout.split(/\r?\n/).filter(Boolean), stderr, exitCode, signalCode: proc.signalCode }).toEqual({ + lines: ["after close(): true", "in close event: false"], + stderr: "", + exitCode: 0, + signalCode: null, + }); +}); + +// Teardown can start while a close() is still waiting for its 'close' task (which is +// then never run): the worker's stop phase has to drop the refs that task would have. +test("a worker exiting right after closing ref'd ports shuts down cleanly", async () => { + const w = new Worker( + `const { parentPort, MessageChannel } = require("worker_threads"); + const { port1, port2 } = new MessageChannel(); + port1.on("message", () => {}); + port2.ref(); + parentPort.on("message", () => {}); + port1.close(); + port2.close(); + parentPort.close(); + process.exit(parentPort.hasRef() && port1.hasRef() && port2.hasRef() ? 42 : 1);`, + { eval: true }, + ); + const exited = new Promise(resolve => w.on("exit", resolve)); + expect(await exited).toBe(42); +}); + +// The same close-then-exit sequence on the main thread, checked for leaks: onmessage= +// takes a self-ref on the native port that only its 'close' task or the teardown +// releases, so a port closed right before exit leaks outright if teardown skips it. +// Malloc=1 routes WebKit allocations through the system allocator so LSan can see the +// port. Linux is where that combination is verified clean (CI's ASAN lane); keep the +// fixture on the main thread, since every exited worker thread leaves its thread_local +// eventNames() table behind under Malloc=1, which LSan would report here. +test.skipIf(!isASAN || !isLinux)("ports closed right before exit are released by the VM teardown", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `const { MessageChannel } = require("worker_threads"); + for (let i = 0; i < 50; i++) { + const { port1, port2 } = new MessageChannel(); + port1.onmessage = () => {}; + port2.on("message", () => {}); + port1.close(); + port2.close(); + } + process.exit(0);`, + ], + env: { + ...bunEnv, + BUN_DESTRUCT_VM_ON_EXIT: "1", + Malloc: "1", + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"), + LSAN_OPTIONS: `print_suppressions=0:suppressions=${join(import.meta.dirname, "../../../leaksan.supp")}`, + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ stdout: "", stderr: "", exitCode: 0 }); +}); + // In a node worker only parentPort receives what the parent posts; the global // scope's `self.onmessage` is not a channel there (as in node). Libraries that // install both a parentPort listener and self.onmessage as a node/web shim @@ -997,6 +1174,41 @@ test("hasRef() survives collection of the unreferenced peer", () => { port1.close(); }); +// A collected peer does close the channel (bun collects entangled ports; node +// never does), and once this side's 'close' has fired, ref() must stay a no-op: +// a re-taken loop ref is never released. Spawned: the symptom is "the process +// never exits". +test("ref() inside 'close' after the peer is collected does not pin the loop", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `const { MessageChannel } = require("worker_threads"); + function setup() { + const { port1 } = new MessageChannel(); // port2 unreachable from birth + port1.on("message", () => {}); + return port1; + } + const port1 = setup(); + const pump = setInterval(() => Bun.gc(true), 10); + port1.on("close", () => { + clearInterval(pump); + port1.ref(); + console.log("refInClose:" + port1.hasRef()); + });`, + ], + env: bunEnv, + stderr: "pipe", + }); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // signalCode null => it exited on its own rather than being killed. + expect({ stdout: stdout.trim(), exitCode, signalCode: proc.signalCode }).toEqual({ + stdout: "refInClose:false", + exitCode: 0, + signalCode: null, + }); +}); + // markAsUncloneable blocks *cloning*, not transfer: a marked port in the transfer // list is moved, so node lets it through and it still works on the far side. test("markAsUncloneable blocks cloning a port but not transferring it", async () => { @@ -1928,6 +2140,30 @@ test("parentPort.unref() lets a listening worker exit", async () => { expect(await exited).toBe(0); }); +// Same close() timing as any other port (node): hasRef() is still true right after +// parentPort.close() and false once its 'close' event runs. Results travel over a +// separate port because parentPort itself is the one being closed. +test("parentPort.hasRef() stays true after parentPort.close() until its 'close' event", async () => { + const { port1, port2 } = new MessageChannel(); + const { promise, resolve } = Promise.withResolvers(); + port1.on("message", resolve); + const w = new Worker( + `const { parentPort, workerData } = require("worker_threads"); + parentPort.on("message", () => {}); + parentPort.on("close", () => { + workerData.report.postMessage({ afterClose, inCloseEvent: parentPort.hasRef() }); + workerData.report.close(); + }); + parentPort.close(); + const afterClose = parentPort.hasRef();`, + { eval: true, workerData: { report: port2 }, transferList: [port2] }, + ); + const exited = new Promise(resolve => w.on("exit", resolve)); + expect(await promise).toEqual({ afterClose: true, inCloseEvent: false }); + expect(await exited).toBe(0); + port1.close(); +}); + test("receiveMessageOnPort distinguishes an undefined message from an empty queue", () => { const { port1, port2 } = new MessageChannel(); port1.postMessage(undefined);