Repository navigation
Conversation
|
Status Reproduced on main with a raw TCP client: import { connect } from "node:net";
function raw(port: number, request: string): Promise<string> {
const { promise, resolve } = Promise.withResolvers<string>();
const socket = connect(port, "127.0.0.1");
let data = "";
socket.on("connect", () => socket.write(request));
socket.on("data", chunk => {
data += chunk.toString("latin1");
if (data.includes("\r\n\r\n")) {
socket.destroy();
resolve(data.split("\r\n")[0] + " | " + data.split("\r\n\r\n")[1]);
}
});
socket.on("close", () => resolve(data === "" ? "(socket closed, no response)" : data.split("\r\n")[0]));
return promise;
}
const tables: Record<string, any> = {
A: { "/x": new Response("x") },
B: { "/x": { HEAD: new Response("h") } },
C: { "/u/:id": () => new Response("id"), "/u/:name": () => new Response("name") },
};
for (const [name, routes] of Object.entries(tables)) {
const server = Bun.serve({ port: 0, routes, fetch: req => new Response("fetch saw " + req.method) });
console.log(name, await raw(server.port, "BREW /nope HTTP/1.1\r\nHost: x\r\n\r\n"));
server.stop(true);
}Before: With this PR, each line is The stack: #44510, #45396, #45397, #45398, #45399 (draft), #45400 (draft). CI: the one red test in the runs of this stack is |
c58a788 to
f81c8f6
Compare
f81c8f6 to
71e415c
Compare
71e415c to
a375993
Compare
There was a problem hiding this comment.
I reviewed the latest push and found no bugs. Since this changes what Bun.serve puts on the wire for every unknown-method request (and now always closes the connection after the 501), a human look at that design choice is still worthwhile — and the PR description's Expect paragraph ("the connection stays open, except with an Expect header") no longer matches the code, which sends Connection: close and closes after every 501 and 404 (packages/bun-uws/src/HttpContext.h:530-540, asserted by the "server closes the connection" tests).
What was reviewed:
- The 501/404 dispatch branch:
end({}, true)runs before any body is consumed, and the post-dispatchis_shut_down/is_closedchecks stop the parser, so a declared body (Content-Length, chunked, Expect) is never delivered to a handler; the tests cover all three. - The
useStrictMethodValidationremoval end to end: no remaining reader in C++, Rust, or JS;uws_app_set_flags/Server__setAppFlags/set_flagsarities match parameter-by-parameter, and the JS call sites pass 7 and 4 arguments as the new ASSERTs expect. - Whether the new 404 branch is reachable for node:http: every known method gets a
"/*"handler insrc/runtime/server/mod.rs(step 9), and node:http's parser already answers 400 to unknown methods, so node:http behavior is unchanged.
Extended reasoning...
The change rewrites the HTTP/1.1 request-dispatch fallback in the vendored uWS server (packages/bun-uws/src/HttpContext.h) so that an unknown method gets a 501 and a router miss gets a 404, both with close-after-response, instead of a silently closed socket, and removes the per-context useStrictMethodValidation flag across the C++/Rust/JS FFI chain in favor of the IsNodeHttp template constant. It touches no auth, crypto, or data-exposure surface, but it is a user-visible protocol behavior change on a hot path in third-party-vendored code with cross-language ABI edits. The code is internally consistent and the new tests pin the exact bytes, but the PR description still describes the earlier keep-alive-after-501 design, so a maintainer should confirm the always-close behavior is the intended one before merge.
a375993 to
b9147c1
Compare
…not a closed socket An HTTP/1.1 request whose method is none of Bun's 36 methods gets 501 Not Implemented before the router, as HTTP/2 does since #40676. No handler runs for it. A request that no route takes gets 404, as over HTTP/2 and HTTP/3. Both answers carry Connection: close, because no handler reads the content of such a request. Before, the socket closed with no response, and after a repeated route registration a handler ran and saw the unknown method as GET. The node:http parser answers 400 to an unknown method. That check was a flag that node:http set after listen. It now follows the server kind, and the flag is removed.
b9147c1 to
eb10891
Compare
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; since this changes what every Bun.serve HTTP/1.1 server answers on the wire (501/404 where the socket used to close) and the PR itself flags a maintainer decision on whether an unknown method should ever reach fetch, a human look is still worthwhile.
What was reviewed:
- The 501/404 branches in
packages/bun-uws/src/HttpContext.h:end({}, true)setsHTTP_CONNECTION_CLOSE, the next dispatch on that socket is refused byisDrainingBeforeClose()and onData's close gate runs after the read, so pipelined requests and withheldExpect: 100-continuebodies behind the answer are not dispatched. - The 404 path: every
set_yield(true)exit inFileRoute.rs/DirectoryRoute.rswrites nothing to the response and balanceson_pending_request, so the status line lands on a fresh response. - The
useStrictMethodValidationremoval end to end (C++ header,uws_app_set_flags,App.rsextern,server_body.rsshim,NodeHTTP.cpparg indices and function lengths, TS binding types) — no stale reference remains, andapplyServerCustomOptionsruns after route registration so the"*"branch never saw the flag true. - The new tests:
netis imported at module scope, the reload test no longer sets a per-test timeout, and the 501 predicate is static-asserted againstHTTP_METHOD_NAMES.
Extended reasoning...
The change replaces a force-close in the vendored uWS HTTP/1.1 dispatch lambda with a 501 for methods outside HTTP_METHOD_NAMES and a 404 for a router miss, both with Connection: close, and removes the useStrictMethodValidation flag across the C++/Rust/JS FFI chain in favor of the IsNodeHttp template parameter; it touches no auth, crypto, or data-exposure surface, only HTTP status semantics. The flag removal is complete (no stale references in the repo), the FFI argument order is consistent at every layer, and the pipelined-dispatch and yield paths behave as the PR describes. It does not get an approve because it is a user-visible behavior change on every Bun.serve server's hot path, the PR description itself defers a product question to maintainers, and an earlier pre-existing note about known-method misdispatch via root.children.back() was resolved by the author rather than independently. No CODEOWNERS entry covers the changed files.
Part of #6556 and #18406
Problem
BREW,get) closes the socket with no response. On some route tables a handler runs and seesreq.method === "GET".HttpRouter::route()(packages/bun-uws/src/HttpRouter.h:276) falls back toroot.children.back(). A repeated registration erases the empty any-method node, so that is another method's tree.BREW /gives 502 to every client for about 10 s (Notes).Fix
501 Not Implementedbefore the router, like HTTP/2 since Bun.serve http2: reject the request bytes the HTTP/1 parser rejects #40676. No handler runs.404 Not Found, like over HTTP/2 and HTTP/3.Connection: close, because no handler reads the content of that request.test/js/bun/http/bun-serve-routes.test.ts, 15 fail without the change. Self-reviewed: 17 concerns raised, 16 addressed (Notes).Background
Downsides
GET.Notes
Answers, before and after. Raw requests.
fetchisreq => new Response("fetch saw " + req.method). Each new answer hasConnection: closeand no body.routesBREW /"/x": { HEAD: new Response("h") }BREW /fetch saw GET"/u/:id": fn1, "/u/:name": fn2GETS /u/1fn2, asGET"/x": fn, "/y": { HEAD: new Response("h") }patch /xfn, asPATCH"/*": new Response(Bun.file(missing))GET /xBehind a proxy. Measured with nginx 1.30.6, default upstream settings (
max_fails=1,fail_timeout=10s), two Bun processes as upstream peers.BREW /gets 502. nginx logsupstream prematurely closed connectionandupstream server temporarily disabledfor both peers. The next 21GET /requests get 502 (no live upstreams). The first 200 comes 10.9 s after theBREW.GET /requests get 200. With"/*": new Response(Bun.file(missing))nginx relays the 404 and marks no peer.proxy_pass http://localhost:PORTis two peers for nginx ([::1]and127.0.0.1), so the same happens there.GETS /secretor a lowercasedeleteasGETorDELETE. On the route tables of the next paragraph Bun ran a handler for them, asGETfor an uppercase unknown token and as the method itself for a lowercase known name. A method rule at the proxy did not hold for those requests.How a second registration changed the answer.
HttpRouter::add()first removes an earlier handler for the same method, pattern and priority.cullNode()then erases every node with no handler and no child. On HTTP/1.1 the any-method node is always empty, becauseany()registers under each of the 36 method names. After the removal,root.children.back()is the tree of the method that sorts last. Any second registration of one method, pattern shape and priority does it. Five sources are confirmed: a static route whose method set hasHEAD(apply_static_routeregistersHEADtwice), two parameter routes of one shape (/u/:idand/u/:name), a static"/*"route with a method object,"/*": false, andwebsocketwith"/*": { GET: fn }.Why 501. HTTP/2 answers 501 since #40676. RFC 9110 15.6.2 names 501 for a method the server does not recognize. Another status is a one-line change at the check in
HttpContext.h. #35299 proposed the HTTP/1.1 501 earlier and closed in a cleanup of stale PRs. It answered after the router missed, and it closed the connection. Its raw-socket cases (FROB,get,BREW,GETS) are in the new tests.Why the connection closes. The parser's own error responses (400, 431, 505) and the 413 close the connection too. The first shape of this PR kept the connection open after the 501. The self-review found two problems with that. A client that pipelines
BREWrequests and never reads made the server queue a 501 for each one, with no bound. A client that sendsExpect: 100-continue, holds back the content and then sends its next request had that request read as the content. WithConnection: closethe server answers once, does not dispatch a request behind that answer, and closes. So there is no rule forExpecthere.The 404.
route()returns false when the method has no tree, or when each handler for the URL yields. A file route yields when it cannot open its file. HTTP/2 and HTTP/3 answer 404 to such a request (Http2Context.h:1729,Http3Context.h:41). HTTP/1.1 closed the socket, with the same effect on a proxy as above. #45397 then registers the default handler for every method, sofetchanswersPROPFIND /xunder"/*": { GET: fn }.HTTP/3. Over HTTP/3 an unknown
:methodruns the any-method routes on every server, and the handler seesGET. #45357 answers 501 there.Reach. A differential test of the router found the GET dispatch. No user reported it. A handler runs on a route table with a repeated registration. Every other server moves from an empty reply to 501. #6556 asks for a custom method that reaches the handler, or for a more graceful answer: this PR gives the second. #21566 asked for the first and closed as a duplicate. #18406 expects 501 for an unknown method: a raw
BUN /testnow gets it.fetch(url, { method: "BUN" })still sendsGET(#42497), so a test that sends the request withfetch()still gets 200.A method outside the 36. Bun.serve has no route for such a method and gives it to no handler. #44718 (open) lets a
Requestcarry any method token, forfetch()andnew Request(). With both PRs,server.fetch(new Request(url, { method: "BREW" }))reaches the handler and aBREWon the wire gets 501. Whether a wireBREWshould reachfetchis the first ask of #6556. That needs a maintainer decision. This check is one branch, and either decision keeps it or deletes it.node:http. A node:http server does not change. The strict method check was a runtime flag,
useStrictMethodValidation, that node:http's JavaScript set right afterBun.serve()returned. The parser of the node:http instantiation now always has it. A server made with the privateonNodeHTTPRequestoption and no node:http module gets it too. The flag had one reader left, the"*"expansion inonHttp, which runs when routes are registered. For a node:http server that is before its JavaScript sets the flag. So the flag and its plumbing are removed:HttpFlags,setFlags,uws_app_set_flags,Server__setAppFlags, the two bindings inNodeHTTP.cpp, and the argument in_http_server.ts. One case changes. Underbun --hot, a script that callsBun.serve()and thenhttp.createServer().listen()on the same port gets one plain server, and node:http pushes its options to it. That server answered 400 toBREWand now answers 501.Landing order. The fix needs only a yes or no for the method. It is on top of #44510 so that the hot path stays below main: GET is 2435 instructions on main, 2414 with #44510, and 2423 with this PR. On main the same check is
isKnownMethod(), the predicate of the HTTP/2 check, and two changed lines. The self-review measured that shape at 33 instructions more per request than main. If a maintainer wants the fix before the refactor, I re-cut this PR onto main.Measurements.
fetch: 2435, 2414, 2423. POST tofetch: 2750, 2719, 2724. PUT tofetch: 2635, 2614, 2629. PROPFIND tofetch: 2751, 2673, 2693. UNSUBSCRIBE tofetch: 2861, 2753, 2776. GET to a GET route: 2905, 2869, 2892. POST to a POST route: 3219, 3174, 3193. PUT to a PUT route: 3105, 3069, 3098. The new work is onemethodIdFromWire()per request: a switch on the length and one or two compares. The two PRs together are below main on all eight rows. A standalone driver runs the realHttpParser.handHttpRouter.h(clang -O3) underqemu-x86_64 -one-insn-per-tb -d exec,nochain. Repeat runs differ by at most 4 instructions in 2.6 million. The layout of the driver moves single rows by up to 10 instructions: an earlier build of it gave 2406, 2712 and 2606 for the first three numbers of uws: resolve a request method to its id inline #44510, and 2732 for POST on this PR. So a difference under 10 is not a signal.perfis not available in the build container (perf_event_paranoidis 4), andvalgrindis not installed.BREWrequest: 0. No handler runs.llvm-size): text 106,004,655 -> 106,003,623 bytes (uws: resolve a request method to its id inline #44510, then this PR, on base a407eae). data and bss do not change.Self-review. A first review of the whole stack raised 33 concerns. It asked for this cut: the 501 alone, then the default handler, then the router. 32 are done. Not done: the router PR (#45400) is not based on #44323, and the one that lands second adapts. A second review of this PR raised 17 concerns. 16 are done: the 404 for a router miss, the close after the 501 (which removed the
Expectrule), the comment that states the policy, the server with nofetchin the tests, and the text of this description. Not done: a seeded model test of the router in the test suite. It is a test-only change for its own PR. The review also preferred this PR on main without #44510: see Landing order.Not in this PR.
new Request(),fetch()andserver.fetch()still turn an unknown method intoGET(Support arbitrary method infetch#6021, fetch and Request silently turn an unrecognised HTTP method into GET #42497, fetch, Request, server.fetch: stop sending GET for a method outside the method table #44718)._(M2,BREW_X), and 505 to one with a byte above 0x7f. HTTP/2 answers 501.root.children.back(). A known method with no tree of its own can reach the tree of another method:PROPFIND /xunder"/*": { GET: new Response("s"), TRACE: fn }runs theTRACEhandler. Bun.serve: register the default handler on "/*" for every method #45397 and uws: key HttpRouter by method id #45400 change that.BREW: fn, is not read when one of the nine is present.Expect: 100-Continuein another case gets no100 Continue(Bun.serve does not auto-respond "100 Continue" when "Expect" header value uses non-lowercase casing (RFC 7231 violation) #30248, Bun.serve: handle Expect per RFC 9110 (case-insensitive, params/list, auto-417) #30250).HEADand the built-in 404 with no bound. That is on main. The 501 and this 404 are not in that class, because they close the connection.requireHostHeader, the two lenient parser bits,httpAllowHalfOpen,maxHeadersCount,maxHeaderSize, andusingCustomExpectHandler.server.reload(), a TLS connection whose SNI matches atls.serverNamestill uses that name's own router, which reload does not rebuild (Segfault regression in v1.2.3 when using TLS with locally signed certificate #19868). serve: fix route use-after-free after reload() with tls.serverName #38636 removes those routers and edits the same dispatch block. The PR that lands second needs a rebase.websocket, an upgrade request under"/*": { GET: fn }reachesfetchand not the route.Suites (debug build with ASAN). On this PR:
bun-serve-routes(78 tests),serve.test.ts,bun-serve-static,bun-serve-file,serve-directory-routes,bun-serve-html,node-http,request-smuggling,serve-http2-protocol, and 35test-http-*Node tests (methods,Expect, parser options, client errors). Inserve.test.tstwo tests fail in this container without the change too: one needs a user that is not root, and one is a 10 s throughput test that takes 8 to 12 s here. Innode-http, tests of one group hit their 5 s timeout. The group has 11 tests that run at once, each with a child process, and each passes alone.