-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(node:http): hand off upgrade socket to userland for bidirectional communication #28158
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -606,15 +606,16 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort | |
| http_res.end(); | ||
| socket.destroy(); | ||
| } else if (is_upgrade) { | ||
| // Hand off the connection to userland, mirroring CONNECT handler. | ||
| // Enable streaming so socket.write() and socket.on("data") work. | ||
| socket[kEnableStreaming](true); | ||
| // Tell uWebSockets to stop parsing HTTP on this connection, | ||
| // switching to raw pass-through mode for bidirectional data. | ||
| socketHandle.markAsRawMode(); | ||
| const { promise: upgradePromise, resolve: upgradeResolve } = $newPromiseCapability(Promise); | ||
| socket.once("close", upgradeResolve); | ||
| server.emit("upgrade", http_req, socket, kEmptyBuffer); | ||
| if (!socket._httpMessage) { | ||
| if (canUseInternalAssignSocket) { | ||
| // ~10% performance improvement in JavaScriptCore due to avoiding .once("close", ...) and removing a listener | ||
| assignSocketInternal(http_res, socket); | ||
| } else { | ||
| http_res.assignSocket(socket); | ||
| } | ||
| } | ||
| return upgradePromise; | ||
|
Comment on lines
617
to
+618
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't drop buffered post-header bytes in the upgrade path.
Suggested change- server.emit("upgrade", http_req, socket, kEmptyBuffer);
+ const head = connectHead ?? kEmptyBuffer;
+ server.emit("upgrade", http_req, socket, head);
return upgradePromise;Based on learnings, 🤖 Prompt for AI Agents
Comment on lines
608
to
+618
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The upgrade handler (line 608) unconditionally calls Extended reasoning...What the bug isThe new upgrade handler at lines 608-618 unconditionally enters raw mode and returns a promise, without checking whether any "upgrade" event listeners are registered on the server. This is in contrast to the CONNECT handler at line 538, which correctly guards with How it manifestsWhen a client sends an HTTP request with an
Why existing code doesn't prevent itThe ImpactThe socket is left in an irrecoverable state: the HTTP parser is permanently disabled via Step-by-step proof
How to fixMirror the CONNECT handler pattern. Before entering raw mode, check
Comment on lines
617
to
+618
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The upgrade handler at line 617 always passes Extended reasoning...What the bug isThe The inconsistencyThe CONNECT handler at line 546 correctly forwards this buffer: const head = connectHead ? connectHead : kEmptyBuffer;
server.emit("connect", http_req, socket, head);But the upgrade handler at line 617 ignores server.emit("upgrade", http_req, socket, kEmptyBuffer);Why the data is permanently lostThe C++ layer ( Step-by-step proof
ImpactAny protocol that pipelines initial data with the HTTP upgrade request (which is common in WebSocket and other upgrade protocols) will silently lose those bytes. The existing regression test doesn't catch this because it sends the protocol payload only after receiving the 101 response. FixChange line 617 from: server.emit("upgrade", http_req, socket, kEmptyBuffer);to: server.emit("upgrade", http_req, socket, connectHead ?? kEmptyBuffer); |
||
| } else if (http_req.headers.expect !== undefined) { | ||
| if (http_req.headers.expect === "100-continue") { | ||
| if (server.listenerCount("checkContinue") > 0) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| test("node:http upgrade socket hands off to userland for bidirectional communication", async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| import http from "node:http"; | ||
| import net from "node:net"; | ||
|
|
||
| const server = http.createServer(); | ||
|
|
||
| server.on("upgrade", (req, socket, head) => { | ||
| socket.write( | ||
| "HTTP/1.1 101 Switching Protocols\\r\\n" + | ||
| "Upgrade: custom\\r\\n" + | ||
| "Connection: Upgrade\\r\\n" + | ||
| "\\r\\n" | ||
| ); | ||
|
|
||
| socket.on("data", (chunk) => { | ||
| socket.write("ECHO:" + chunk.toString()); | ||
| }); | ||
|
|
||
| socket.resume(); | ||
| }); | ||
|
|
||
| server.listen(0, "127.0.0.1", () => { | ||
| const port = server.address().port; | ||
|
|
||
| const client = net.connect(port, "127.0.0.1", () => { | ||
| client.write( | ||
| "GET / HTTP/1.1\\r\\n" + | ||
| "Host: 127.0.0.1\\r\\n" + | ||
| "Upgrade: custom-protocol\\r\\n" + | ||
| "Connection: Upgrade\\r\\n" + | ||
| "\\r\\n" | ||
| ); | ||
|
Comment on lines
+33
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Cover the buffered-head path in this regression. The client only sends Based on learnings, Also applies to: 49-52 🤖 Prompt for AI Agents |
||
| }); | ||
|
|
||
| let gotUpgrade = false; | ||
| let buf = ""; | ||
|
|
||
| client.on("data", (chunk) => { | ||
| buf += chunk.toString(); | ||
|
|
||
| if (!gotUpgrade && buf.includes("\\r\\n\\r\\n")) { | ||
| gotUpgrade = true; | ||
| client.write("hello from client"); | ||
| } | ||
|
|
||
| if (buf.includes("ECHO:")) { | ||
| console.log(buf.substring(buf.indexOf("ECHO:"))); | ||
| client.end(); | ||
| server.close(() => process.exit(0)); | ||
| } | ||
| }); | ||
|
|
||
| setTimeout(() => { | ||
| console.error("TIMEOUT"); | ||
| process.exit(1); | ||
| }, 5000); | ||
|
Comment on lines
+61
to
+64
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move the watchdog out of the child process. This fixed 5s Based on learnings, spawned parent/child tests need explicit timeouts because the default 5000ms is often insufficient; as per coding guidelines, "Do not use 🤖 Prompt for AI Agents |
||
| }); | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).not.toContain("TIMEOUT"); | ||
| expect(stdout.trim()).toBe("ECHO:hello from client"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 When an upgrade request has a body (
hasBody=true),handle.pause()at line 566 pauses the OS socket beforemarkAsRawMode()at line 614 setsisConnectRequest=true. Later,resumeSocket()in Zig checksisConnectRequest()which is now true and returns early without callingus_socket_resume— the OS-level pause is never cleared, making the connection permanently unreadable. The CONNECT handler avoids this by returning at line 548 beforehandle.pause(). Fix: move the upgrade check beforehandle.pause()(like CONNECT) or explicitly resume aftermarkAsRawMode().Extended reasoning...
What the bug is
The new upgrade handler at lines 608-618 runs after the
if (hasBody) { handle.pause(); }check at line 565-566. When an upgrade request includes a body (Content-Length > 0 or Transfer-Encoding present), the OS-level socket is paused viahandle.pause()→doPause()→pauseSocket()(NodeHTTPResponse.zig:124). At this point,isConnectRequest()is stillfalse, so the guard at line 126 passes and the OS socket is paused.Then at line 614,
socketHandle.markAsRawMode()setsisConnectRequest = trueon the uWebSocketsHttpResponseData.Why the socket can never be unpaused
When userland calls
socket.resume(), it flows through#resumeSocket()→response.resume()→doResume()→resumeSocket()(NodeHTTPResponse.zig:133). At line 135, the guard checksthis.raw_response.?.isConnectRequest()— which is nowtrue— and returns early WITHOUT callingthis.raw_response.?.resume(). The OS-level socket pause is never cleared. No data can be received on the connection.Why the CONNECT handler is unaffected
The CONNECT handler correctly avoids this issue because it returns at line 548, before
handle.pause()at line 565. The upgrade handler at line 608 runs after the pause, creating a window where the socket is OS-level paused and thenmarkAsRawMode()prevents it from ever being unpaused.Step-by-step proof with a concrete example
POST / HTTP/1.1\r\nHost: localhost\r\nUpgrade: custom-protocol\r\nConnection: Upgrade\r\nContent-Length: 5\r\n\r\nhellohasBodyistrue(Content-Length > 0), so line 566 callshandle.pause()→pauseSocket()at Zig:124 →isConnectRequest()isfalse→ OS socket is paused viaus_socket_pause.is_upgradeis truthy ("custom-protocol"), entering theelse if (is_upgrade)branch at line 608.socketHandle.markAsRawMode()setsisConnectRequest = true.socket.resume()→#resumeSocket()→resumeSocket()at Zig:133 → guard at line 135 checksisConnectRequest()→true→ returns early. OS socket stays paused.Impact
This is a nit/edge case because standard WebSocket upgrades use GET without a body (
hasBody=false), sohandle.pause()is never called and the socket works fine. Only custom protocol upgrades using POST/PUT with bodies would trigger this. However, for those cases, the connection becomes completely non-functional after the upgrade.Suggested fix
Either (a) move the upgrade check before
handle.pause()at line 565 — mirroring how the CONNECT handler returns at line 548 before the pause — or (b) explicitly resume the OS socket in the upgrade handler aftermarkAsRawMode(), e.g. by callinghandle.resume()beforemarkAsRawMode()or directly after it.