node:http: keep parsing other connections after an upgrade from a request body handler

Closed
RO
1mo ago
+115-20

Problem

  • A node:http server upgrades a request to a WebSocket from the request's body handler (wss.handleUpgrade() inside req.on("end") of an Upgrade request with a body). After that, the next request on any other connection of the server stops the parser after its first request. Requests pipelined behind it get no response and the connection stays open. Node answers all of them.
  • HttpResponse::upgrade() sets httpContextData->upgradedWebSocket while the parser runs. Only the request handler in HttpContext::onData checks the field (HttpContext.h:560). The data handler does not. After an upgrade from inStream() it writes into the destroyed HttpResponseData and returns the socket. onData takes its normal path and never resets the field. The next request on another connection sees the stale field after route() and returns nullptr.

Fix

  • The data handler returns nullptr when upgradedWebSocket is set after inStream() returns, like it does for a closed socket. onData then takes its upgrade path, which uncorks the WebSocket and resets the field.
  • upgrade() sets the field only for the socket onData parses (httpContextData->parsingSocket). A body handler can upgrade another connection's parked request. To the parser that is an async upgrade, and the parsed connection keeps going.
  • The parser's Content-Length branches (HttpParser.h, three places) update remainingStreamingBytes before they call the data handler, like the chunked branch does. A handler that upgrades destroys the HttpResponseData the parser lives in. A handler that only shuts the socket down leaves it alive, and onEnd reads the count from it.
  • Verified: test/js/first_party/ws/ws.test.ts (three new cases: body in the same read as the head, body in a read of its own, upgrade of another connection from the body handler; all fail on bun 1.4.3). Also the rest of that file, test/js/node/http/node-http*.test.ts, test/js/bun/http/serve.test.ts and test/js/bun/websocket/.

Background

  • HttpContext::onData runs the HTTP parser on each read. The parser calls a request handler for each request head and a data handler for each body chunk. A handler returns the socket to continue, or nullptr to stop.
  • HttpResponse::upgrade() adopts the socket into the WebSocket context in place. It destroys the HttpResponseData in the socket ext, which also holds the parser state. It then sets upgradedWebSocket so that onData knows the socket changed.
  • onData tells a stopped parse apart by that field: nullptr with the field set means upgraded, without it means closed.

Fixes #43163

Notes
  • Repro: the script in #43163. On main it prints ["/one"] (no answer to /two after 3 s). With this change it prints ["/one","/two"] (connection closed). The same for a body in the same read as the head and for a chunked body.
  • The node:http tunnel data event (JSNodeHTTPServerSocket::onData) is posted as a task, so JS never runs inside onSocketData. No check is needed there.
  • Bun.serve does not reach the data-handler upgrade path: server.upgrade(req) after await req.text() returns true on main, and a second connection with two pipelined requests gets both responses.
  • The parsingSocket guard is the same idea as #37463, which also removes isParsingHttp. This PR keeps that flag. #43153 dispatches the bytes after the request head to the WebSocket. Neither covers an upgrade from the data handler.
  • The first version of this PR returned from the Content-Length branches before the state update. On a TLS socket that a handler shut down, onEnd would then have reported a spurious HPE_INVALID_EOF. Review caught it. The update now runs before the handler.
  • test/js/bun/websocket/websocket-server.test.ts has 9 to 10 timeouts in this container under the debug build, on main and with this change alike (the tests that run concurrently with the 10-client benchmark). They pass in isolation. serve.test.ts has two failures that come from running as root in the container.