Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 54 additions & 24 deletions src/bun.js/bindings/webcore/WebSocket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,58 @@ 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 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);
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& 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 {
// 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();
Comment thread
robobun marked this conversation as resolved.
});
} else {
m_state = CLOSED;
disablePendingActivity();
}
updateHasPendingActivity();
}

ExceptionOr<void> WebSocket::close(std::optional<unsigned short> optionalCode, const String& reason)
{
int code = optionalCode ? optionalCode.value() : static_cast<int>(1000);
Expand All @@ -938,18 +990,7 @@ ExceptionOr<void> WebSocket::close(std::optional<unsigned short> 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;
Expand Down Expand Up @@ -994,18 +1035,7 @@ ExceptionOr<void> 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;
Expand Down
1 change: 1 addition & 0 deletions src/bun.js/bindings/webcore/WebSocket.h
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ class WebSocket final : public RefCounted<WebSocket>, 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);
Expand Down
90 changes: 90 additions & 0 deletions test/js/web/websocket/websocket-close-connecting.test.ts
Original file line number Diff line number Diff line change
@@ -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 { 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 => {
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<void>();
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<CloseEvent>(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<void>();
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<unknown>[] = [];
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();
}
});
});
Loading