Repository navigation
Close the connection when a Bun.file response ends short of its Content-Length #41622
+230
−25
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,21 @@ | ||
| import type { Server } from "bun"; | ||
| import { afterAll, beforeAll, describe, expect, it, mock, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isASAN, isLinux, isWindows, rmScope, rss, tempDir, tempDirWithFiles, tls } from "harness"; | ||
| import { mkfifo } from "mkfifo"; | ||
| import { | ||
| closeSync, | ||
| openSync, | ||
| readFileSync, | ||
| statSync, | ||
| truncateSync, | ||
| unlinkSync, | ||
| utimesSync, | ||
| writeFileSync, | ||
| writeSync, | ||
| } from "node:fs"; | ||
| import { connect as netConnect } from "node:net"; | ||
| import { join } from "node:path"; | ||
| import { connect as tlsConnect } from "node:tls"; | ||
|
|
||
| const LARGE_SIZE = 1024 * 1024 * 8; | ||
| const files = { | ||
|
|
@@ -1454,6 +1466,178 @@ test("file routes frame a slice that reaches or starts past EOF by the bytes the | |
| }); | ||
| }); | ||
|
|
||
| // Bun.serve writes Content-Length for a Bun.file body from the fstat it takes | ||
| // when the response starts. When the file then yields fewer bytes, the | ||
| // response used to end like a complete one and the connection stayed open: | ||
| // the client waited for the missing bytes until the idle timeout (forever | ||
| // with idleTimeout: 0). A body that ends short of the committed length must | ||
| // take the connection down with it: the bytes that exist, then the end. | ||
| // | ||
| // The end of the file reaches the server in three shapes: sendfile(2) returns | ||
| // 0 (plain TCP on Linux, bodies of 1 MiB and up), the reader reports the end | ||
| // with no data (the file now ends before the reader's offset), or the reader | ||
| // delivers a last short chunk with the end. | ||
| describe("Bun.file body that ends short of its Content-Length", () => { | ||
| // A wait that never ends is how these tests fail, and then nothing scoped | ||
| // to a test body is released. This hook still runs. | ||
| const resources = new DisposableStack(); | ||
| afterAll(() => resources.dispose()); | ||
|
|
||
| // One raw GET on its own connection. `closed` settles when the server ends | ||
| // the connection, and `seen` is what arrived until then (`bodyStart` keeps | ||
| // the first 4096 body bytes). `onBodyStarted` runs once, with the first body | ||
| // bytes; its `settle` ends the wait when the test cannot go on. | ||
| function getUntilClosed( | ||
| server: ReturnType<typeof Bun.serve>, | ||
| useTls: boolean, | ||
| pathname: string, | ||
| onBodyStarted: (settle: () => void) => void = () => {}, | ||
| ) { | ||
| const seen = { head: "", contentLength: -1, bodyBytes: 0, bodyStart: "", error: undefined as string | undefined }; | ||
| const { promise: closed, resolve, reject } = Promise.withResolvers<void>(); | ||
| let waiting = true; | ||
| const settle = () => { | ||
| waiting = false; | ||
| resolve(); | ||
| }; | ||
| let received = ""; | ||
|
|
||
| const port = server.port!; | ||
| const socket = useTls | ||
| ? tlsConnect({ host: "127.0.0.1", port, rejectUnauthorized: false }) | ||
| : netConnect({ host: "127.0.0.1", port }); | ||
| resources.defer(() => socket.destroy()); | ||
| // Runs before the server stops: say where a wait that never ended stood. | ||
| resources.defer(() => { | ||
| if (waiting) { | ||
| console.error("the server did not end the connection:", { | ||
| pathname, | ||
| useTls, | ||
| contentLength: seen.contentLength, | ||
| bodyBytes: seen.bodyBytes, | ||
| pendingRequests: server.pendingRequests, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // The server ends the connection in order, so a reset is part of the result. | ||
| socket.on("error", (error: NodeJS.ErrnoException) => { | ||
| seen.error = error.code ?? error.message; | ||
| }); | ||
| socket.on("close", settle); | ||
| socket.on("data", (chunk: Buffer) => { | ||
| if (seen.contentLength < 0) { | ||
| received += chunk.toString("latin1"); | ||
| const headEnd = received.indexOf("\r\n\r\n"); | ||
| if (headEnd < 0) return; | ||
| seen.head = received.slice(0, headEnd); | ||
| seen.contentLength = Number(/^content-length:\s*(\d+)/im.exec(seen.head)?.[1]); | ||
| chunk = Buffer.from(received.slice(headEnd + 4), "latin1"); | ||
| } | ||
| const bodyStarted = seen.bodyBytes > 0; | ||
| seen.bodyBytes += chunk.length; | ||
| if (seen.bodyStart.length < 4096) { | ||
| seen.bodyStart += chunk.toString("latin1", 0, 4096 - seen.bodyStart.length); | ||
| } | ||
| if (seen.bodyBytes > 0 && !bodyStarted) { | ||
| try { | ||
| onBodyStarted(settle); | ||
| } catch (error) { | ||
| waiting = false; | ||
| reject(error); | ||
| } | ||
| } | ||
| }); | ||
| socket.once(useTls ? "secureConnect" : "connect", () => { | ||
| socket.write(`GET ${pathname} HTTP/1.1\r\nHost: x\r\n\r\n`); | ||
| }); | ||
| return { closed, seen }; | ||
| } | ||
|
|
||
| // The server sends until the socket stops taking bytes, so a file that the | ||
| // loopback buffers can hold is sent whole before anything shrinks. This | ||
| // length is far beyond them. Only the length is set: no bytes are written. | ||
| const SHRINKING_FILE_SIZE = 256 * 1024 * 1024; | ||
|
|
||
| describe.concurrent.each([ | ||
| { via: "fetch handler", pathname: "/big.bin", transport: "plain" }, | ||
| { via: "fetch handler", pathname: "/big.bin", transport: "tls" }, | ||
| { via: "file route", pathname: "/route", transport: "plain" }, | ||
| { via: "directory route", pathname: "/dir/big.bin", transport: "tls" }, | ||
| ] as const)("from a $via over $transport, when the file shrinks mid-body", ({ pathname, transport }) => { | ||
| test("ends the connection instead of leaving the client waiting for the missing bytes", async () => { | ||
| const dir = resources.use(tempDir("serve-file-shrinks", {})); | ||
| const filePath = join(String(dir), "big.bin"); | ||
| writeFileSync(filePath, ""); | ||
| truncateSync(filePath, SHRINKING_FILE_SIZE); | ||
|
|
||
| const server = resources.use( | ||
| Bun.serve({ | ||
| port: 0, | ||
| hostname: "127.0.0.1", | ||
| idleTimeout: 0, | ||
| ...(transport === "tls" ? { tls } : {}), | ||
| routes: { | ||
| "/route": new Response(Bun.file(filePath)), | ||
| "/dir/*": { dir: String(dir) }, | ||
| }, | ||
| fetch: () => new Response(Bun.file(filePath)), | ||
| }), | ||
| ); | ||
|
|
||
| // The client sends nothing after its request, so a server that treats | ||
| // the short body as complete never ends the connection. | ||
| let inFlightWhenShrunk = false; | ||
| const { closed, seen } = getUntilClosed(server, transport === "tls", pathname, settle => { | ||
| // The head is on the wire with the full size and the body has started. | ||
| inFlightWhenShrunk = server.pendingRequests === 1; | ||
| if (!inFlightWhenShrunk) return settle(); | ||
| truncateSync(filePath, 4096); | ||
| }); | ||
| await closed; | ||
|
|
||
| expect({ | ||
| inFlightWhenShrunk, | ||
| contentLength: seen.contentLength, | ||
| bodyShort: seen.bodyBytes < seen.contentLength, | ||
| error: seen.error, | ||
| }).toEqual({ inFlightWhenShrunk: true, contentLength: SHRINKING_FILE_SIZE, bodyShort: true, error: undefined }); | ||
|
Comment on lines
+1602
to
+1604
Contributor
There was a problem hiding this comment. 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win Assert that the shrink responses contain only file bytes. The sparse file contains only zero bytes. 🤖 Prompt for AI Agents |
||
| }); | ||
| }); | ||
|
|
||
| // A sysfs attribute is a regular file whose stat size is a whole page while | ||
| // a read returns only its few bytes. The committed length is wrong before | ||
| // anything shrinks, and the end of the file arrives with the only chunk. | ||
| // Other platforms have no regular file like it. | ||
| test.concurrent.skipIf(!isLinux)( | ||
| "sends what a file has when its stat size overstates it, then ends the connection", | ||
|
Comment on lines
+1606
to
+1613
Contributor
There was a problem hiding this comment. 🟡 nit (optional): maintainers running the suite in a sandbox without a real sysfs get a hard failure from this test instead of a skip. The test at test/js/bun/http/bun-serve-file.test.ts:1606-1613 reads /sys/devices/system/cpu/online and asserts statSize > content.length, but only gates on isLinux. Fix: skip when the attribute is missing or its stat size is not larger than its content (containers with a masked or emulated /sys), so the test only runs where the kernel exhibits the overstated-size property it exists to exercise. Why this was flaggedThe test at test/js/bun/http/bun-serve-file.test.ts:1606-1613 runs on every Linux host (test.concurrent.skipIf(!isLinux)) and reads /sys/devices/system/cpu/online with readFileSync and statSync. On a Linux host where /sys is not mounted, masked by the container runtime, or emulated with a 0 or exact stat size, readFileSync throws or the guard expect(statSize).toBeGreaterThan(content.length) fails, so the test reports a failure that is unrelated to the server code. The base branch has no such test. A skipIf on the precondition (attribute exists and stat size exceeds its content) would make the environment dependence a skip rather than a red run; it is a nit because standard CI runners mount sysfs. Verification: Triggers only when the suite runs on a Linux host whose /sys is unmounted, masked, or emulated. test/js/bun/http/bun-serve-file.test.ts:1612 gates solely on |
||
| async () => { | ||
| const attribute = "/sys/devices/system/cpu/online"; | ||
| const content = readFileSync(attribute).toString("latin1"); | ||
| const statSize = statSync(attribute).size; | ||
| expect(statSize).toBeGreaterThan(content.length); | ||
|
|
||
| const server = resources.use( | ||
| Bun.serve({ | ||
| port: 0, | ||
| hostname: "127.0.0.1", | ||
| idleTimeout: 0, | ||
| fetch: () => new Response(Bun.file(attribute)), | ||
| }), | ||
| ); | ||
|
|
||
| const { closed, seen } = getUntilClosed(server, false, "/online"); | ||
| await closed; | ||
| expect({ | ||
| contentLength: seen.contentLength, | ||
| connection: /^connection:\s*(.+)$/im.exec(seen.head)?.[1], | ||
| body: seen.bodyStart, | ||
| error: seen.error, | ||
| }).toEqual({ contentLength: statSize, connection: "close", body: content, error: undefined }); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| // A request that declares a body arms the request-body (onData) callback on | ||
| // the uWS response before the fetch handler runs. uWS keeps a single shared | ||
| // userdata slot per response, so when the handler returns a file response | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 pre-existing, not blocking: pre-existing: a Bun.file body whose written Content-Length differs from
StartOptions::lengthstill ends as complete when short, so the client waits exactly as on the base branch.remainingis seeded fromopts.lengthat FileResponseStream.rs:183, not from the Content-Length the caller committed, so the two can disagree. Fix: make the short check compare against the Content-Length actually written, or make every caller write Content-Length equal tolength(RequestContext.rs:3886-3891 forBun.file(fd).slice(), FileRoute.rs:439-443 when a route keeps a user Content-Length header).A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A fetch handler returns
new Response(Bun.file(fd).slice(0, 10))for a 1000-byte file. RequestContext.rs:1812-1815 sets auto_close false for an fd blob, so RequestContext.rs:1906-1910 never sets needs_content_range; RequestContext.rs:1893 overwrites blob.size with stat_size, and RequestContext.rs:3788-3792 pickssize = blob.size()= 1000 for the Content-Length written at RequestContext.rs:3887, whilelengthpassed at RequestContext.rs:2030-2034 issendfile.remain= 10. FileResponseStream.rs:183 seedsremainingwith 10; after 10 bytescharge_remainingreaches 0 and FileResponseStream.rs:557 computesshort = false, so FileResponseStream.rs:558 ends the response keep-alive with 990 bytes owed. The base branch hangs the client the same way; the new close-on-short logic does not reach these cases because its balance is not the committed length.Verification: pre-existing — the base branch hangs the client by the same route; but the PR's new short-body close does not reach this case. Trigger: a fetch handler returns
new Response(Bun.file(fd).slice(0, n)). RequestContext.rs:3886-3888 writes Content-Length = stat_size while :2030-2031 passeslength= n; FileResponseStream.rs:183 seedsremainingwith n, so :553-560 computesshort= false.