Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
72 changes: 48 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,52 @@
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();

Check warning on line 960 in src/bun.js/bindings/webcore/WebSocket.cpp

View check run for this annotation

Claude / Claude Code Review

Missing error event before close event when failing CONNECTING WebSocket

Per the WHATWG WebSocket spec, calling `close()` while `readyState === CONNECTING` must "fail the WebSocket connection", which requires firing an `error` event before the `close` event — Chrome, Firefox, and the npm `ws` package all emit `error`→`close` here. `failConnectingWebSocket()` only dispatches the `CloseEvent`, and the new test asserts `errors: 0`, baking this divergence into the suite. Not blocking (the leak fix is a strict improvement over firing nothing), but consider dispatching an
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 +984,7 @@
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 +1029,7 @@
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
85 changes: 85 additions & 0 deletions test/js/web/websocket/websocket.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,91 @@
}
});

// 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,

Check warning on line 891 in test/js/web/websocket/websocket.test.js

View check run for this annotation

Claude / Claude Code Review

Explicit test timeout violates test/CLAUDE.md convention

nit: per `test/CLAUDE.md` ("**CRITICAL**: Do not set a timeout on tests. Bun already has timeouts."), drop the trailing `20000` argument from this `it.serial` — the neighboring `instances should be finalized when GC'd` test does ~15× more work without one.
Comment thread
robobun marked this conversation as resolved.
Outdated
);
});

it.serial("instances should be finalized when GC'd", async () => {
let current_websocket_count = 0;
let initial_websocket_count = 0;
Expand Down
Loading