Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. Walkthrough
ChangesHTTP response header termination
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Header termination preserves the inspected HTTP response output; no issue identified here prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the equivalence risks of the refactor: the terminateHeaders(true) call at HttpResponse.h:611 sits under an HTTP_WROTE_TRANSFER_ENCODING_HEADER guard, so it cannot emit a second Transfer-Encoding line; the second writeMark() inside terminateHeaders() on the Content-Length path is a no-op via HTTP_WROTE_DATE_HEADER, so Date still precedes Content-Length; and uws_res_prepare_for_sendfile runs inside the corker callback, so Super::write("\r\n") lands in the same cork buffer getSendBuffer(2) used.
Extended reasoning...
The change is a refactor of the vendored uWebSockets HttpResponse.h and the uws_res_prepare_for_sendfile C shim, consolidating eleven hand-inlined Date/Transfer-Encoding/blank-line sequences into one terminateHeaders(bool chunked) helper, with no tests added. It touches HTTP/1 response framing (which headers are emitted and in what order) but no auth, crypto, or input-parsing surface. Each replaced site was compared against its old sequence and the chunked/non-chunked argument matches what the old code wrote at every site; the one inline finding is an extra send() on the uncorked Content-Length end path rather than a wire-format difference.
…eaders() Add HttpResponse::writeOwnedHeaders(chunked), which writes the Date header (unless the caller wrote one) and the Transfer-Encoding: chunked header for a chunked body (unless the caller wrote one), and terminateHeaders(), which writes them and then the blank line. Every path in HttpResponse.h that closed the header section with its own writeMark() + CRLF block now calls terminateHeaders(), and so does uws_res_prepare_for_sendfile. The Content-Length path of internalEnd() calls writeOwnedHeaders() and keeps its blank line in the same write as the Content-Length line, so an uncorked end stays one send(). uws_res_end_without_body stays as it is: it writes no Date today. Wire output is unchanged on every path.
81f8803 to
c5b9d81
Compare
Part of #43853
Behaviour change: none
Problem
packages/bun-uws/src/HttpResponse.hend the HTTP/1 header section with their ownwriteMark()+"\r\n"block, anduws_res_prepare_for_sendfile(src/uws_sys/libuwsockets.cpp) has one more. Five of them also copy theTransfer-Encoding: chunkedline.internalEndcan write aConnection: closeline. The fix for node:http default agent reuses a socket after a Connection: close request in 1.4.2 (every second request ECONNRESET/EPIPE) #43853 needs that decision on every path, so it needs one place where every path ends its headers.Fix
HttpResponse::writeOwnedHeaders(chunked):Date(unless the caller wrote one) andTransfer-Encoding: chunkedfor a chunked body (unless the caller wrote one).terminateHeaders(chunked)adds the blank line. Every block inHttpResponse.handuws_res_prepare_for_sendfilenow call it. The Content-Length path ofinternalEndcallswriteOwnedHeadersand keeps the blank line in the Content-Length write.uws_res_end_without_bodystays as it is: it writes noDatetoday, so routing it through would change the wire. The next PR does that.Connection: closeand an HTTP/1.0 request, is byte-identical to a build ofmain(Date masked).test/js/bun/http/bun-serve-headers.test.ts,bun-serve-static,bun-serve-file,request-smuggling,serve-direct-readable-stream,serve.test.ts,bun-server.test.ts,test/js/node/http/,websocket-server.test.ts.Background
writeMark()writes theDateheader once per response and records that inHTTP_WROTE_DATE_HEADER. The blocks this PR folds all called it right before the blank line..textof the stack: 80,656,785 to 80,655,761 bytes (size, -1,024). TheHttpResponse<true|false>anduws_res_*symbols involved: 12,809 to 12,223 bytes (nm -S).Notes
internalEnd(a large in-memory body from an async handler, aBun.fileend from a read callback) issues the samesend()calls as before:writeOwnedHeadersiswriteMark(), and the Content-Length line plus the blank line stay one write.uws_res_prepare_for_sendfileusedgetSendBuffer(2)for the blank line.Super::write("\r\n", 2)does the same when corked. When not corked, both end in onesendfor the two bytes:getSendBuffertook a cork slot anduncork()flushed it.bun-profile(objdump):HttpResponse<false>::internalEnd493 to 465,write300 to 276,terminateHeaders60 (new, shared).terminateHeaders():writeContinue()(100 Continue) and the raw 1xx writer. They are informational responses with no header section of their own.terminateHeaders().websocket-server.test.tshas 4 tests that time out in this container onmaintoo (send() > Buffer (utf-8)and siblings).serve.test.tshas 2 that need a non-root user or a non-loopback interface.no test proof · iteration 1 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check