diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index cd7327a0ec0f..850ee0bf43b3 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -1000,24 +1000,15 @@ where // SAFETY: FFI handle, just checked Some let has_responded = resp.has_responded(); - // The status line is already committed (a direct ReadableStream's - // pull() threw synchronously after do_render_stream wrote headers). - // error() cannot replace a response whose status is on the wire; - // report the failure and terminate the body so the client observes an - // incomplete message instead of a second header block spliced into - // the chunked body. + // The status line is already committed (a direct stream's pull() threw + // synchronously after the headers were written): report and close. if !has_responded && self.flags.has_written_status() { if !value.is_empty_or_undefined_or_null() && let Some(server) = self.server.get() { server.vm().as_mut().run_error_handler(value, None); } - let state = resp.state(); - if state.is_http_write_called() && state.is_response_pending() { - self.force_close(); - } else { - self.end_stream(self.should_close_connection()); - } + self.close_incomplete_stream(); return; } @@ -1290,6 +1281,18 @@ where } } + /// Closes a response whose body failed after the status line was committed. + /// Never the terminating chunk: it would make a truncated body look complete. + pub(crate) fn close_incomplete_stream(&self) { + if let Some(resp) = self.resp.get() { + if resp.state().is_response_pending() { + self.force_close(); + return; + } + } + self.end_stream(self.should_close_connection()); + } + fn on_writable_complete_response_buffer( this: *mut Self, write_offset: u64, @@ -3001,50 +3004,9 @@ where self.render_metadata(); } - if DEBUG_MODE { - if let Some(server) = self.server.get() { - if !err.is_empty_or_undefined_or_null() { - let server = &*server; - let mut exception_list: jsc::ExceptionList = Vec::new(); - server - .vm() - .as_mut() - .run_error_handler(err, Some(&mut exception_list)); - - // The fallback page below writes into `resp`, which must - // not be dereferenced once the sink has already ended the - // response (see `end_already_responded_stream`). - if !ended_response && server.dev_server().is_some() { - // Render the error fallback HTML page like renderDefaultError does - if !self.flags.has_written_status() { - self.flags.set_has_written_status(true); - if let Some(resp) = self.resp.get() { - resp.write_status(b"500 Internal Server Error"); - resp.write_header( - b"content-type", - &bun_http_types::MimeType::HTML.value, - ); - } - } - - let bb = DevErrorPage { - message: b"Stream error during server-side rendering", - cwd: bun_resolver::fs::FileSystem::get().top_level_dir, - exceptions: &exception_list, - log: None, - } - .render(); - - if let Some(resp) = self.resp.get() { - // SAFETY: FFI handle - resp.write(&bb); - } - - self.end_stream(self.should_close_connection()); - return; - } - } - } + // Production mode keeps this asynchronous JS path quiet. + if DEBUG_MODE && self.report_committed_body_error(err, !ended_response) { + return; } // HTTP/1 only: the sink already fully ended the response, so `resp` // can no longer be dereferenced (see `end_already_responded_stream`). @@ -3054,17 +3016,43 @@ where self.end_already_responded_stream(); return; } - // Body bytes were already written: close without the terminating chunk - // (RFC 9112 section 7) so the client sees an incomplete message, not a - // truncated body that looks like a complete, successful response. + self.close_incomplete_stream(); + } + + /// Reports a body failure that arrived after the status line was + /// committed. Under a bake dev server the error page ends the response: + /// returns `true`, and `resp` must not be touched again. + fn report_committed_body_error(&self, err: JSValue, resp_writable: bool) -> bool { + if err.is_empty_or_undefined_or_null() { + return false; + } + let server = self.server(); + if !DEBUG_MODE || !resp_writable || server.dev_server().is_none() { + server.vm().as_mut().run_error_handler(err, None); + return false; + } + + let mut exception_list: jsc::ExceptionList = Vec::new(); + server + .vm() + .as_mut() + .run_error_handler(err, Some(&mut exception_list)); + + let bb = DevErrorPage { + message: b"Stream error during server-side rendering", + cwd: bun_resolver::fs::FileSystem::get().top_level_dir, + exceptions: &exception_list, + log: None, + } + .render(); + if let Some(resp) = self.resp.get() { - let state = resp.state(); - if state.is_http_write_called() && state.is_response_pending() { - self.force_close(); - return; - } + // SAFETY: FFI handle + resp.write(&bb); } + self.end_stream(self.should_close_connection()); + true } pub(crate) fn do_render_with_body( @@ -3353,13 +3341,13 @@ where this.run_error_handler(js_err); return; } - if let Some(resp) = this.resp.get() { - let state = resp.state(); - if state.is_http_write_called() && state.is_response_pending() { - this.force_close(); - return; - } + // Committed status: report in both modes, then close. + let global_this = this.server().global_this(); + if this.report_committed_body_error(err.to_js(global_this), true) { + return; } + this.close_incomplete_stream(); + return; } else if !this.flags.has_written_status() { // Upstream ended cleanly before any chunk: flush the deferred // status/headers so the client sees them before the terminator. diff --git a/test/js/bun/http/async-iterator-stream.test.ts b/test/js/bun/http/async-iterator-stream.test.ts index e6412d841691..537f30cc3000 100644 --- a/test/js/bun/http/async-iterator-stream.test.ts +++ b/test/js/bun/http/async-iterator-stream.test.ts @@ -114,10 +114,17 @@ describe.concurrent("Streaming body via", () => { expect(exitCode).toBe(0); }); - test("async generator function throws an error but continues to send the headers", async () => { + // The generator fails before producing any chunk. The status and headers + // were already committed to uWS, so the server closes the connection without + // a complete response instead of sending them with an empty body and a clean + // chunked terminator. + test("async generator function throws an error before its first chunk closes the connection", async () => { + let outcome: string | undefined; const onMessage = mock(async url => { - const response = await fetch(url); - expect(response.headers.get("X-Hey")).toBe("123"); + outcome = await fetch(url).then( + response => `resolved ${response.status}`, + (err: any) => `rejected ${err.code}`, + ); subprocess?.kill(); }); @@ -132,6 +139,7 @@ describe.concurrent("Streaming body via", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); + expect(outcome).toBe("rejected ECONNRESET"); expect(stderr).toContain("error: Oops"); expect(onMessage).toHaveBeenCalledTimes(1); }); diff --git a/test/js/bun/http/readable-stream-throws.fixture.js b/test/js/bun/http/readable-stream-throws.fixture.js index ff32f9450787..7fc86669fad7 100644 --- a/test/js/bun/http/readable-stream-throws.fixture.js +++ b/test/js/bun/http/readable-stream-throws.fixture.js @@ -2,6 +2,7 @@ const server = Bun.serve({ port: 0, idleTimeout: 0, error(err) { + console.error("error handler called"); return new Response("Failed", { status: 555 }); }, diff --git a/test/js/bun/http/serve-direct-readable-stream.test.ts b/test/js/bun/http/serve-direct-readable-stream.test.ts index 1e36b5aeb619..9179b054dca9 100644 --- a/test/js/bun/http/serve-direct-readable-stream.test.ts +++ b/test/js/bun/http/serve-direct-readable-stream.test.ts @@ -793,7 +793,7 @@ describe("sync pull() throw after status is written does not re-render error()", expect(exitCode).toBe(0); }); - test("no body bytes flushed: stream is ended without splicing error() headers", async () => { + test("no body bytes flushed: connection is force-closed without splicing error() headers", async () => { await using proc = Bun.spawn({ cmd: [bunExe(), "-e", fixture(`throw new Error("boom");`)], env: bunEnv, @@ -803,11 +803,12 @@ describe("sync pull() throw after status is written does not re-render error()", const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(stderr).toContain("error: boom"); const { wire, errorHandlerCalls } = JSON.parse(stdout); - // Status 200 was already written; the stream is ended empty. The error() - // response's status (500), headers, and body must not appear on the wire. - expect(wire).not.toContain("x-err"); - expect(wire).not.toContain("FROM-ERROR-HANDLER"); - expect(wire.startsWith("HTTP/1.1 200 OK\r\n")).toBe(true); + // Status 200 was already written to the corked response, so error() cannot + // replace it. Ending the stream here would send that 200 with an empty + // chunked body and a clean terminator: a complete-looking response for a + // body that failed. The connection is closed instead, and since the status + // never left the cork buffer the client sees an empty reply. + expect(wire).toBe(""); expect(errorHandlerCalls).toBe(0); expect(exitCode).toBe(0); }); diff --git a/test/js/bun/http/serve-stream-body-error-fixture.html b/test/js/bun/http/serve-stream-body-error-fixture.html new file mode 100644 index 000000000000..f75efe4b1148 --- /dev/null +++ b/test/js/bun/http/serve-stream-body-error-fixture.html @@ -0,0 +1,3 @@ + + +dev server page diff --git a/test/js/bun/http/serve-stream-body-error-fixture.ts b/test/js/bun/http/serve-stream-body-error-fixture.ts index 41aec508a0e3..25e9ded60a14 100644 --- a/test/js/bun/http/serve-stream-body-error-fixture.ts +++ b/test/js/bun/http/serve-stream-body-error-fixture.ts @@ -1,22 +1,47 @@ // An error thrown by a ReadableStream used as a Response body must never // escape as a global unhandledRejection. Bun's default unhandledRejection // policy exits the process, so before the fix a single bad request took the -// entire server down. +// entire server down. The `nativeBodies` variants pin the same wire and +// reporting contract for bodies Bun.serve streams without a JS ReadableStream. // -// argv[2]: variant name (see `sources` below) -// argv[3]: "development" to run the server with development: true +// argv[2]: variant name (see `sources` and `nativeBodies` below) +// argv[3..]: "development" to run the server with development: true; +// "dev-server" to also give it an HTML route, which makes the +// development server run a bake dev server (the requests below +// still go to fetch()) // // Prints a single JSON object on stdout and exits 0. import net from "node:net"; +import devServerPage from "./serve-stream-body-error-fixture.html"; const variant = process.argv[2]; -const development = process.argv[3] === "development"; +const flags = process.argv.slice(3); +const development = flags.includes("development"); +const devServer = flags.includes("dev-server"); -// Set by the mid-stream variant so it only errors once its chunk has provably -// reached the client socket, i.e. after the 200 response is committed on the -// wire. Called from the raw request's data handler below. +// Set by the mid-stream variants so they only error once their chunk has +// provably reached the client socket, i.e. after the 200 response is committed +// on the wire. Called from the raw request's data handler below. let midStreamResolve: (() => void) | undefined; +// Arms `midStreamResolve` for one request. Every mid-stream variant calls this +// before it emits "chunk-a", so the resolver is in place by the time the +// client can see the chunk. +function chunkReachedClient(): Promise { + const { promise, resolve } = Promise.withResolvers(); + midStreamResolve = resolve; + return promise; +} + +// Set by the variants that fail after the status line but before any body byte +// (pending-error-after-headers, proxied-fetch-after-status) so they only error +// once the status line has provably reached the client socket. (uWS terminates +// the header block only with the first body byte, so a blank line never +// arrives for these variants.) The forced close that follows sends a RST, and +// a RST can discard data the peer has not read yet (Windows always does), so +// the error must not race the client's read. +let statusLineReceivedResolve: (() => void) | undefined; + // Set by the cancel variants: resolved once the source's cancel() has run, so // the test observes any resulting rejection before declaring success. const cancelRan = Promise.withResolvers(); @@ -62,14 +87,24 @@ const sources: Record ReadableStream> = { }, { highWaterMark: 0 }, ), + // The first pull() is pending when the server flushes the headers; the + // source then errors without ever producing a chunk. + "pending-error-after-headers": () => + new ReadableStream({ + async pull(c) { + const { promise, resolve } = Promise.withResolvers(); + statusLineReceivedResolve = resolve; + await promise; + c.error(new Error("boom")); + }, + }), // Errors only after a chunk has already been flushed to the client. "mid-stream-reject": () => new ReadableStream({ async pull(c) { - const { promise, resolve } = Promise.withResolvers(); - midStreamResolve = resolve; + const reached = chunkReachedClient(); c.enqueue("chunk-a"); - await promise; + await reached; throw new Error("boom"); }, }), @@ -112,8 +147,92 @@ const sources: Record ReadableStream> = { }), }; +// HTMLRewriter input that lets the rewritten "chunk-a" reach the client +// before delivering the element whose handler fails. +function rewriterInput(): ReadableStream { + const reached = chunkReachedClient(); + let pulls = 0; + return new ReadableStream({ + async pull(c) { + if (pulls++ === 0) { + c.enqueue("chunk-a"); + return; + } + await reached; + c.enqueue("

x

"); + c.close(); + }, + }); +} + +function rewriterResponse(element: () => void | Promise): Response { + return new HTMLRewriter() + .on("p", { element }) + .transform(new Response(rewriterInput(), { headers: { "content-type": "text/html" } })); +} + +// Upstream for the proxied variants: a scratch server for one request that +// answers with `firstWrite` (announcing more body than it will ever send), so +// the fetch() body fails deterministically when `fail()` closes the connection. +async function fetchFromFailingUpstream(firstWrite: string) { + const sockets: net.Socket[] = []; + const upstream = net.createServer(socket => { + sockets.push(socket); + socket.resume(); + socket.on("error", () => {}); + socket.write(firstWrite); + }); + await new Promise(resolve => upstream.listen(0, "127.0.0.1", resolve)); + // Must never be what keeps the process alive if fail() is not reached. + upstream.unref(); + const { port } = upstream.address() as net.AddressInfo; + const res = await fetch(`http://127.0.0.1:${port}/`); + return { + res, + fail() { + for (const socket of sockets) socket.end(); + upstream.close(); + }, + }; +} + +// Bodies Bun.serve streams natively (no JS ReadableStream pump) whose producer +// fails after the status line is committed. error() can no longer answer, so +// the failure is reported and the connection is closed without the terminating +// chunk, whether or not a body byte went out first. +const nativeBodies: Record Response | Promise> = { + "rewriter-mid-stream-throw": () => + rewriterResponse(() => { + throw new Error("boom"); + }), + "rewriter-mid-stream-reject": () => + rewriterResponse(async () => { + await Bun.sleep(0); + throw new Error("boom"); + }), + // `return fetch(...)` whose upstream dies once its first chunk has made it + // through to our client. + "proxied-fetch-mid-stream": async () => { + const reached = chunkReachedClient(); + const { res, fail } = await fetchFromFailingUpstream("HTTP/1.1 200 OK\r\nContent-Length: 100\r\n\r\nchunk-a"); + reached.then(fail); + return res; + }, + // Same, but the upstream dies before producing a single body byte. Bun.serve + // commits the status as soon as it attaches to a fetch() body, so the client + // has the status line and then the connection closes with no body at all. + "proxied-fetch-after-status": async () => { + const { promise, resolve } = Promise.withResolvers(); + statusLineReceivedResolve = resolve; + const { res, fail } = await fetchFromFailingUpstream("HTTP/1.1 200 OK\r\nContent-Length: 100\r\n\r\n"); + promise.then(fail); + return res; + }, +}; + const source = sources[variant]; -if (!source) { +const nativeBody = nativeBodies[variant]; +if (!source && !nativeBody) { console.error(`unknown variant: ${variant}`); process.exit(3); } @@ -131,28 +250,37 @@ let errorCb = 0; await using server = Bun.serve({ port: 0, development, + ...(devServer ? { routes: { "/dev-server-page": devServerPage } } : {}), error() { errorCb++; return new Response("err-body", { status: 500 }); }, - fetch() { - return new Response(source()); + fetch(req) { + if (new URL(req.url).pathname === "/ok") { + return new Response("ok"); + } + return nativeBody ? nativeBody() : new Response(source()); }, }); // Sends one request over a raw socket and returns everything received before // the server closed the connection, so the test can assert on the HTTP // framing. A forced close (ECONNRESET) is an expected, asserted-on outcome -// for the mid-stream variant, so socket errors are not fatal. -function rawRequest(abort: boolean): Promise { +// for the erroring variants, so socket errors are not fatal. +function rawRequest(path: string, abort: boolean): Promise { const chunks: Buffer[] = []; return new Promise(resolve => { const sock = net.connect(server.port, "127.0.0.1", () => { - sock.write("GET / HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n"); + sock.write(`GET ${path} HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n`); }); sock.on("data", d => { chunks.push(d); - if (Buffer.concat(chunks).includes("chunk-a")) { + const received = Buffer.concat(chunks); + if (received.includes("\r\n")) { + statusLineReceivedResolve?.(); + statusLineReceivedResolve = undefined; + } + if (received.includes("chunk-a")) { midStreamResolve?.(); // The cancel variants tear down the socket once a body chunk has // provably reached the client, so the server's onAborted path fires @@ -165,12 +293,11 @@ function rawRequest(abort: boolean): Promise { }); } -const wire = await rawRequest(clientAborts); +const wire = await rawRequest("/", clientAborts); if (clientAborts) await cancelRan.promise; -// A second request proves the server is still accepting and answering. For the -// cancel variants the body stream never self-terminates, so abort that one too; -// only the status line is asserted. -const secondWire = await rawRequest(clientAborts); +// A second request to a healthy route proves the server is still accepting +// and answering; only its status line is asserted. +const secondWire = await rawRequest("/ok", false); // Cycle the event loop so any stray rejected promise reaches the // unhandledRejection reporter before we declare success. diff --git a/test/js/bun/http/serve-stream-body-error.test.ts b/test/js/bun/http/serve-stream-body-error.test.ts index c0e5635ea7bd..80177caa5f09 100644 --- a/test/js/bun/http/serve-stream-body-error.test.ts +++ b/test/js/bun/http/serve-stream-body-error.test.ts @@ -1,5 +1,6 @@ // A `Response` body backed by a `ReadableStream` whose source errors must not -// take the whole server down. Before the fix: +// take the whole server down, and must never reach the client as a complete +// message. Before the fixes: // 1. The rejection from the stream pump was never given a handler, so it // reached the global unhandledRejection reporter, whose default policy // exits the process: one bad request was a whole-process outage. @@ -8,6 +9,13 @@ // 3. A body that errored after chunks were already sent was still terminated // with a clean `0\r\n\r\n`, so the client could not tell the truncated // body apart from a complete one. +// 4. A body that errored before any chunk was sent went out as the +// Response's status and headers plus an empty chunked body with a clean +// terminator: a complete, successful-looking response for a body that +// failed. +// The natively streamed bodies further down (HTMLRewriter, proxied fetch) pin +// the same contract for the non-ReadableStream path, which also dropped the +// producer's error without reporting it. import { expect, test } from "bun:test"; import { bunEnv, bunExe, isASAN } from "harness"; import { join } from "node:path"; @@ -25,50 +33,132 @@ async function runFixture(variant: string, ...extra: string[]) { return { stdout, stderr, exitCode }; } -// The wire-level contract for a stream source that errors before producing any -// body bytes is unchanged: the Response's own status and headers go out with -// an empty chunked body and the server `error()` callback is NOT invoked (see -// "throw on pull renders headers, does not call error handler" in -// serve.test.ts). What changes is that the rejection no longer reaches the -// unhandledRejection reporter; it is reported directly instead. +// A stream source that errors before producing any body bytes. The status and +// headers were already handed to uWS (corked) when the error arrives, so the +// server `error()` callback cannot supply a replacement and is NOT invoked. +// The connection is closed before anything reaches the wire: the client sees +// an empty reply, never a complete 200 with an empty body. The rejection does +// not reach the unhandledRejection reporter; it is reported directly instead. test.concurrent.each([ "pull-throw", "pull-async-reject", "controller-error", "start-async-reject", "deferred-pull-throw", -])("%s: the rejection is handled and the error is still reported", async variant => { +])("%s: the connection is closed without a complete response and the error is reported", async variant => { const { stdout, stderr, exitCode } = await runFixture(variant); - expect({ result: JSON.parse(stdout), exitCode }).toEqual({ - result: { - statusLine: "HTTP/1.1 200 OK", - cleanChunkedTerminator: true, - body: "0\r\n\r\n", - errorCb: 0, - unhandled: 0, - secondStatusLine: "HTTP/1.1 200 OK", - }, - exitCode: 0, + expect(JSON.parse(stdout)).toEqual({ + statusLine: "", + cleanChunkedTerminator: false, + body: "", + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", }); // With `development: false` the unhandledRejection report used to be the // only place the error surfaced; it must still reach stderr without it. expect(stderr).toContain("boom"); + expect(exitCode).toBe(0); }); // Same under `development: true` (the DEBUG RequestContext monomorphization). -test.concurrent("pull-throw in development mode: the rejection is handled", async () => { +test.concurrent("pull-throw in development mode: the connection is closed without a complete response", async () => { const { stdout, stderr, exitCode } = await runFixture("pull-throw", "development"); - expect({ result: JSON.parse(stdout), exitCode }).toEqual({ + expect(JSON.parse(stdout)).toEqual({ + statusLine: "", + cleanChunkedTerminator: false, + body: "", + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", + }); + expect(stderr).toContain("boom"); + expect(exitCode).toBe(0); +}); + +// The headers are already on the wire (the first pull() was still pending +// when the server flushed them) and the source then errors without ever +// producing a chunk. The 200 is irrevocable, so the connection is closed +// without the terminating chunk: headers, then an incomplete body. +// +// No stderr assertion: see the mid-stream test below for why this path is +// quiet with `development: false`. +test.concurrent("pending-error-after-headers: headers go out, the body is not terminated as complete", async () => { + const { stdout, exitCode } = await runFixture("pending-error-after-headers"); + expect(JSON.parse(stdout)).toEqual({ + statusLine: "HTTP/1.1 200 OK", + cleanChunkedTerminator: false, + body: "", + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", + }); + expect(exitCode).toBe(0); +}); + +// Bodies that Bun.serve streams natively (an HTMLRewriter transform, a proxied +// fetch() Response) reach the server through RequestContext::end_chunk rather +// than handle_reject_stream. Their status is committed before the body starts, +// so error() cannot answer. Before the fix end_chunk dropped the error (a +// rewriter handler that threw after the first byte left no trace anywhere) and +// a producer that failed before its first chunk went out as a clean, empty 200. +// Now the failure is reported in both modes and the connection is closed +// without the terminating chunk, with whatever body bytes were already out. +// +// [variant, body bytes on the wire, what the report must name] +const nativeBodies = [ + ["rewriter-mid-stream-throw", "e\r\nchunk-a\r\n", "boom"], + ["rewriter-mid-stream-reject", "e\r\nchunk-a\r\n", "boom"], + ["proxied-fetch-mid-stream", "7\r\nchunk-a\r\n", "ECONNRESET"], + ["proxied-fetch-after-status", "", "ECONNRESET"], +] as const; + +for (const flags of [[], ["development"]]) { + const mode = flags.length ? "development" : "production"; + test.concurrent.each(nativeBodies)( + `%s in ${mode} mode: reported, and the body is not terminated as complete`, + async (variant, body, reported) => { + const { stdout, stderr, exitCode } = await runFixture(variant, ...flags); + expect({ result: JSON.parse(stdout), exitCode }).toEqual({ + result: { + statusLine: "HTTP/1.1 200 OK", + cleanChunkedTerminator: false, + body, + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", + }, + exitCode: 0, + }); + expect(stderr).toContain(reported); + }, + ); +} + +// When the development server also runs a bake dev server (it has an HTML +// route), the report additionally renders the dev error page as the rest of +// the body and ends the response normally, so the browser shows the error +// after whatever was already streamed. The JS stream has always taken this +// branch; the native body now goes through the same code. +test.concurrent.each([ + ["rewriter-mid-stream-throw", "e\r\nchunk-a\r\n"], + ["mid-stream-reject", "7\r\nchunk-a\r\n"], +])("%s under a dev server: the error page is appended to the body", async (variant, firstChunk) => { + const { stdout, stderr, exitCode } = await runFixture(variant, "development", "dev-server"); + const { body, ...result } = JSON.parse(stdout); + expect({ result, exitCode }).toEqual({ result: { statusLine: "HTTP/1.1 200 OK", cleanChunkedTerminator: true, - body: "0\r\n\r\n", errorCb: 0, unhandled: 0, secondStatusLine: "HTTP/1.1 200 OK", }, exitCode: 0, }); + expect(body).toStartWith(firstChunk); + expect(body).toContain("Stream error during server-side rendering"); + expect(body).toContain("boom"); expect(stderr).toContain("boom"); }); @@ -218,16 +308,21 @@ test.concurrent("a stream body error does not kill the server process", async () `const server = Bun.serve({ port: 0, development: false, - fetch() { + fetch(req) { + if (new URL(req.url).pathname === "/ok") return new Response("ok"); return new Response(new ReadableStream({ pull() { throw new Error("boom"); } })); }, }); - const res = await fetch(server.url); - await res.arrayBuffer(); + // The failed body closes the connection without a complete response. + const first = await fetch(server.url).then( + res => res.arrayBuffer().then(() => "complete"), + err => "rejected " + err.code, + ); // Tick the event loop past the unhandledRejection checkpoint that used // to exit the process before this line was reached. for (let i = 0; i < 10; i++) await Bun.sleep(0); - console.log("alive", res.status); + const res = await fetch(new URL("/ok", server.url)); + console.log("alive", first, res.status); server.stop(true);`, ], env: bunEnv, @@ -235,9 +330,10 @@ test.concurrent("a stream body error does not kill the server process", async () stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect({ stdout, exitCode }).toEqual({ stdout: "alive 200\n", exitCode: 0 }); + expect(stdout).toBe("alive rejected ECONNRESET 200\n"); // The error must still be surfaced to the operator. expect(stderr).toContain("boom"); + expect(exitCode).toBe(0); }); // An already-rejected async fetch handler reaches handle_reject synchronously diff --git a/test/js/bun/http/serve.test.ts b/test/js/bun/http/serve.test.ts index 83ed8ab21198..1cce88860437 100644 --- a/test/js/bun/http/serve.test.ts +++ b/test/js/bun/http/serve.test.ts @@ -622,12 +622,17 @@ it.each([ describe("streaming", () => { describe("error handler", () => { - it("throw on pull renders headers, does not call error handler", async () => { + // The body source fails before any byte is written. The Response's status + // and headers are already committed to uWS, so error() cannot replace them + // and is not called; the connection is closed without a complete response + // so the client cannot mistake the failed body for an empty one. + it("throw on pull closes the connection, does not call error handler", async () => { + let outcome: string | undefined; const onMessage = mock(async url => { - const response = await fetch(url); - expect(response.status).toBe(402); - expect(response.headers.get("X-Hey")).toBe("123"); - expect(response.text()).resolves.toBe(""); + outcome = await fetch(url).then( + response => `resolved ${response.status}`, + (err: any) => `rejected ${err.code}`, + ); subprocess.kill(); }); @@ -642,17 +647,23 @@ describe("streaming", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); + expect(outcome).toBe("rejected ECONNRESET"); expect(stderr).toContain("error: Oops"); + expect(stderr).not.toContain("error handler called"); expect(onMessage).toHaveBeenCalled(); }); - it("throw on pull after writing should not call the error handler", async () => { + // pull() queues two chunks, requests close, then throws: per the streams + // spec the error wins and the queued chunks are discarded, so this is the + // same "failed before any byte" case as above. + it("throw on pull after writing closes the connection, does not call the error handler", async () => { + let outcome: string | undefined; const onMessage = mock(async href => { const url = new URL("write", href); - const response = await fetch(url); - expect(response.status).toBe(402); - expect(response.headers.get("X-Hey")).toBe("123"); - expect(response.text()).resolves.toBe(""); + outcome = await fetch(url).then( + response => `resolved ${response.status}`, + (err: any) => `rejected ${err.code}`, + ); subprocess.kill(); }); @@ -667,7 +678,9 @@ describe("streaming", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); + expect(outcome).toBe("rejected ECONNRESET"); expect(stderr).toContain("error: Oops"); + expect(stderr).not.toContain("error handler called"); expect(onMessage).toHaveBeenCalled(); }); diff --git a/test/js/web/encoding/text-encoder-stream.test.ts b/test/js/web/encoding/text-encoder-stream.test.ts index d37ff1663d23..d35e1b465c58 100644 --- a/test/js/web/encoding/text-encoder-stream.test.ts +++ b/test/js/web/encoding/text-encoder-stream.test.ts @@ -408,7 +408,10 @@ describe.skipIf(!isASAN)("a failed output buffer allocation errors the stream in }); const results = {}; for (const name of Object.keys(inputs)) { - await fetch(new URL(name, server.url)).then(response => response.arrayBuffer(), () => {}); + // The failed body closes the connection without a complete response: + // fetch() itself or the body read rejects, depending on whether the + // headers were already flushed. + await fetch(new URL(name, server.url)).then(response => response.arrayBuffer()).catch(() => {}); results[name] = await cancelReasons[name]; } results.afterwards = Array.from(await (await fetch(new URL("small", server.url))).bytes());