Skip to content

fix(node:http): socket.write not sending data in upgrade event handler #27237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(node:http): socket.write not sending data in upgrade event handler #27237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5a2571
fix(node:http): socket.write not sending data in upgrade event handler
Feb 20, 2026
15dfc70
Merge branch 'main' into claude/fix-http-upgrade-socket-write
Jarred-Sumner Mar 16, 2026
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
5 changes: 4 additions & 1 deletion src/js/node/_http_server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,7 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort
http_res.end();
socket.destroy();
} else if (is_upgrade) {
socket[kEnableStreaming](true);
server.emit("upgrade", http_req, socket, kEmptyBuffer);
if (!socket._httpMessage) {
if (canUseInternalAssignSocket) {
Comment on lines 608 to 612

@claude claude Bot Mar 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing: The upgrade handler (line 608) is missing a listenerCount("upgrade") guard that the CONNECT handler (line 538) already has — upgrade requests with no listeners will now enable streaming on an unmanaged socket and leak the internal promise. Additionally, assignSocketInternal at line 614 stores the close callback via setCloseCallback(socket, ...), but NodeHTTPServerSocket never calls callCloseCallback(this) on close (unlike ServerResponse.prototype.emit), so the internal promise returned from onNodeHTTPRequest hangs forever for upgrade connections. The CONNECT handler avoids both issues by checking listenerCount and using socket.once("close", resolve) directly.

Extended reasoning...

Bug 1: Missing listenerCount check

The CONNECT handler at line 538 checks server.listenerCount("connect") > 0 before enabling streaming and emitting the event. If no listeners exist, it closes the socket handle. The upgrade handler at line 608 has no equivalent guard — it unconditionally calls socket[kEnableStreaming](true) and emits the "upgrade" event regardless of whether any listeners are registered.

In Node.js's reference implementation (_http_server.js), when there are no upgrade listeners, socket.destroy() is called to clean up the connection. Without this guard in Bun, upgrade requests to a server with no "upgrade" listener will set up ondata/ondrain handlers on a socket that nobody is managing, and the internal promise will hang forever.

Before this PR, the no-listener case was relatively inert because streaming was disabled (writes silently failed). After this PR, socket[kEnableStreaming](true) at line 609 is called unconditionally, making the leak slightly worse by activating event handlers on the orphaned socket.

Bug 2: Internal promise never resolves for upgrade connections

The promise lifecycle for upgrade connections is broken. Here's the intended chain:

  1. setCloseCallback(http_res, onClose) at line 602 stores the promise-resolving onClose as http_res[kCloseCallback]
  2. assignSocketInternal(http_res, socket) at line 614 stores onServerResponseClose as socket[kCloseCallback]
  3. When the socket closes, callCloseCallback(socket) should fire → onServerResponseClose → emitCloseNT(http_res) → callCloseCallback(http_res) → onClose() → resolve

However, this chain is broken because NodeHTTPServerSocket extends Duplex and does not override emit() to intercept the "close" event. Only ServerResponse.prototype.emit (line 1631) calls callCloseCallback(this) when it sees a "close" event. When the socket's #onClose() method fires (line 899), it does not invoke callCloseCallback(socket), so onServerResponseClose is never called.

For normal requests this doesn't matter because resolution goes through http_res.end() → http_res.emit("close") → callCloseCallback(http_res) → onClose(). But for upgrade connections, user code works with the raw socket and never calls http_res.end().

Proof by comparison with CONNECT handler

The CONNECT handler at lines 540-544 correctly handles both issues:

socket[kEnableStreaming](true);
const { promise, resolve } = $newPromiseCapability(Promise);
socket.once("close", resolve);  // Direct binding — no broken callback chain
server.emit("connect", http_req, socket, head);
return promise;

It checks listenerCount first, and ties promise resolution directly to the socket's "close" event via .once(), bypassing the assignSocketInternal/callCloseCallback mechanism entirely.

Impact

Both issues are pre-existing (the upgrade branch with assignSocketInternal and without listenerCount existed before this PR). However, this PR makes upgrade connections actually functional (writes now work), which means more users will exercise this code path and encounter these issues. The fix should mirror the CONNECT handler: guard with listenerCount("upgrade") > 0, call socket.destroy() when there are no listeners, and use socket.once("close", resolve) for promise resolution instead of relying on assignSocketInternal.

Expand Down Expand Up @@ -633,7 +634,9 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort
server.emit("request", http_req, http_res);
}

socket.cork();
if (!is_upgrade) {
socket.cork();
}

if (handle.finished || didFinish) {
handle = undefined;
Expand Down
162 changes: 162 additions & 0 deletions test/regression/issue/09882.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import { expect, test } from "bun:test";
import http from "node:http";
import net from "node:net";

test("socket.write sends data in http upgrade event handler", async () => {
const { promise, resolve, reject } = Promise.withResolvers<void>();

const server = http.createServer();
server.on("upgrade", (_req, socket) => {
socket.write("x", () => {
// After the write completes, close the socket
socket.end();
});
});

server.listen(0, "127.0.0.1", () => {
const addr = server.address() as net.AddressInfo;

const client = net.createConnection(addr.port, "127.0.0.1", () => {
client.write(
"GET / HTTP/1.1\r\n" +
"Host: 127.0.0.1\r\n" +
"Upgrade: websocket\r\n" +
"Connection: Upgrade\r\n" +
"Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n" +
"Sec-WebSocket-Version: 13\r\n" +
"\r\n",
);
});

let received = "";
client.on("data", (data: Buffer) => {
received += data.toString();
});

client.on("end", () => {
try {
expect(received).toBe("x");
resolve();
} catch (e) {
reject(e);
} finally {
server.close();
}
});

client.on("error", (err: Error) => {
server.close();
reject(err);
});
});

await promise;
});

test("socket.write with 101 handshake in http upgrade event handler", async () => {
const { promise, resolve, reject } = Promise.withResolvers<void>();

const server = http.createServer();
server.on("upgrade", (_req, socket) => {
socket.write(
"HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n\r\nhello from upgrade",
() => {
socket.end();
},
);
});

server.listen(0, "127.0.0.1", () => {
const addr = server.address() as net.AddressInfo;

const client = net.createConnection(addr.port, "127.0.0.1", () => {
client.write(
"GET / HTTP/1.1\r\n" +
"Host: 127.0.0.1\r\n" +
"Upgrade: websocket\r\n" +
"Connection: Upgrade\r\n" +
"Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n" +
"Sec-WebSocket-Version: 13\r\n" +
"\r\n",
);
});

let received = "";
client.on("data", (data: Buffer) => {
received += data.toString();
});

client.on("end", () => {
try {
expect(received).toContain("HTTP/1.1 101 Switching Protocols");
expect(received).toContain("hello from upgrade");
resolve();
} catch (e) {
reject(e);
} finally {
server.close();
}
});

client.on("error", (err: Error) => {
server.close();
reject(err);
});
});

await promise;
});

test("multiple socket.write calls in http upgrade event handler", async () => {
const { promise, resolve, reject } = Promise.withResolvers<void>();

const server = http.createServer();
server.on("upgrade", (_req, socket) => {
socket.write("first");
socket.write("second");
socket.write("third", () => {
socket.end();
});
});

server.listen(0, "127.0.0.1", () => {
const addr = server.address() as net.AddressInfo;

const client = net.createConnection(addr.port, "127.0.0.1", () => {
client.write(
"GET / HTTP/1.1\r\n" +
"Host: 127.0.0.1\r\n" +
"Upgrade: websocket\r\n" +
"Connection: Upgrade\r\n" +
"Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n" +
"Sec-WebSocket-Version: 13\r\n" +
"\r\n",
);
});

let received = "";
client.on("data", (data: Buffer) => {
received += data.toString();
});

client.on("end", () => {
try {
expect(received).toContain("first");
expect(received).toContain("second");
expect(received).toContain("third");
resolve();
} catch (e) {
reject(e);
} finally {
server.close();
}
});

client.on("error", (err: Error) => {
server.close();
reject(err);
});
});

await promise;
});
Loading