diff --git a/packages/bun-uws/src/HttpContext.h b/packages/bun-uws/src/HttpContext.h index 6f7cf7441cca..e34be2f1fe76 100644 --- a/packages/bun-uws/src/HttpContext.h +++ b/packages/bun-uws/src/HttpContext.h @@ -744,9 +744,17 @@ struct HttpContext { * nothing in AsyncSocketData::buffer). A retry that moves zero bytes * after the peer's FIN is EPIPE; close instead of spinning. Except * on libuv, where the retry can stall while the TLS layer's spill - * is still blocked on a healthy socket; there the kernel is asked. */ + * is still blocked on a healthy socket; there the kernel is asked. + * HTTP_END_CALLED (with HTTP_RESPONSE_PENDING) is what identifies a + * tryEnd tail; it is the only deferred shape whose progress shows + * up in `offset`. A deferred file body (HTTP_FIXED_LENGTH_FILE_BODY) + * moves bytes with sendfile()/write() without touching it and + * would read as stalled here every time; it notices a dead peer on + * its own (a failed sendfile force-closes, a failed write() lands + * in the buffer and trips the flushed == 0 check above). */ if ((httpResponseData->state & HttpResponseData::HTTP_NODE_RECEIVED_FIN) && (httpResponseData->state & HttpResponseData::HTTP_RESPONSE_PENDING) + && (httpResponseData->state & HttpResponseData::HTTP_END_CALLED) && httpResponseData->offset == offsetBefore && asyncSocket->hasFullyDrained() && us_socket_stalled_write_means_peer_gone((us_socket_t *) asyncSocket)) { @@ -861,22 +869,26 @@ struct HttpContext { return s; } } else { - /* Bun.serve: response bytes already handed to uWS must drain before - * the connection shuts down (from the existing shouldCloseConnection() - * gates), not be discarded by the close() below. Only a response that - * is fully determined qualifies: a tryEnd tail (content-length path - * sets HTTP_END_CALLED while offset < total keeps HTTP_RESPONSE_PENDING) - * or a completed response that has not fully drained. A streaming body - * the application is still producing (HTTP_END_CALLED clear, - * HTTP_RESPONSE_PENDING set) closes here so onAborted / request.signal - * fires on client disconnect. */ + /* Bun.serve: a response that is already fully determined must finish + * and drain before the connection shuts down (from the existing + * shouldCloseConnection() gates), not be cut off by the close() below. + * That is a tryEnd tail (the content-length path sets HTTP_END_CALLED + * while offset < total keeps HTTP_RESPONSE_PENDING), a file body the + * runtime is still delivering under a Content-Length that has already + * gone out (HTTP_FIXED_LENGTH_FILE_BODY), or a completed response that + * has not fully drained. A streaming body the application is still + * producing (neither bit, HTTP_RESPONSE_PENDING set) closes here so + * onAborted / request.signal fires on client disconnect. A deferred + * connection whose peer is really gone still closes: the kernel + * reports the reset, or the next write()/sendfile() fails (onWritable + * above, FileResponseStream). */ HttpResponseData *httpResponseData = (HttpResponseData *) us_socket_ext(s); uint32_t state = httpResponseData->state; - bool tryEndTail = (state & HttpResponseData::HTTP_END_CALLED) + bool determinedTail = (state & (HttpResponseData::HTTP_END_CALLED | HttpResponseData::HTTP_FIXED_LENGTH_FILE_BODY)) && (state & HttpResponseData::HTTP_RESPONSE_PENDING); bool doneButBuffered = !(state & HttpResponseData::HTTP_RESPONSE_PENDING) && !asyncSocket->hasFullyDrained(); - if (tryEndTail || doneButBuffered) { + if (determinedTail || doneButBuffered) { httpResponseData->state |= HttpResponseData::HTTP_NODE_RECEIVED_FIN; return s; } diff --git a/packages/bun-uws/src/HttpResponseData.h b/packages/bun-uws/src/HttpResponseData.h index c64ff4493f79..73f8aa476b3f 100644 --- a/packages/bun-uws/src/HttpResponseData.h +++ b/packages/bun-uws/src/HttpResponseData.h @@ -134,7 +134,8 @@ struct HttpResponseData : AsyncSocketData, HttpParser { * 'upgrade' listener's socket. */ HTTP_NODE_TUNNEL_AFTER_BODY = 1 << 14, /* The peer half-closed (FIN) while there was still something to flush - * before teardown: buffered/pinned response bytes, or (with + * before teardown: buffered/pinned response bytes, a file body still + * being delivered (HTTP_FIXED_LENGTH_FILE_BODY), or (with * httpAllowHalfOpen) an in-flight or queued pipelined response. The * connection is shut down from the shouldCloseConnection()/onWritable * gates once those have drained. Without httpAllowHalfOpen, onWritable @@ -151,6 +152,17 @@ struct HttpResponseData : AsyncSocketData, HttpParser { * shutdown sweep; the shouldCloseConnection() gates act on it once the * in-flight work completes. */ HTTP_CLOSE_WHEN_IDLE = 1 << 17, + /* The response in flight is a file of known size whose Content-Length + * is already on the wire, delivered by the runtime itself + * (FileResponseStream: sendfile() from the fd, or read()+write() + * chunks) rather than handed to uWS up front. Unlike a tryEnd tail + * nothing else in this object shows that such a response is already + * fully determined, so onEnd needs this bit to let it finish + * after a peer FIN instead of closing on it (which leaves the client a + * short body under that Content-Length). Per-response: cleared by + * resetResponseState(), moot once markDone() clears + * HTTP_RESPONSE_PENDING. */ + HTTP_FIXED_LENGTH_FILE_BODY = 1 << 18, /* Bits that describe the connection rather than the response in flight. * There is one HttpResponseData per socket, reused by every request on a diff --git a/src/runtime/server/FileResponseStream.rs b/src/runtime/server/FileResponseStream.rs index af51dd28e600..b2ee54d441ad 100644 --- a/src/runtime/server/FileResponseStream.rs +++ b/src/runtime/server/FileResponseStream.rs @@ -158,6 +158,9 @@ impl FileResponseStream { let resp = this_ref.resp.get(); resp.timeout(opts.idle_timeout); + if opts.length.is_some() { + resp.mark_fixed_length_file_body(); + } resp.on_aborted( |p: *mut FileResponseStream, r| { // SAFETY: uWS hands back the userdata pointer set below; the diff --git a/src/uws_sys/Response.rs b/src/uws_sys/Response.rs index d0650880c162..c68306ac723a 100644 --- a/src/uws_sys/Response.rs +++ b/src/uws_sys/Response.rs @@ -322,6 +322,11 @@ impl Response { c::uws_res_mark_wrote_content_length_header(Self::ssl_flag(), self.as_raw()) } + /// Sets `HTTP_FIXED_LENGTH_FILE_BODY`; see `HttpResponseData.h`. + pub(crate) fn mark_fixed_length_file_body(&mut self) { + c::uws_res_mark_fixed_length_file_body(Self::ssl_flag(), self.as_raw()) + } + pub(crate) fn mark_wrote_date_header(&mut self) { c::uws_res_mark_wrote_date_header(Self::ssl_flag(), self.as_raw()) } @@ -721,6 +726,11 @@ impl AnyResponse { any_dispatch!(self, |r| r.mark_wrote_content_length_header()) } + /// See `Response::mark_fixed_length_file_body`. + pub fn mark_fixed_length_file_body(self) { + any_dispatch!(self, |r| r.mark_fixed_length_file_body()) + } + pub fn mark_wrote_date_header(self) { any_dispatch!(self, |r| r.mark_wrote_date_header()) } @@ -1081,6 +1091,7 @@ pub mod c { // unsafe. unsafe extern "C" { pub(crate) safe fn uws_res_mark_wrote_content_length_header(ssl: i32, res: &mut uws_res); + pub(crate) safe fn uws_res_mark_fixed_length_file_body(ssl: i32, res: &mut uws_res); pub(crate) safe fn uws_res_mark_wrote_date_header(ssl: i32, res: &mut uws_res); pub(crate) safe fn uws_res_write_mark(ssl: i32, res: &mut uws_res); pub(crate) safe fn us_socket_mark_needs_more_not_ssl(socket: &mut uws_res); diff --git a/src/uws_sys/h3.rs b/src/uws_sys/h3.rs index da8962fa9ffa..425ace3408fb 100644 --- a/src/uws_sys/h3.rs +++ b/src/uws_sys/h3.rs @@ -145,6 +145,8 @@ impl Response { pub(crate) fn mark_wrote_content_length_header(&mut self) { c::uws_h3_res_mark_wrote_content_length_header(self) } + /// No-op: only the TCP peer-FIN handling reads this bit. + pub(crate) fn mark_fixed_length_file_body(&mut self) {} pub(crate) fn mark_wrote_date_header(&mut self) { c::uws_h3_res_mark_wrote_date_header(self) } diff --git a/src/uws_sys/libuwsockets.cpp b/src/uws_sys/libuwsockets.cpp index 89a62dcca2b1..c3d0c6570b91 100644 --- a/src/uws_sys/libuwsockets.cpp +++ b/src/uws_sys/libuwsockets.cpp @@ -1277,6 +1277,16 @@ extern "C" } } + void uws_res_mark_fixed_length_file_body(int ssl, uws_res_r res) { + if (ssl) { + uWS::HttpResponse *uwsRes = (uWS::HttpResponse *)res; + uwsRes->getHttpResponseData()->state |= uWS::HttpResponseData::HTTP_FIXED_LENGTH_FILE_BODY; + } else { + uWS::HttpResponse *uwsRes = (uWS::HttpResponse *)res; + uwsRes->getHttpResponseData()->state |= uWS::HttpResponseData::HTTP_FIXED_LENGTH_FILE_BODY; + } + } + void uws_res_mark_wrote_date_header(int ssl, uws_res_r res) { if (ssl) { uWS::HttpResponse *uwsRes = (uWS::HttpResponse *)res; diff --git a/test/js/bun/http/serve.test.ts b/test/js/bun/http/serve.test.ts index 342f62110092..0ec4c6ec03a7 100644 --- a/test/js/bun/http/serve.test.ts +++ b/test/js/bun/http/serve.test.ts @@ -4235,16 +4235,21 @@ it("survives aborted uploads while responding with a tee()d request-body branch" }); // A client that half-closes its write side right after the request (the raw -// socket.end(request) pattern) must receive every response byte already handed -// to uWS, not just what the kernel accepted on the first send. No -// Connection: close on the request: the post-drain shutdown is driven by the -// HTTP_NODE_RECEIVED_FIN clause of shouldCloseConnection(), not by -// HTTP_CONNECTION_CLOSE. +// socket.end(request) pattern) must receive the whole of a response whose +// length is already settled: a body handed to uWS up front, or a file body the +// runtime streams itself under a Content-Length it has already sent. Not just +// what the kernel accepted on the first send. No Connection: close on the +// request: the post-drain shutdown is driven by the HTTP_NODE_RECEIVED_FIN +// clause of shouldCloseConnection(), not by HTTP_CONNECTION_CLOSE. describe("a client half-close after the request does not truncate a large response body", () => { const BODY = 8 * 1024 * 1024; + // The advertised Content-Length, the body bytes actually delivered, and + // whether the server closed cleanly (FIN rather than a reset). + const WHOLE_RESPONSE = { contentLength: BODY, body: BODY, ended: true }; + function countBody(socket: net.Socket | nodeTls.TLSSocket) { - const out = { body: 0, ended: false }; + const out = { contentLength: -1, body: 0, ended: false }; let head = ""; let gotHead = false; socket.on("data", chunk => { @@ -4253,6 +4258,7 @@ describe("a client half-close after the request does not truncate a large respon const i = head.indexOf("\r\n\r\n"); if (i >= 0) { gotHead = true; + out.contentLength = Number(/^content-length:\s*(\d+)/im.exec(head.slice(0, i))?.[1] ?? -1); out.body = Buffer.byteLength(head.slice(i + 4), "latin1"); } } else { @@ -4264,22 +4270,28 @@ describe("a client half-close after the request does not truncate a large respon return out; } - async function halfCloseRequest(port: number): Promise<{ body: number; ended: boolean }> { - const socket = connect(port, "127.0.0.1"); + async function halfCloseRequest(port: number, secure = false): Promise { + const socket = secure + ? nodeTls.connect({ port, host: "127.0.0.1", rejectUnauthorized: false }) + : connect(port, "127.0.0.1"); const out = countBody(socket); const closed = new Promise(r => socket.once("close", () => r())); - await new Promise(r => socket.once("connect", () => r())); + await new Promise(r => socket.once(secure ? "secureConnect" : "connect", () => r())); socket.end("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n"); await closed; return out; } + function bigFile(prefix: string) { + return tempDir(prefix, { "big.bin": Buffer.alloc(BODY, "a") }); + } + it("fetch handler (tryEnd tail)", async () => { using server = serve({ port: 0, fetch: () => new Response(Buffer.alloc(BODY, "a"), { headers: { "content-length": String(BODY) } }), }); - expect(await halfCloseRequest(server.port)).toEqual({ body: BODY, ended: true }); + expect(await halfCloseRequest(server.port)).toEqual(WHOLE_RESPONSE); }); it("static route (tryEnd tail)", async () => { @@ -4290,7 +4302,7 @@ describe("a client half-close after the request does not truncate a large respon }, fetch: () => new Response("miss", { status: 404 }), }); - expect(await halfCloseRequest(server.port)).toEqual({ body: BODY, ended: true }); + expect(await halfCloseRequest(server.port)).toEqual(WHOLE_RESPONSE); }); it("https fetch handler (tryEnd tail)", async () => { @@ -4299,13 +4311,36 @@ describe("a client half-close after the request does not truncate a large respon tls, fetch: () => new Response(Buffer.alloc(BODY, "a"), { headers: { "content-length": String(BODY) } }), }); - const socket = nodeTls.connect({ port: server.port, host: "127.0.0.1", rejectUnauthorized: false }); - const out = countBody(socket); - const closed = new Promise(r => socket.once("close", () => r())); - await new Promise(r => socket.once("secureConnect", () => r())); - socket.end("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n"); - await closed; - expect(out).toEqual({ body: BODY, ended: true }); + expect(await halfCloseRequest(server.port, true)).toEqual(WHOLE_RESPONSE); + }); + + // A file body is not handed to uWS up front: FileResponseStream writes the + // Content-Length, then moves the bytes itself (sendfile() on Linux over plain + // TCP, read()+write() chunks over TLS and on the other platforms), so uWS + // only knows the response is settled because the stream marks it as such + // (HTTP_FIXED_LENGTH_FILE_BODY). Without that mark the FIN closed the socket + // mid-transfer: a 200 with Content-Length: 8388608 followed by a few MiB and + // then the server's FIN. + it("fetch handler returning Bun.file() (file body)", async () => { + using dir = bigFile("half-close-file"); + using server = serve({ port: 0, fetch: () => new Response(file(join(String(dir), "big.bin"))) }); + expect(await halfCloseRequest(server.port)).toEqual(WHOLE_RESPONSE); + }); + + it("Bun.file() route (file body)", async () => { + using dir = bigFile("half-close-file-route"); + using server = serve({ + port: 0, + routes: { "/": file(join(String(dir), "big.bin")) }, + fetch: () => new Response("miss", { status: 404 }), + }); + expect(await halfCloseRequest(server.port)).toEqual(WHOLE_RESPONSE); + }); + + it("https fetch handler returning Bun.file() (file body)", async () => { + using dir = bigFile("half-close-file-tls"); + using server = serve({ port: 0, tls, fetch: () => new Response(file(join(String(dir), "big.bin"))) }); + expect(await halfCloseRequest(server.port, true)).toEqual(WHOLE_RESPONSE); }); // The deferred connection must close promptly, not spin the writable @@ -4337,9 +4372,38 @@ describe("a client half-close after the request does not truncate a large respon expect(server.pendingRequests).toBe(0); }); + // Same for a deferred file body. It is not covered by onWritable's + // zero-progress check (its progress never shows up in the uWS write offset, + // which is why that check is scoped to tryEnd tails): the peer's reset is + // reported by the kernel, or the next sendfile()/write() fails and the + // stream tears the request down itself. Either way the request must not sit + // there until idleTimeout. + it("a file body closes without spinning when the peer goes away mid-transfer", async () => { + using dir = bigFile("half-close-file-peer-gone"); + const dispatched = Promise.withResolvers(); + using server = serve({ + port: 0, + idleTimeout: 60, + fetch() { + dispatched.resolve(); + return new Response(file(join(String(dir), "big.bin"))); + }, + }); + const socket = connect(server.port, "127.0.0.1"); + socket.on("error", () => {}); + await new Promise(r => socket.once("connect", () => r())); + socket.end("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n"); + socket.once("data", () => socket.destroy()); + await dispatched.promise; + const deadline = Date.now() + 4000; + while (server.pendingRequests > 0 && Date.now() < deadline) await Bun.sleep(5); + expect(server.pendingRequests).toBe(0); + }); + // The defer in onEnd is gated on the response being fully determined - // (HTTP_END_CALLED). A streaming body the handler is still producing must - // close on client FIN so onAborted / request.signal fires. + // (HTTP_END_CALLED or HTTP_FIXED_LENGTH_FILE_BODY). A streaming body the + // handler is still producing must close on client FIN so onAborted / + // request.signal fires. it("request.signal still fires on client FIN for a streaming body", async () => { const aborted = Promise.withResolvers(); using server = serve({