Repository navigation
node:http: a tunnel holds the event loop only while it reads or has bytes left to send #44885
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,6 +203,7 @@ void JSNodeHTTPServerSocket::readStop() | |
| } | ||
| tunnelReadsStopped = true; | ||
| applyTunnelReads(); | ||
| updateTunnelIdle(); | ||
| } | ||
|
|
||
| void JSNodeHTTPServerSocket::readStart() | ||
|
|
@@ -212,6 +213,7 @@ void JSNodeHTTPServerSocket::readStart() | |
| } | ||
| tunnelReadsStopped = false; | ||
| applyTunnelReads(); | ||
| updateTunnelIdle(); | ||
|
dylan-conway marked this conversation as resolved.
|
||
| } | ||
|
|
||
| void JSNodeHTTPServerSocket::didDeliverQueuedTunnelBytes(size_t length) | ||
|
|
@@ -396,6 +398,8 @@ JSC::EncodedJSValue JSNodeHTTPServerSocket::halfClose(JSC::JSGlobalObject* globa | |
| if (socket && cycleReads) { | ||
| us_socket_resume(socket); | ||
| } | ||
| // The shutdown can send what TLS still held, and no writable event comes behind the FIN. | ||
| updateTunnelIdle(); | ||
| return result; | ||
| } | ||
|
|
||
|
|
@@ -884,16 +888,26 @@ extern "C" bool Bun__NodeHTTPServerSocket__writeBehindResponse(us_socket_t* sock | |
| return is_ssl ? writeBehindResponse<true>(socket, data, length) : writeBehindResponse<false>(socket, data, length); | ||
| } | ||
|
|
||
| template<bool SSL> | ||
| static void updateTunnelIdleImpl(us_socket_t* socket, bool reads, bool buffered) | ||
| { | ||
| auto* response = reinterpret_cast<uWS::HttpResponse<SSL>*>(socket); | ||
| const bool sending = buffered || !response->hasFullyDrained(); | ||
| response->setNodeHttpTunnelSending(sending); | ||
| response->setNodeHttpTunnelIdle(!reads && !sending); | ||
| } | ||
|
|
||
| void JSNodeHTTPServerSocket::updateTunnelIdle() | ||
| { | ||
| if (!isTunnel(this)) { | ||
| return; | ||
| } | ||
|
Comment on lines
+902
to
904
Contributor
There was a problem hiding this comment. 🟣 pre-existing, not blocking: pre-existing: on an Upgrade request that carries a body, bytes a listener writes before the body completes neither hold the event loop nor get flushed, so behind server.unref() the process exits with them unsent and socket.end() after them never sends its FIN. updateTunnelIdle() at JSNodeHTTPServerSocket.cpp:902 returns while isConnectRequest is still false (HTTP_NODE_TUNNEL_AFTER_BODY), so no +5 fires, and HttpContext.h:940 skips onSocketDrain for the same reason, so the writable event drops its interest with streamBuffer still full. Fix: treat a HTTP_NODE_TUNNEL_AFTER_BODY socket as a tunnel for drain and sending/idle accounting, so the rule "bytes left to send hold the loop" covers this sibling path too. Why this was flaggedAn 'upgrade' listener for a request with Content-Length > 0 calls socket.write(data) larger than the kernel takes while the body is still arriving. jsFunctionNodeHTTPServerSocketWrite (JSNodeHTTPServerSocketPrototype.cpp:281) parks the rest in streamBuffer, then calls updateTunnelIdle(), which returns at JSNodeHTTPServerSocket.cpp:902 because isTunnel() reads isConnectRequest and that is only set at HttpContext.h:672 when the body's fin is parsed; so setNodeHttpTunnelSending(true) never runs and sending_tunnel_ref (mod.rs:1633) is never taken. The writable event reaches HttpContext::onWritable, but :940 gates onSocketDrain on isConnectRequest, so JSNodeHTTPServerSocket::onDrain never flushes streamBuffer and loop.c:665 drops the writable interest. With server.unref() and nothing else holding the loop, the process exits with the bytes unsent; without unref, a later socket.end() returns at JSNodeHTTPServerSocketPrototype.cpp:317-320 waiting for a drain that never comes, so the FIN is never sent. The base branch behaves the same on this path. Verification: pre-existing. An 'upgrade' listener for an Upgrade request that carries a body writes more than the kernel takes while the body is still arriving.
Member
There was a problem hiding this comment. Stays as is, and the description lists it under "Not changed by this PR": the same output on 1.4.3, on a canary of main, on 5af7e9d and here, for Generated by Claude Code |
||
| const bool reads = !tunnelReadEnded && !tunnelReadsStopped; | ||
| const bool buffered = streamBuffer.bufferedSize() > 0; | ||
| if (is_ssl) { | ||
| updateTunnelIdleImpl<true>(socket, reads, buffered); | ||
| } else { | ||
| updateTunnelIdleImpl<false>(socket, reads, buffered); | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
🟣 pre-existing, not blocking: pre-existing: over plain http a request body sent behind the FIN of req.socket.end() is still dropped when the finished response does not fit the kernel send buffer; the new SSL-only branch at HttpContext.h:899 lets https read it. For non-SSL, deferShutdownUntilResponseDrains (JSNodeHTTPServerSocket.cpp:358-369) sees uWS bytes and sets HTTP_CONNECTION_CLOSE, so the gate at HttpContext.h:1017-1021 runs shutdown() then close() once drained. Fix: on the non-SSL twin, a half-close deferred behind buffered response bytes must end in a shutdown only (a half-close-after-drain state consumed by onWritable), as the small-response path and Node (allowHalfOpen) do; then unskip the http row at node-http-server-socket-end-drain.test.ts:187. [also at: test/js/node/http/node-http-server-socket-end-drain.test.ts:204 - nit, pre-existing: this test locks in https keeping a
Connection: closeconnection open afterreq.socket.end()to read the body behind its FIN, which Node and Bun's own http twin do not do.]A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A node:http (not https) handler calls res.end(body) with a body larger than the kernel takes in one send, then req.socket.end(), and the client sends a request body after it receives the server's FIN (the http row that test/js/node/http/node-http-server-socket-end-drain.test.ts:187 skips). jsFunctionNodeHTTPServerSocketEnd (JSNodeHTTPServerSocketPrototype.cpp:303) calls shutdownAfterResponseDrains; deferShutdownUntilResponseDrains at JSNodeHTTPServerSocket.cpp:358-369 finds getBufferedAmount() > 0 and sets HTTP_CONNECTION_CLOSE instead of a half-close. When the buffer drains, HttpContext::onWritable reaches the gate at HttpContext.h:997-1023: shouldCloseConnection() is true, so it runs shutdown() and close() and the request body never reaches req ('close' without 'end'). Over https getBufferedAmount() is 0, halfClose() defers only the shutdown, and the new early return at HttpContext.h:899-905 keeps that gate from running, so the body is read. The base branch drops the body on http the same way; this PR fixes only the SSL twin.
Verification: JSNodeHTTPServerSocket.cpp:358-369 sets HTTP_CONNECTION_CLOSE (line 364) when getBufferedAmount() != 0, so halfClose() is never reached. On the next writable event the gate at HttpContext.h:1017-1021 runs asyncSocket->shutdown(); asyncSocket->close(); and the body the client sends after seeing the FIN is never read. The new early return at HttpContext.h:899-905 is SSL-only.
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.
Both points are right, and both stay as they are in this PR: I ran
res.end(body)+req.socket.end()+ a request body behind the FIN for http/https, with and withoutConnection: close, with 2 B / 64 KiB / 8 MiB responses behind a 4 KiBSO_SNDBUF, and this head gives the same result as5af7e9dcin all 12 cases.Connection: close, Node v22.22.0 reads the body in all six cases. Bun drops it over http at 64 KiB and 8 MiB, and over https at 8 MiB. A half-close that waits for the uWS buffer is a change to ordinary responses, not to tunnels, so it belongs in its own PR.Connection: close, Node reads the body in none of the six cases. Bun reads it only over https at 64 KiB, where TLS alone holds the rest of the response. The test pins that case because it is the one place where the new writable event behind the FIN could change what an ordinary response does, not because it matches Node. If another push comes, the comment above the test will say so.Generated by Claude Code