From c2e809827d7deb192f8d7afa5523e9efe8d32b7d Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 29 Apr 2026 01:27:09 +0000 Subject: [PATCH 1/4] WebSocket: release pending-activity ref when close()/terminate() called during CONNECTING MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit connect() takes a pending-activity ref that is only released by didConnect / didFailWithErrorCode / didClose. When close() or terminate() is called while still CONNECTING, the Zig upgrade client's cancel() clears its back-pointer without calling any of those, so the WebSocket stayed in CLOSING with m_pendingActivityCount > 0 forever — hasPendingActivity() permanently true, never GC'd, and no close event fired. failConnectingWebSocket() now cancels the upgrade and queues a task that transitions to CLOSED, fires the close event (code 1006, wasClean=false — matching browsers/Node ws), and calls disablePendingActivity(). --- src/bun.js/bindings/webcore/WebSocket.cpp | 72 ++++++++++++------- src/bun.js/bindings/webcore/WebSocket.h | 1 + test/js/web/websocket/websocket.test.js | 85 +++++++++++++++++++++++ 3 files changed, 134 insertions(+), 24 deletions(-) diff --git a/src/bun.js/bindings/webcore/WebSocket.cpp b/src/bun.js/bindings/webcore/WebSocket.cpp index 4a3bb45ff25..677559591f6 100644 --- a/src/bun.js/bindings/webcore/WebSocket.cpp +++ b/src/bun.js/bindings/webcore/WebSocket.cpp @@ -920,6 +920,52 @@ void WebSocket::sendWebSocketString(const String& message, const Opcode op) updateHasPendingActivity(); } +// Called from close()/terminate() while m_state == CONNECTING. +// +// The Zig-side upgrade client's cancel() clears its back-pointer to us +// without calling didAbruptClose, so none of didConnect / +// didFailWithErrorCode / didClose will ever fire for this socket. We +// must therefore finish the close ourselves: cancel the upgrade, queue +// a task that moves to CLOSED, fires the close event (code 1006 per +// spec — connection never established), and releases the +// pending-activity ref taken in connect(). Without this the WebSocket +// stays in CLOSING with m_pendingActivityCount > 0 forever and is +// never garbage-collected. +void WebSocket::failConnectingWebSocket() +{ + ASSERT(m_state == CONNECTING); + m_state = CLOSING; + + if (m_upgradeClient != nullptr) { + void* upgradeClient = m_upgradeClient; + m_upgradeClient = nullptr; + bool useTLSClient = (m_connectionType == ConnectionType::TLS || m_connectionType == ConnectionType::ProxyTLS); + if (useTLSClient) { + Bun__WebSocketHTTPSClient__cancel(upgradeClient); + } else { + Bun__WebSocketHTTPClient__cancel(upgradeClient); + } + } + + if (auto* context = scriptExecutionContext()) { + context->postTask([protectedThis = Ref { *this }](ScriptExecutionContext&) { + if (protectedThis->m_state == CLOSED) + return; + protectedThis->m_state = CLOSED; + if (protectedThis->m_native.onClose) { + protectedThis->m_native.onClose(protectedThis->m_native.ctx, 1006); + } else { + protectedThis->dispatchEvent(CloseEvent::create(false, 1006, emptyString())); + } + protectedThis->disablePendingActivity(); + }); + } else { + m_state = CLOSED; + disablePendingActivity(); + } + updateHasPendingActivity(); +} + ExceptionOr WebSocket::close(std::optional optionalCode, const String& reason) { int code = optionalCode ? optionalCode.value() : static_cast(1000); @@ -938,18 +984,7 @@ ExceptionOr WebSocket::close(std::optional optionalCode, c if (m_state == CLOSING || m_state == CLOSED) return {}; if (m_state == CONNECTING) { - m_state = CLOSING; - if (m_upgradeClient != nullptr) { - void* upgradeClient = m_upgradeClient; - m_upgradeClient = nullptr; - bool useTLSClient = (m_connectionType == ConnectionType::TLS || m_connectionType == ConnectionType::ProxyTLS); - if (useTLSClient) { - Bun__WebSocketHTTPSClient__cancel(upgradeClient); - } else { - Bun__WebSocketHTTPClient__cancel(upgradeClient); - } - } - updateHasPendingActivity(); + failConnectingWebSocket(); return {}; } m_state = CLOSING; @@ -994,18 +1029,7 @@ ExceptionOr WebSocket::terminate() if (m_state == CLOSING || m_state == CLOSED) return {}; if (m_state == CONNECTING) { - m_state = CLOSING; - if (m_upgradeClient != nullptr) { - void* upgradeClient = m_upgradeClient; - m_upgradeClient = nullptr; - bool useTLSClient = (m_connectionType == ConnectionType::TLS || m_connectionType == ConnectionType::ProxyTLS); - if (useTLSClient) { - Bun__WebSocketHTTPSClient__cancel(upgradeClient); - } else { - Bun__WebSocketHTTPClient__cancel(upgradeClient); - } - } - updateHasPendingActivity(); + failConnectingWebSocket(); return {}; } m_state = CLOSING; diff --git a/src/bun.js/bindings/webcore/WebSocket.h b/src/bun.js/bindings/webcore/WebSocket.h index 9d9077685a9..f9cde8b5396 100644 --- a/src/bun.js/bindings/webcore/WebSocket.h +++ b/src/bun.js/bindings/webcore/WebSocket.h @@ -271,6 +271,7 @@ class WebSocket final : public RefCounted, public EventTargetWithInli void didReceiveClose(CleanStatus wasClean, unsigned short code, WTF::String reason, bool isConnectionError = false); void didUpdateBufferedAmount(unsigned bufferedAmount); void didStartClosingHandshake(); + void failConnectingWebSocket(); void sendWebSocketString(const String& message, const Opcode opcode); void sendWebSocketData(const char* data, size_t length, const Opcode opcode); diff --git a/test/js/web/websocket/websocket.test.js b/test/js/web/websocket/websocket.test.js index 7caee1a2795..fabcc606adf 100644 --- a/test/js/web/websocket/websocket.test.js +++ b/test/js/web/websocket/websocket.test.js @@ -807,6 +807,91 @@ it.concurrent("#16995", async () => { } }); +// close()/terminate() during CONNECTING must transition to CLOSED, fire a +// close event, and release the pending-activity ref so the WebSocket can be +// garbage collected. Previously the upgrade client was cancelled without any +// of the completion callbacks firing, leaving the socket stuck in CLOSING +// with hasPendingActivity() permanently true. +describe.each(["close", "terminate"])("%s() during CONNECTING", method => { + it("fires close event and transitions to CLOSED", async () => { + // TCP server that accepts connections but never responds — keeps the + // WebSocket in CONNECTING until we abort it. + const listening = Promise.withResolvers(); + const server = createServer(() => {}).listen(0, "127.0.0.1", listening.resolve); + await listening.promise; + try { + const port = server.address().port; + const ws = new WebSocket(`ws://127.0.0.1:${port}`); + expect(ws.readyState).toBe(WebSocket.CONNECTING); + + const errors = []; + ws.onerror = e => errors.push(e); + const closed = new Promise(resolve => { + ws.onclose = resolve; + }); + + ws[method](); + expect(ws.readyState).toBe(WebSocket.CLOSING); + + const ev = await closed; + expect({ + readyState: ws.readyState, + code: ev.code, + wasClean: ev.wasClean, + errors: errors.length, + }).toEqual({ + readyState: WebSocket.CLOSED, + code: 1006, + wasClean: false, + errors: 0, + }); + } finally { + server.close(); + } + }); + + it.serial( + "does not leak", + async () => { + function getWebSocketCount() { + Bun.gc(true); + return require("bun:jsc").heapStats().objectTypeCounts?.WebSocket || 0; + } + + const listening = Promise.withResolvers(); + const server = createServer(() => {}).listen(0, "127.0.0.1", listening.resolve); + await listening.promise; + try { + const port = server.address().port; + const before = getWebSocketCount(); + + const closes = []; + for (let i = 0; i < 64; i++) { + const ws = new WebSocket(`ws://127.0.0.1:${port}`); + closes.push(new Promise(resolve => (ws.onclose = resolve))); + ws[method](); + } + await Promise.all(closes); + + // disablePendingActivity is posted as a separate task after the close + // event fires; drain those plus any pending socket-close callbacks + // before counting. Without the fix, all 64 instances remain alive. + let after; + for (let i = 0; i < 10; i++) { + await new Promise(resolve => setImmediate(resolve)); + after = getWebSocketCount(); + if (after - before <= 5) break; + } + + expect(after - before).toBeLessThanOrEqual(5); + } finally { + server.close(); + } + }, + 20000, + ); +}); + it.serial("instances should be finalized when GC'd", async () => { let current_websocket_count = 0; let initial_websocket_count = 0; From 3ec2a845adab1f2a49c7ae1ac75d390aa43b90bc Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 29 Apr 2026 02:31:10 +0000 Subject: [PATCH 2/4] Dispatch error event before close; drop explicit test timeout Per the WHATWG WebSocket spec, close() while CONNECTING runs 'fail the WebSocket connection', which requires an error event before the close event. Mirrors the wasConnecting && isConnectionError branch in didReceiveClose(). Matches Chrome/Firefox and npm ws. --- src/bun.js/bindings/webcore/WebSocket.cpp | 20 ++++-- test/js/web/websocket/websocket.test.js | 86 +++++++++++------------ 2 files changed, 56 insertions(+), 50 deletions(-) diff --git a/src/bun.js/bindings/webcore/WebSocket.cpp b/src/bun.js/bindings/webcore/WebSocket.cpp index 677559591f6..778e3343cc8 100644 --- a/src/bun.js/bindings/webcore/WebSocket.cpp +++ b/src/bun.js/bindings/webcore/WebSocket.cpp @@ -926,11 +926,11 @@ void WebSocket::sendWebSocketString(const String& message, const Opcode op) // without calling didAbruptClose, so none of didConnect / // didFailWithErrorCode / didClose will ever fire for this socket. We // must therefore finish the close ourselves: cancel the upgrade, queue -// a task that moves to CLOSED, fires the close event (code 1006 per -// spec — connection never established), and releases the -// pending-activity ref taken in connect(). Without this the WebSocket -// stays in CLOSING with m_pendingActivityCount > 0 forever and is -// never garbage-collected. +// a task that moves to CLOSED, fires error + close events (spec "fail +// the WebSocket connection" path — code 1006, wasClean false), and +// releases the pending-activity ref taken in connect(). Without this +// the WebSocket stays in CLOSING with m_pendingActivityCount > 0 +// forever and is never garbage-collected. void WebSocket::failConnectingWebSocket() { ASSERT(m_state == CONNECTING); @@ -948,14 +948,20 @@ void WebSocket::failConnectingWebSocket() } if (auto* context = scriptExecutionContext()) { - context->postTask([protectedThis = Ref { *this }](ScriptExecutionContext&) { + context->postTask([protectedThis = Ref { *this }](ScriptExecutionContext& context) { if (protectedThis->m_state == CLOSED) return; protectedThis->m_state = CLOSED; if (protectedThis->m_native.onClose) { protectedThis->m_native.onClose(protectedThis->m_native.ctx, 1006); } else { - protectedThis->dispatchEvent(CloseEvent::create(false, 1006, emptyString())); + // Spec: close() while CONNECTING runs "fail the WebSocket + // connection", which requires an error event before the + // close event. Matches Chrome/Firefox and npm ws. + auto reason = "WebSocket is closed before the connection is established"_s; + auto eventInit = createErrorEventInit(protectedThis, reason, context.jsGlobalObject()); + protectedThis->dispatchEvent(ErrorEvent::create(eventNames().errorEvent, WTF::move(eventInit), EventIsTrusted::Yes)); + protectedThis->dispatchEvent(CloseEvent::create(false, 1006, reason)); } protectedThis->disablePendingActivity(); }); diff --git a/test/js/web/websocket/websocket.test.js b/test/js/web/websocket/websocket.test.js index fabcc606adf..3270c63ceb4 100644 --- a/test/js/web/websocket/websocket.test.js +++ b/test/js/web/websocket/websocket.test.js @@ -813,7 +813,7 @@ it.concurrent("#16995", async () => { // of the completion callbacks firing, leaving the socket stuck in CLOSING // with hasPendingActivity() permanently true. describe.each(["close", "terminate"])("%s() during CONNECTING", method => { - it("fires close event and transitions to CLOSED", async () => { + it("fires error + close events and transitions to CLOSED", async () => { // TCP server that accepts connections but never responds — keeps the // WebSocket in CONNECTING until we abort it. const listening = Promise.withResolvers(); @@ -824,72 +824,72 @@ describe.each(["close", "terminate"])("%s() during CONNECTING", method => { const ws = new WebSocket(`ws://127.0.0.1:${port}`); expect(ws.readyState).toBe(WebSocket.CONNECTING); - const errors = []; - ws.onerror = e => errors.push(e); + const events = []; + ws.onerror = e => events.push({ type: e.type, message: e.message }); const closed = new Promise(resolve => { - ws.onclose = resolve; + ws.onclose = e => { + events.push({ type: e.type }); + resolve(e); + }; }); ws[method](); expect(ws.readyState).toBe(WebSocket.CLOSING); const ev = await closed; + // Spec "fail the WebSocket connection": error event then close event. + expect(events.map(e => e.type)).toEqual(["error", "close"]); + expect(events[0].message).toContain("closed before the connection is established"); expect({ readyState: ws.readyState, code: ev.code, wasClean: ev.wasClean, - errors: errors.length, }).toEqual({ readyState: WebSocket.CLOSED, code: 1006, wasClean: false, - errors: 0, }); } finally { server.close(); } }); - it.serial( - "does not leak", - async () => { - function getWebSocketCount() { - Bun.gc(true); - return require("bun:jsc").heapStats().objectTypeCounts?.WebSocket || 0; - } + it.serial("does not leak", async () => { + function getWebSocketCount() { + Bun.gc(true); + return require("bun:jsc").heapStats().objectTypeCounts?.WebSocket || 0; + } - const listening = Promise.withResolvers(); - const server = createServer(() => {}).listen(0, "127.0.0.1", listening.resolve); - await listening.promise; - try { - const port = server.address().port; - const before = getWebSocketCount(); - - const closes = []; - for (let i = 0; i < 64; i++) { - const ws = new WebSocket(`ws://127.0.0.1:${port}`); - closes.push(new Promise(resolve => (ws.onclose = resolve))); - ws[method](); - } - await Promise.all(closes); - - // disablePendingActivity is posted as a separate task after the close - // event fires; drain those plus any pending socket-close callbacks - // before counting. Without the fix, all 64 instances remain alive. - let after; - for (let i = 0; i < 10; i++) { - await new Promise(resolve => setImmediate(resolve)); - after = getWebSocketCount(); - if (after - before <= 5) break; - } + const listening = Promise.withResolvers(); + const server = createServer(() => {}).listen(0, "127.0.0.1", listening.resolve); + await listening.promise; + try { + const port = server.address().port; + const before = getWebSocketCount(); - expect(after - before).toBeLessThanOrEqual(5); - } finally { - server.close(); + const closes = []; + for (let i = 0; i < 64; i++) { + const ws = new WebSocket(`ws://127.0.0.1:${port}`); + closes.push(new Promise(resolve => (ws.onclose = resolve))); + ws[method](); + } + await Promise.all(closes); + + // disablePendingActivity is posted as a separate task after the close + // event fires; drain those plus any pending socket-close callbacks + // before counting. Without the fix, all 64 instances remain alive. + let after; + for (let i = 0; i < 10; i++) { + await new Promise(resolve => setImmediate(resolve)); + after = getWebSocketCount(); + if (after - before <= 5) break; } - }, - 20000, - ); + + expect(after - before).toBeLessThanOrEqual(5); + } finally { + server.close(); + } + }); }); it.serial("instances should be finalized when GC'd", async () => { From 1e449314913848190f7e08cac29ef35bc91ccb01 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 29 Apr 2026 03:16:20 +0000 Subject: [PATCH 3/4] Move close-during-CONNECTING tests to their own file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit websocket.test.js has several pre-existing tests (300× TLS handshakes, 1000× connect+close, external echo host) that run near the 5s default timeout under debug+ASAN regardless of this change. Keeping the new tests in a dedicated file — consistent with the other per-topic test files in this directory — lets them be evaluated without those unrelated borderline tests. --- .../websocket-close-connecting.test.ts | 90 +++++++++++++++++++ test/js/web/websocket/websocket.test.js | 85 ------------------ 2 files changed, 90 insertions(+), 85 deletions(-) create mode 100644 test/js/web/websocket/websocket-close-connecting.test.ts diff --git a/test/js/web/websocket/websocket-close-connecting.test.ts b/test/js/web/websocket/websocket-close-connecting.test.ts new file mode 100644 index 00000000000..bdeee031c06 --- /dev/null +++ b/test/js/web/websocket/websocket-close-connecting.test.ts @@ -0,0 +1,90 @@ +// close()/terminate() during CONNECTING must transition to CLOSED, fire +// error + close events, and release the pending-activity ref so the +// WebSocket can be garbage-collected. Previously the Zig upgrade client +// was cancelled without any completion callback reaching C++, leaving +// the socket stuck in CLOSING with hasPendingActivity() permanently true +// — never GC'd, close event never fired, process never exited. + +import { describe, expect, it } from "bun:test"; +import { heapStats } from "bun:jsc"; +import { createServer, type AddressInfo } from "net"; + +describe.each(["close", "terminate"] as const)("%s() during CONNECTING", method => { + it("fires error + close events and transitions to CLOSED", async () => { + // TCP server that accepts connections but never responds — keeps the + // WebSocket in CONNECTING until we abort it. + const listening = Promise.withResolvers(); + const server = createServer(() => {}).listen(0, "127.0.0.1", listening.resolve); + await listening.promise; + try { + const port = (server.address() as AddressInfo).port; + const ws = new WebSocket(`ws://127.0.0.1:${port}`); + expect(ws.readyState).toBe(WebSocket.CONNECTING); + + const events: { type: string; message?: string }[] = []; + ws.onerror = e => events.push({ type: e.type, message: (e as ErrorEvent).message }); + const closed = new Promise(resolve => { + ws.onclose = e => { + events.push({ type: e.type }); + resolve(e); + }; + }); + + ws[method](); + expect(ws.readyState).toBe(WebSocket.CLOSING); + + const ev = await closed; + // Spec "fail the WebSocket connection": error event then close event. + expect(events.map(e => e.type)).toEqual(["error", "close"]); + expect(events[0].message).toContain("closed before the connection is established"); + expect({ + readyState: ws.readyState, + code: ev.code, + wasClean: ev.wasClean, + }).toEqual({ + readyState: WebSocket.CLOSED, + code: 1006, + wasClean: false, + }); + } finally { + server.close(); + } + }); + + it("does not leak", async () => { + function getWebSocketCount() { + Bun.gc(true); + return heapStats().objectTypeCounts?.WebSocket || 0; + } + + const listening = Promise.withResolvers(); + const server = createServer(() => {}).listen(0, "127.0.0.1", listening.resolve); + await listening.promise; + try { + const port = (server.address() as AddressInfo).port; + const before = getWebSocketCount(); + + const closes: Promise[] = []; + for (let i = 0; i < 64; i++) { + const ws = new WebSocket(`ws://127.0.0.1:${port}`); + closes.push(new Promise(resolve => (ws.onclose = resolve))); + ws[method](); + } + await Promise.all(closes); + + // disablePendingActivity is posted as a separate task after the close + // event fires; drain those plus any pending socket-close callbacks + // before counting. Without the fix, all 64 instances remain alive. + let after: number; + for (let i = 0; i < 10; i++) { + await new Promise(resolve => setImmediate(resolve)); + after = getWebSocketCount(); + if (after - before <= 5) break; + } + + expect(after! - before).toBeLessThanOrEqual(5); + } finally { + server.close(); + } + }); +}); diff --git a/test/js/web/websocket/websocket.test.js b/test/js/web/websocket/websocket.test.js index 3270c63ceb4..7caee1a2795 100644 --- a/test/js/web/websocket/websocket.test.js +++ b/test/js/web/websocket/websocket.test.js @@ -807,91 +807,6 @@ it.concurrent("#16995", async () => { } }); -// close()/terminate() during CONNECTING must transition to CLOSED, fire a -// close event, and release the pending-activity ref so the WebSocket can be -// garbage collected. Previously the upgrade client was cancelled without any -// of the completion callbacks firing, leaving the socket stuck in CLOSING -// with hasPendingActivity() permanently true. -describe.each(["close", "terminate"])("%s() during CONNECTING", method => { - it("fires error + close events and transitions to CLOSED", async () => { - // TCP server that accepts connections but never responds — keeps the - // WebSocket in CONNECTING until we abort it. - const listening = Promise.withResolvers(); - const server = createServer(() => {}).listen(0, "127.0.0.1", listening.resolve); - await listening.promise; - try { - const port = server.address().port; - const ws = new WebSocket(`ws://127.0.0.1:${port}`); - expect(ws.readyState).toBe(WebSocket.CONNECTING); - - const events = []; - ws.onerror = e => events.push({ type: e.type, message: e.message }); - const closed = new Promise(resolve => { - ws.onclose = e => { - events.push({ type: e.type }); - resolve(e); - }; - }); - - ws[method](); - expect(ws.readyState).toBe(WebSocket.CLOSING); - - const ev = await closed; - // Spec "fail the WebSocket connection": error event then close event. - expect(events.map(e => e.type)).toEqual(["error", "close"]); - expect(events[0].message).toContain("closed before the connection is established"); - expect({ - readyState: ws.readyState, - code: ev.code, - wasClean: ev.wasClean, - }).toEqual({ - readyState: WebSocket.CLOSED, - code: 1006, - wasClean: false, - }); - } finally { - server.close(); - } - }); - - it.serial("does not leak", async () => { - function getWebSocketCount() { - Bun.gc(true); - return require("bun:jsc").heapStats().objectTypeCounts?.WebSocket || 0; - } - - const listening = Promise.withResolvers(); - const server = createServer(() => {}).listen(0, "127.0.0.1", listening.resolve); - await listening.promise; - try { - const port = server.address().port; - const before = getWebSocketCount(); - - const closes = []; - for (let i = 0; i < 64; i++) { - const ws = new WebSocket(`ws://127.0.0.1:${port}`); - closes.push(new Promise(resolve => (ws.onclose = resolve))); - ws[method](); - } - await Promise.all(closes); - - // disablePendingActivity is posted as a separate task after the close - // event fires; drain those plus any pending socket-close callbacks - // before counting. Without the fix, all 64 instances remain alive. - let after; - for (let i = 0; i < 10; i++) { - await new Promise(resolve => setImmediate(resolve)); - after = getWebSocketCount(); - if (after - before <= 5) break; - } - - expect(after - before).toBeLessThanOrEqual(5); - } finally { - server.close(); - } - }); -}); - it.serial("instances should be finalized when GC'd", async () => { let current_websocket_count = 0; let initial_websocket_count = 0; From 81bd2a9581f0f4443fe02765e81e7821f3487c2e Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Wed, 29 Apr 2026 03:18:07 +0000 Subject: [PATCH 4/4] [autofix.ci] apply automated fixes --- test/js/web/websocket/websocket-close-connecting.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/web/websocket/websocket-close-connecting.test.ts b/test/js/web/websocket/websocket-close-connecting.test.ts index bdeee031c06..f4dc7dfcf62 100644 --- a/test/js/web/websocket/websocket-close-connecting.test.ts +++ b/test/js/web/websocket/websocket-close-connecting.test.ts @@ -5,8 +5,8 @@ // the socket stuck in CLOSING with hasPendingActivity() permanently true // — never GC'd, close event never fired, process never exited. -import { describe, expect, it } from "bun:test"; import { heapStats } from "bun:jsc"; +import { describe, expect, it } from "bun:test"; import { createServer, type AddressInfo } from "net"; describe.each(["close", "terminate"] as const)("%s() during CONNECTING", method => {