Skip to content

node:http: keep the method of a dispatched request as sent in 'clientError' rawPacket - #45398

Open
robobun wants to merge 1 commit into
robobun/5627effc/serve-star-default-anyfrom
robobun/5627effc/node-http-rawpacket-method
Open

robobun wants to merge 1 commit into
robobun/5627effc/serve-star-default-anyfrom
robobun/5627effc/node-http-rawpacket-method

Conversation

@robobun

@robobun robobun commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • node:http: the rawPacket of a 'clientError' shows an earlier request of the same read with a lowercase method, get /a HTTP/1.1. Node.js shows the bytes as the client sent them.
  • HttpRequest::getMethod() (packages/bun-uws/src/HttpParser.h:539) lowercases the method in the receive buffer when a handler reads it. rawPacket is that buffer.

Fix

Background

  • uWS parses in place. The fields of a request are views into the bytes of the read.
  • 'clientError' gets the read that held the bad request. An earlier request of that read ran its handler before the error.
  • Considered a copy of the method for the lowercase form. No reader needs the lowercase form.

Downsides

  • Code that matched a lowercase method in rawPacket sees a difference.
  • Per request: GET 2423 to 2395 instructions, POST 2732 to 2697 (Notes). Release binary: 1,280 bytes larger.
Notes
  • The read in the test is GET /a HTTP/1.1\r\nHost: x\r\n\r\nGET /b HTTP/1.1\r\nHost: x\r\nno colon here\r\n\r\n. The handler of /a runs, then the parser fails on /b. Node.js v26.3.0 reports the read as sent. Bun reported get /a ... for the first request.
  • getMethod() has two readers. uws_req_get_method gives the name to Rust, where each reader looks it up with Method::which or Method::find. METHOD_MAP has an uppercase and a lowercase entry for each method. The second reader is a fallback in NodeHTTP.cpp that a node:http server does not reach, because its parser accepts only the 36 uppercase names.
  • Before Bun.serve: answer 501 to an unknown method and 404 to a router miss, not a closed socket #45396 a mixed-case method such as Post that reached a handler was read as POST through this lowercase pass. With Bun.serve: answer 501 to an unknown method and 404 to a router miss, not a closed socket #45396 that request gets 501 before any handler. So this PR changes only rawPacket.
  • HTTP/2 and HTTP/3 are not involved. Http3Request::getMethod() keeps its lowercase copy, because their routers register lowercase names.
  • Instructions per request for parse, route and method read, Bun.serve: register the default handler on "/*" for every method #45397 then this PR. GET to fetch: 2423, 2395. POST to fetch: 2732, 2697. PUT to fetch: 2629, 2601. PROPFIND to fetch: 2693, 2630. UNSUBSCRIBE to fetch: 2776, 2692. node:http, GET: 2581, 2553. A standalone driver runs the real HttpParser.h and HttpRouter.h (clang -O3) under qemu-x86_64 -one-insn-per-tb -d exec,nochain. Repeat runs differ by at most 4 instructions in 2.6 million.
  • Instructions per request end to end: not measured. perf is not available in the build container (perf_event_paranoid is 4), and valgrind is not installed.
  • Release binary (llvm-size): text 80,700,499 -> 80,701,779 bytes (Bun.serve: register the default handler on "/*" for every method #45397, then this PR, on base f4d755a). data and bss do not change. The C shim behind AnyRequest::method() is now small enough that link-time optimization inlines the function into its callers: 584 bytes less for it, and 422 to 589 bytes more in each of 6 route callbacks.
  • Suites (debug build with ASAN). On this PR: node-http-pipeline-callbacks, bun-serve-routes, node-http, and 35 test-http-* Node tests. On the PRs after it, which have this change: node-http-displaced-response, serve-http2, serve-http3, three test-http-*-client-error Node tests.

@robobun

robobun commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on main, and compared with Node.js v26.3.0:

const http = require("node:http");
const net = require("node:net");
const payload = "GET /a HTTP/1.1\r\nHost: x\r\n\r\nGET /b HTTP/1.1\r\nHost: x\r\nno colon here\r\n\r\n";
const server = http.createServer((req, res) => res.end("ok"));
server.on("clientError", (err, socket) => {
  console.log(JSON.stringify({ code: err.code, rawPacket: err.rawPacket.toString("latin1") }));
  socket.destroy();
  server.close();
});
server.listen(0, "127.0.0.1", () => {
  const c = net.connect(server.address().port, "127.0.0.1", () => c.write(payload));
  c.on("error", () => {});
});

Node.js prints a rawPacket that starts with GET /a. Bun printed get /a. With this PR Bun prints GET /a.

The stack: #44510, #45396, #45397, #45398, #45399 (draft), #45400 (draft).

CI: the one red test in the runs of this stack is test/cli/install/bun-install-windows-rename-retry.test.ts on Windows 11 aarch64. It is red on other PRs too, and this diff does not touch install code.

@robobun
robobun force-pushed the robobun/5627effc/node-http-rawpacket-method branch from c2b76a4 to de273e4 Compare October 15, 2026 20:32
@robobun
robobun force-pushed the robobun/5627effc/serve-star-default-any branch from 9cd6431 to 6a5010d Compare October 15, 2026 20:32

@claude claude Bot left a comment

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.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked whether dropping the in-place lowercasing changes HTTP/1 routing or node:http's req.method: HttpRouter::route (packages/bun-uws/src/HttpRouter.h:255) matches method nodes by their uppercase names ("GET") with the "*" node as the last fallback, so the as-sent method routes the same as before; and node:http's strict request-line validation (isStrictMethodChar plus the methodIdFromWire check in packages/bun-uws/src/HttpParser.h:877-914) rejects any lowercase byte before dispatch, so a node:http handler still only ever sees an all-uppercase method. The mixed-case concern therefore only applies where strict validation is off and the 501 guard is skipped (a direct Bun.serve({ onNodeHTTPRequest }) context), which is not a documented user path.

Extended reasoning...

The change removes the |= 32 lowercasing in uWS HttpRequest::getMethod(), deletes getCaseSensitiveMethod(), rewires its two HttpContext.h callers, and adds one node:http pipelining test; it touches the HTTP/1 request parser (untrusted network bytes) but adds no new parsing, only removes a write into the read buffer. Two confirmed inline findings exist (test failure wiring and the H3 casing divergence), so the inline comments already signal that a human look is needed; this note only records what else was checked and ruled out.

Comment thread packages/bun-uws/src/Http3Request.h Resolved

@claude claude Bot left a comment

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.

Code review found no issues

No high-confidence issues detected in this change.

Still open from earlier reviews (2):

  • Unresolved: 2 minor or pre-existing.

@robobun
robobun force-pushed the robobun/5627effc/serve-star-default-any branch from 6a5010d to c7df286 Compare October 15, 2026 21:45
@robobun
robobun force-pushed the robobun/5627effc/node-http-rawpacket-method branch from de273e4 to 8b4be0f Compare October 15, 2026 21:45
@robobun

robobun commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:49 AM PT - Oct 17th, 2026

⏳ @robobun, your commit 84f2a1d is still building in Build #127447, but has 1 failures so far (All Failures):

@claude claude Bot left a comment

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.

Code review found no issues

No high-confidence issues detected in this change.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

@robobun
robobun force-pushed the robobun/5627effc/serve-star-default-any branch from c7df286 to fbde686 Compare October 15, 2026 21:57
@robobun
robobun force-pushed the robobun/5627effc/node-http-rawpacket-method branch from 8b4be0f to 989d5db Compare October 15, 2026 21:57

@claude claude Bot left a comment

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.

Code review found no issues

No high-confidence issues detected in this change.

@robobun
robobun force-pushed the robobun/5627effc/serve-star-default-any branch from fbde686 to 47a7b48 Compare October 17, 2026 09:17
@robobun
robobun force-pushed the robobun/5627effc/node-http-rawpacket-method branch from 989d5db to 84f2a1d Compare October 17, 2026 09:28

@claude claude Bot left a comment

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.

Code review found no issues

No high-confidence issues detected in this change.

@robobun
robobun force-pushed the robobun/5627effc/node-http-rawpacket-method branch from 84f2a1d to c5d8886 Compare October 17, 2026 10:06
@robobun
robobun force-pushed the robobun/5627effc/serve-star-default-any branch from 47a7b48 to 142d9eb Compare October 17, 2026 10:06

@claude claude Bot left a comment

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.

Code review found no issues

No high-confidence issues detected in this change.

…Error' rawPacket

HttpRequest::getMethod() lowercased the method in the receive buffer when
a handler read it. rawPacket is that buffer, so an earlier request of the
same read showed as "get /a". getMethod() now returns the method as the
client sent it, and getCaseSensitiveMethod(), which did the same, goes.
@robobun
robobun force-pushed the robobun/5627effc/node-http-rawpacket-method branch from c5d8886 to 5c34108 Compare October 17, 2026 11:03
@robobun
robobun force-pushed the robobun/5627effc/serve-star-default-any branch from 142d9eb to eaf2275 Compare October 17, 2026 11:03

@claude claude Bot left a comment

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.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant