Repository navigation
Bun.serve: pause the file reader while the response is backpressured - #41244
Conversation
A pollable file body (a FIFO, a character device, a socket) re-arms its poll after a read that ends in EAGAIN. FileResponseStream paused the reader on backpressure only on Windows, so on POSIX the next readable event wrote into the backpressured response again. A client that stopped reading made the server buffer the whole source in memory. Pause the reader on every platform when a body write returns backpressure, and unpause it in on_writable before the next read. A paused POSIX reader ignores read(), so without the unpause a regular file body would stall at its first backpressure.
…e resume The io crate's on_read_chunk doc said that returning false stops the reader. The POSIX read loop still re-arms the poll after a read that ended in EAGAIN, so a parent that wants no more chunks has to pause the reader. The FIFO test now runs three cases. Bun.file(fd) keeps the fd open after the response ends, so a poll that an aborted stream left armed fires as soon as the pipe has data. The resume case fails if on_writable does not unpause the reader.
|
Status: the fix and the test are in this PR. CI and review are next. Reproduction (Linux x64, release build of main):
The server reads the FIFO without a limit: 64 MiB while the client read 0 bytes. With The FIFO test in |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .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; 6 remain after this review. WalkthroughChangesFile stream backpressure
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds regression coverage for pausing and resuming file-response reads under client backpressure, including disconnect handling. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, root cause, fix, test coverage, verification results, limitations, and related issues. Although it does not use the exact template headings, it provides the required content, including how the change was verified. Comment |
Jarred-Sumner
left a comment
There was a problem hiding this comment.
is there a way to write the test without using sleep/timeouts? can we make this determinstic
|
Yes. The fixture and the server share one event loop, so a request to the same server can replace each sleep. The server must poll for I/O to answer the request. If the reader's poll is armed, the server reads the full FIFO during that poll. If the reader is paused, the FIFO stays full. Each step becomes: fill the FIFO, |
Each step fills the FIFO and makes one request to the same server. The server shares the fixture's event loop and must poll for I/O to answer, so a reader with an armed poll reads the full FIFO before the answer arrives. The loops end on a byte limit or a request count.
|
Done in f290f24. The test has no sleeps and no timeouts now. Each step fills the FIFO, makes one request to the same server, and counts the bytes that the server read during that request. The loops end on a byte limit (32 MiB) or on a request count (200), not on time. The results are the same in every run:
I also removed the 30 s test timeout. |
A stream that waits for pipe data when its client disconnects stays allocated while the pipe is open. The resume case exited in that state, so the leak check on the ASAN lane reported the stream. The case now stops reading again and waits for the stall before the disconnect.
…its highwater mark (#42038) ### Problem - A locked, idle reader on `Bun.spawn` stdout or `Bun.stdin.stream()` pins the process once the peer goes away. Repro on 1.4.2: read one chunk of a child's stdout, SIGKILL the child, `await proc.exited`. The process never exits. Node exits. - `FileReader::on_read_chunk` stops reading at its 16 KB highwater mark when no JS read waits. On POSIX it only returned `false`: the read loop ends, but the fired one-shot poll stays unarmed yet still counts as active on the loop. Unarmed, it cannot see the peer close, so the count never drops. - The reverse too: `child.stdout.pause()` then `resume()` re-armed the poll without its hold, so an unref'd child's parent exited mid-stream. ### Fix - `on_read_chunk` calls `reader.pause()` on every platform (it was Windows-only). That unregisters the poll and drops its hold. `on_pull` unpauses. - `try_register_poll()` re-applies `KEEP_ALIVE` on every arm (before: first registration only), and `update_ref(true)` takes the hold only while armed. A ref'd reader holds the loop exactly while it can deliver something, as in Node (`readStop()` leaves the handle inactive). - A forced `FilePoll` unregister (teardown) also deletes the kernel entry that a fired, softly unregistered poll leaves behind. Without it, cancelling a stopped stdin reader left fd 0 registered and the next reader got EEXIST. - Verified: new rows in `spawn.test.ts` and `process-stdin.test.ts`. Six fail on 1.4.2 and pass now, two guard holds that must survive. ### Background - `PosixBufferedReader` arms its fd with `EPOLLONESHOT` / `EV_DISPATCH`: each wakeup disarms the poll until `register_poll` re-arms it. The poll adds one to the loop's active count while it "keeps the process alive". That count keeps the loop running. - `FileReader` is the `ReadableStream` source for pipes, sockets and FIFOs. With no pending JS read it buffers up to `highwater_mark`, then stops until the next `pull`. <details><summary>Notes</summary> - Second behavior change, on purpose: pausing at the mark also turns the EAGAIN re-arm in `read_loop` into a no-op. Before, when the read that crossed the mark ended in EAGAIN, the poll was re-armed despite the stop, so an idle reader kept draining a trickling writer into `buffered` between pulls (a FIFO trace showed 50+ consecutive 8 KB chunks with no pull, far past 16 KB). Now a stopped POSIX reader reads nothing until the next pull, which is what Windows already did. The "trickling writer" row covers it. - Node ground truth (v26.3.0): `process.stdin.once("data", () => process.stdin.pause())` and `Readable.toWeb(process.stdin).getReader().read()` both exit 1 to 2 ms after the first chunk with `readableLength=65536, reading=false` while the writer is still alive. `uv_ref` on an inactive handle does not hold the loop either, which is what the `update_ref(true)` gate mirrors (`process.stdin.ref()` while stopped at the mark does not re-pin, the next read does). - Alternative considered: keep HUP/ERR interest while stopped. epoll can wait for `EPOLLHUP` without `EPOLLIN`, but kqueue's `EVFILT_READ` is level-triggered on available bytes and a stopped reader always has unread bytes in the pipe, so there is no portable HUP-only wait. Releasing the loop is uniform across platforms and is what Node does. In a long-lived process the stopped stream stays open (and GC-rooted through `waiting_for_on_reader_done`) until it is read or cancelled, as before. - `PipeReader`'s contract says returning `false` from `on_read_chunk` "ends only the current read loop. To stop the reader, call `pause()`". The sink-backpressure path and `setFlowing(false)` already did. The highwater stop now does too. #41244's notes recorded the missing half: "pause() unregisters the poll, and that also releases the poll's hold on the event loop. unpause() does not take it back". Re-applying `KEEP_ALIVE` on every arm is that fix, and the `child_process` pause/resume row is its test. - The teardown hole: `pause()` on a fired poll takes the `needs_rearm` shortcut in `unregister_with_fd_impl` (no syscall, `Poll*` flags cleared, disarmed entry kept so the next arm is a `CTL_MOD`). `PollOrFd::close` then called `deinit_force_unregister`, which saw no `Poll*` flag and skipped the delete. For an fd that outlives the reader (stdin) the next `EPOLL_CTL_ADD` returned EEXIST. On 1.4.2 the cancel usually happened while the flags were still set, so the delete ran. Stopping through `pause()` made the hole reachable. Found by a parallel investigation of the same report (branches `robobun/392a1a11/idle-pipe-reader-loop-ref` and `robobun/e37efcfc/pipe-reader-keepalive-while-armed`, whose fd-0 reuse, shell-pipe and pause/resume rows are adapted here). - Windows: `on_read_chunk` already paused there and `read_into` is `unpause()`, so the added `unpause()` before it is a no-op. No behavior change intended. - Test mechanics: the spawn rows unref the still-running child after taking the reader, so only the stdout reader can hold the parent. Rows that wait inside the child avoid top-level await, since an unsettled entry-module promise keeps a Bun process alive by itself. The fd-0 row waits for `getEventLoopStats().loopActive` to drop instead of sleeping. - Suites run locally (debug+ASAN): spawn.test.ts, spawn-stdin-readable-stream, spawn-streaming-*, readablestream-helpers, spawn leak/GC/stale-fd tests, process-stdin, child_process, streams.test.js, native-source-onclose-leak, bun-serve-file, serve-body-leak, fetch-backpressure, filesink, bunshell, shell file-io/pipeline/hang, terminal. - Pre-existing in this container with or without the change (debug+ASAN timing or environment): `spawn-pipe-leak.test.ts` (todo on ASAN CI), `shell/leak.test.ts` memleak rows, `child_process.test.ts` "default shell" and "extra stdio pipes are not double-closed on GC", the S3 rows of `fetch-backpressure.test.ts`. `bun-serve-file.test.ts` "resumes when the client reads" runs 3.7 to 4.3 s here and occasionally crosses 5 s. - Open PRs in the same functions: #41632 adds the same `unpause()` before `read_into` and force-unregisters in `pause()`. Both compose with this. - Self-reviewed: 3 concerns raised, 3 addressed (route the stop through `pause()` instead of a new io-layer state, close the EAGAIN re-arm hole, add the must-still-hold tests). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/process/process-stdin.test.ts, test/js/bun/spawn/spawn.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Problem
Bun.servekeeps reading a pollableBun.file()body (a FIFO, a character device, a socket) while the client reads nothing. The bytes collect in the response's write buffer, which has no limit.Bun.file(fd), the next readable event reads freed memory:AddressSanitizer: heap-use-after-freeinPosixBufferedReader::begin_read.on_read_chunk(src/runtime/server/FileResponseStream.rs:327) pauses the reader only on Windows. On POSIX the read loop re-arms the poll after EAGAIN (src/io/PipeReader.rs:846).Fix
on_writableunpauses it before the next read.FileReaderuses the same handshake (src/runtime/webcore/FileReader.rs:669).read(). Fix: after pausing stdin, a subprocess should be able to read from stdin #23341 limited the pause to Windows becauseon_writabledid not unpause. On Windowsread()already unpauses.test/js/bun/http/bun-serve-file.test.ts. The server must stop reading a FIFO, forBun.file(path)andBun.file(fd), and read again when the client does. All three cases fail on main.on_read_chunkdoc is fixed, the Fix: after pausing stdin, a subprocess should be able to read from stdin #23341 history is above, and the Notes name the openFileReadercase and the order with ci: retry only the test files listed in test/flaky-tests.txt #41204.Background
FileResponseStreamstreams a file descriptor into a uWS response. Withoutsendfile, aBufferedReaderreads the fd and callson_read_chunkfor each chunk.resp.write()returnsBackpressurewhen the socket is full. uWS buffers the rest and callson_writablewhen the socket drains.Notes
Measurements (Linux x64)
still reading after 33554432 bytes, on the release build and on the debug build. With the fix each case takes about 1.4 s on the debug build, and about 2.5 s with all cores busy.The crash
Bun.file(fd)case: the read is inPosixBufferedReader::begin_read, fromFilePoll::on_update. The free is in the drop ofFileResponseStream, fromon_aborted(HttpContext::onClose). The allocation is inFileResponseStream::start, fromRequestContext::do_sendfile.panic(main thread): Segmentation fault at address 0x98.Dropdoes not unregister the poll, becausestart()clearsCLOSE_HANDLE. Bun.serve: stop FIFO file responses from keeping the process alive #37083 changes that teardown. This PR only makes sure that the poll is not armed when a backpressured stream is freed.The test
stalledalso when the drain never stalled. Its stall loop ended at a time bound, so on main it waited 5 s and passed.O_NONBLOCK. A write to a full pipe fails with EAGAIN, so the byte count grows only when the server reads.unpause()inon_writable. With the fix, the server reads again after 5 requests. A build without the unpause fails that case withno read for resumed, and passes the other two.Regular files
sendfile(TLS, HTTP/3, macOS). A scratch test served a 48 MiB file to slow readers over http and https, through a fetch handler and a static route. All bodies matched by SHA-256, andon_writableran 167 times.Keep-alive
pause()unregisters the poll, and that also releases the poll's hold on the event loop.unpause()does not take it back. The sink path ofFileReaderbehaves the same way.server.unref().Not in this PR
FileReaderstill pauses only on Windows when it stops at its high-water mark (src/runtime/webcore/FileReader.rs:647-655). SoBun.file(fifo).stream()with a stalled consumer can still buffer without a limit on POSIX. That fix needs an unpause beforeread_into, and its own test.test/flaky-tests.txtwith a line fortest/js/bun/http/bun-serve-file.test.ts, because of the CI timeout that this bug causes. ci: retry only the test files listed in test/flaky-tests.txt #41204 is not merged. The PR that lands second removes that line.Other checks
bun bd testonbun-serve-file,bun-serve-static,serve-directory-routes,serve-file-slice-read-error,serve-http3, andserve.test.ts. Two tests inserve.test.tsfail the same way on the release build (a privileged port, and a loopback-only check). This container runs as root.cargo checkforx86_64-pc-windows-msvcpasses.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-file.test.ts