io(posix): bound the streaming read loop so always-ready sources yield to the event loop - #35519
io(posix): bound the streaming read loop so always-ready sources yield to the event loop#35519robobun wants to merge 8 commits into
Conversation
WalkthroughChangesThe PR updates buffered file-stream polling and drain continuation, simplifies Buffered file stream lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:05 AM PT - Jul 25th, 2026
❌ @autofix-ci[bot], your commit e71ca61 has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35519That installs a local version of the PR into your bun-35519 --bun |
||||||||||||||||||||||||||||||||||||||
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re the find-issues suggestion: #29184 is about |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
37f95ce to
a607336
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/io/PipeReader.rs:896-911— Bounding the streaming read loop to one stack-buffer chunk per call breaksBun.serve'sFileResponseStreamfor regular files >~128 KB: after the first chunk,file_type == Fileskipsregister_poll()and returns, butFileResponseStreamis push-driven and only re-entersreader.read()fromon_writable, which is registered solely on theBackpressurearm — soWriteResult::WantMore(the common case) leaves nothing armed and the response hangs after ~128–256 KB. This regressesnew Response(Bun.file(path))and staticroutes: {"/": Bun.file(...)}on macOS (all transports), Linux TLS/H2/H3, and Linux plain HTTP for files 128 KB–1 MB (below the sendfile threshold). Forfile_type == Filewithkeep_going == true, keep the oldhead_start = 0; continuebehaviour — regular files hitbytes_read == 0in bounded time, and/dev/urandomis classifiedNonblockingPipeso it already takes theregister_poll()+return path.Extended reasoning...
What the bug is
Commit ce697ed changed
PosixBufferedReader::read_with_fn's streaming stack-buffer branch from an unbounded drain loop (while _buffer.capacity() == 0 { … head_start = 0; continue }) to a bounded one: after delivering one ≥128 KiB chunk it doesif keep_going && file_type != FileType::File { parent.register_poll(); } return;
For pollable file types this is fine — the re-armed poll re-drives
on_poll→read_with_fn. ForFileType::File(non-pollable regular files)register_poll()is correctly skipped, so the caller is responsible for re-invokingreader.read().FileReaderdoes this: the next JSpull()callson_pull→reader().read(). ButFileResponseStream— theBun.servepath forreturn new Response(Bun.file(path))androutes: { "/x": Bun.file(...) }— is push-driven:start()callsreader.read()exactly once (FileResponseStream.rs:228-229), and the only re-driver ison_writable(line 351-352), which is registered only whenresp.write()returnsWriteResult::Backpressure(line 288-298). Whenresp.write()returnsWriteResult::WantMore— the common case on localhost, HTTP/2 stream buffers, or any socket with a send buffer ≥256 KB —on_read_chunkreturnstrue(line 303) and nothing is armed on the event loop.Step-by-step trace
For
Bun.serve({fetch: () => new Response(Bun.file("big.bin"))})with a 500 KB regular file, on macOS (or Linux TLS/H2/H3, or Linux plain HTTP for a file <1 MB — anything wherecan_sendfile()at FileResponseStream.rs:559 returns false):RequestContextcomputesfile_type=File, pollable=falseand callsFileResponseStream::startwithuse_sendfile=false.startsetsPOLLABLE=false, callsreader.start(fd, false)→handle = PollOrFd::Fd(fd),_buffercleared (capacity 0), no poll. Thenhold_read_ref(); reader.read().read()→get_file_type()==File→read_file→read_with_fn(File, fd, 0, received_hup=false, sys::read).is_streaming_enabled()==true(has_on_read_chunk=trueat FileResponseStream.rs:536),_buffer.capacity()==0→ enters the new bounded block.stack_buffer_len=256 KiB,stack_buffer_cutoff=128 KiB. Firstsys::readfills ~256 KiB →head_start≈256 KiB→(256K−256K)=0 < 128K→ cutoff branch at PipeReader.rs:879.on_read_chunk(256 KiB, Progress)→FileResponseStream::on_read_chunk:max_sizedecrements 500K→244K (≠0, no eof_task),state==Progressskips the EOFend()branch,resp.write(256K)→WriteResult::WantMore→ returnstrue.on_writableis not registered.- Back at PipeReader.rs:896-911:
received_hup==false→ skip the HUPcontinue.keep_going==true && file_type != Fileis false →register_poll()skipped.return. - Control returns to
FileResponseStream::startline 229. Nothing is armed: no poll (File is non-pollable), noon_writable(only registered onBackpressure), noeof_task(max_size≠0).READ_REF_HELDkeeps the allocation alive so it isn't even finalized. - The client receives one ~128–256 KB chunk and the remaining ~244 KB are never read.
resp.end()is never called; the connection hangs until the server's idle timeout aborts it with a truncated body.
Files <
128 KB escape (the first read leaves >128 KiB headroom, the inner128 KB stall.whileiterates, the second read returns 0 → EOF path). Files ≥Why nothing else catches it
The only re-driver of
reader.read()on theFilepath isFileResponseStream::on_writable, and it is registered exclusively on theBackpressurearm.WantMorehas no continuation.can_sendfile()(FileResponseStream.rs:559-582) returnsfalseunconditionally on macOS, and on Linux for anything except plain-TCP +FileType::File+length≥1 MB— so macOS all files, Linux TLS/H2/H3 all files, and Linux plain-HTTP files 128 KB–1 MB all take the BufferedReader path and hit this.Old behaviour: the cutoff branch reset
head_start=0andcontinued the innerwhile(and the outerwhile _buffer.capacity()==0looped again), so a singlereader.read()synchronously drained a regular file tobytes_read==0→done(). Not ideal for very large files, but correct — and bounded, since regular files EOF.Impact
return new Response(Bun.file(regularFile))from aBun.servehandler, androutes: { "/x": Bun.file(...) }static routes, hang for regular files larger than ~128 KB on macOS (all transports), Linux TLS/H2/H3 (all sizes), and Linux plain HTTP for files between ~128 KB and 1 MB. That is a core-feature regression in a very common production pattern.Suggested fix
For
file_type == Filewithkeep_going == true, keep the oldhead_start = 0; continuebehaviour instead of returning:if received_hup || (keep_going && file_type == FileType::File) { head_start = 0; continue; } if keep_going { parent.register_poll(); } return;
Regular files reach
bytes_read == 0in bounded time, so this cannot spin. The/dev/urandomcase this PR fixes is unaffected: after ce697ed reverted the chardev reclassification,/dev/urandomis still openedO_NONBLOCKand classifiedNonblockingPipe(notFile), so it takes theregister_poll()+return path — and theFileReader::on_read_chunkhighwater-mark change makes it returnfalseafter one chunk, so the poll isn't re-armed until the next pull. The same reasoning applies at the second newreturn(PipeReader.rs:962-966, the fall-through after the innerwhileexits on <16 KiB headroom).Alternatively, only bound the loop when the consumer returned
false— the highwater-mark change already makesFileReader::on_read_chunkdo that after one chunk from an infinite source, which is sufficient to fix the original bug without capping thekeep_going==truecase at all. -
🟡
src/runtime/webcore/FileReader.rs:796-807— The PR title and the entire Fix section describe the abandoned reclassification approach from commit ec06736 ("clearis_nonblocking", "isatty()is now checked for anyS_ISCHRfd", "the fd-based path … is also covered"), which ce697ed replaced — the current diff makes no changes toLazy::open_file_bloband instead boundsread_with_fnto one stack-buffer pass per call and removes thereader_is_pollable()bypass so highwater-mark backpressure applies to pollable readers. Please update the title/description before merge (it becomes the squash commit message), and note that the earlier inline comments about the event-driven-chardev regression,S_IFBLKfallthrough, and fd-path test coverage were filed againstopen_file_blobcode that no longer appears in the diff — they can be resolved rather than actioned.Extended reasoning...
What's stale
The PR title — "FileReader: route non-TTY character devices through the non-pollable File path" — and the entire Fix section of the description document the mechanism from commit
ec067364:Treat non-TTY
S_ISCHRfds the same as regular files: clearis_nonblockingso they go through the non-pollableFileType::Filepath … TTYs keep the pollable path via the existingis_attyterm (andisatty()is now checked for anyS_ISCHRfd, not just stdio) … the fd-based path (Bun.file(fd)with or withoutO_NONBLOCK) is also covered.Commit
ce697ed5("Bound the streaming read loop instead of reclassifying chardevs") replaced that approach, and the current diff reflects only the replacement.Step-by-step verification against the diff
Lazy::open_file_blobis untouched. The twoFileReader.rshunks are at@@ -563,22 +563,6 @@(deletereader_is_pollable()) and@@ -809,14 +793,18 @@(this hunk — the highwater-mark predicate). Reading the preloadedFileReader.rs:140-260: there is noelse if sys::S::ISCHR(mode)branch, no widenedisatty()check, andis_nonblockingis only cleared forISREG. A path-opened/dev/urandomis still classifiedpollable = true,file_type = NonblockingPipe— exactly as before this PR.- What actually changed is in
PipeReader.rs::read_with_fn: the outerwhile parent._buffer.capacity() == 0becameif, and after delivering one stack-buffer chunk (either at the< stack_buffer_cutoffflush or when the inner loop exits) the function now callsregister_poll()(for non-Filetypes) and returns, instead of looping untilEAGAIN/EOF.has_pending_read()also switched fromis_registered()tois_watching()so a fired one-shot poll counts as no-pending-read andon_pulldrives the next read. - This hunk removes the
&& !self.reader_is_pollable()exemption fromon_read_chunk's return value, so oncebuffered.len() + reader_buffer.len() >= highwater_markthe consumer returnsfalsefor pollable readers too — which now means "return without re-arming the poll" (backpressure), and the next JSpull()drainsbufferedand re-arms.
So the actual mechanism that lands is "bound the pollable read loop to one stack-buffer chunk per call and apply highwater-mark backpressure to pollable readers" — chardevs remain
NonblockingPipe, they just can no longer spin. None of "clearis_nonblocking", "isatty()is now checked for anyS_ISCHRfd", or "the fd-based path is also covered" appears in the diff.Why it matters
The description becomes the squash commit message. A future bisecter landing on this commit for a pipe-throughput or poll-re-arm regression will read "route non-TTY character devices through the non-pollable File path", grep
open_file_blobfor anS_ISCHRbranch that doesn't exist, and conclude the commit is unrelated..claude/docs/landing-prs.md(PR process) covers exactly this — the description must accurately state the mechanism.Prior-round comments that are now stale
Three of the earlier inline comments were filed against
open_file_blobcode that no longer appears in the diff and should be resolved, not actioned:- 🔴 event-driven chardev regression (
/dev/input/event*→FileType::File+O_NONBLOCK→ EAGAIN with no poll armed) — chardevs are stillNonblockingPipe, so theregister_poll()-on-EAGAIN arm still fires; the regression cannot occur. - 🟡
S_IFBLKfallthrough — the fix no longer enumeratesst_modetypes; block devices are now bounded by the same one-chunk-per-call yield as everything else. - 🟡 fd-path /
/dev/nulltest coverage — the reasoning ("the reclassified device set", "goes through thePathOrFileDescriptor::Fdarm'sget_fcntl_flags") targets the reclassification approach; the/dev/nullEOF test was already added in the current diff.
The remaining nits (
test.concurrent, hang-guard branching, upper-bound assertion) were addressed in the current test file (isASAN || isDebug ? 20_000 : 4_000,signalCodein the asserted object,toBeLessThanOrEqual(1024*1024),rssGrowthMBcheck, "Sequential on purpose" comment).Fix
Update the title to something like "PipeReader: bound the streaming read loop and apply highwater backpressure to pollable readers", and rewrite the Fix section to describe the
while→if+register_poll()-and-return change, thehas_pending_read()is_watching()switch, and the removal of thereader_is_pollable()bypass inon_read_chunk. The Problem and Cause sections remain accurate.Severity
Nit — PR metadata only; no shipped code is wrong. Filed so the squash commit message doesn't mislead future readers and so the stale prior-round threads get resolved rather than reopened.
|
Re the FileResponseStream regression finding: fixed in b86733a. The PR title and body were already updated to describe the bounded-read-loop approach. |
Bun.file("/dev/urandom").stream().getReader().read() wedged the event
loop: open_file_blob opened the chardev with O_NONBLOCK, saw
is_nonblocking=true, marked it POLLABLE + NonblockingPipe, and the
PosixBufferedReader read loop then spun preadv2(RWF_NOWAIT) forever on
the JS thread. /dev/urandom, /dev/zero and /dev/full never EAGAIN and
never return 0, so the loop had no exit; timers never fired and RSS
grew ~1GB/s until OOM.
Route non-TTY S_ISCHR fds through the same non-pollable File path as
regular files (one bounded read per pull; the existing highwater-mark
backpressure in on_read_chunk applies). TTYs keep the pollable path
via the existing is_atty term.
The previous commit routed non-TTY S_ISCHR fds through the non-pollable File path. That regresses event-driven character devices (/dev/input/*, /dev/hidraw*, tun/tap) which DO honour O_NONBLOCK: on the File path an EAGAIN only debug_warn!s and never arms a poll, so the pending read() promise would never resolve. Rework so every pollable source stays pollable and the read loop itself is bounded: - PosixBufferedReader::read_with_fn: deliver at most one stack-buffer chunk per call, then re-arm the poll (when the consumer said keep going) and return. An always-ready source now yields to the event loop once per chunk instead of spinning; pipes/sockets/TTYs are unchanged because they hit EAGAIN before the cutoff anyway. - FileReader::on_read_chunk: apply the highwater-mark backpressure to pollable readers too. Returning false here makes read_with_fn return without re-arming, so an idle stream stops buffering at ~one chunk instead of growing RSS without bound. - PosixBufferedReader::has_pending_read: use is_watching() so a one-shot poll that has fired and not been re-armed counts as no pending read. on_pull then takes the synchronous read path (which re-arms on EAGAIN) instead of parking on a poll that will never fire. Test: assert chunk size upper bound, RSS growth cap, and /dev/null EOF; scale the hang guard for debug/ASAN lanes.
FileResponseStream (Bun.serve file bodies on the non-sendfile path) is push-driven: it calls reader.read() once and relies on read_with_fn draining a regular file to EOF. The previous commit returned after one chunk for FileType::File without re-arming anything, so responses stalled after the first ~128-256 KiB when resp.write() returned WantMore. Only yield for pollable types; File keeps looping to EOF as before (regular files reach bytes_read == 0 in bounded time). Also drop the now-dead WindowsFlags::POLLABLE store in FileReader and branch the test's RSS cap on isASAN/isDebug.
137e1da to
e71ca61
Compare
|
CI build 80232 is red on lanes unrelated to this diff:
The gate (robobun/evidence) passed on this commit: the test fails without the fix and passes with it on both ASAN and release builds. Ready for review. |
|
Closing: this was fixed on main by #35977 (027716a), which dropped the same The |
2 participants
Problem
Bun.file("/dev/urandom").stream().getReader().read()wedges the event loop on POSIX: the firstread()never resolves, timers never fire, and RSS grows ~1 GB/s until OOM. Same for/dev/zero,/dev/full, andnew Response(Bun.file(dev)).body.strace on the stuck thread: back-to-back
preadv2(fd, [{iov_len=262144}], 1, -1, RWF_NOWAIT) = 262144, no epoll.Cause
PosixBufferedReader::read_with_fn's streaming stack-buffer path is an unbounded loop that only exits onread() == 0,EAGAIN, or the consumer returningfalse:/dev/urandom,/dev/zeroand friends never return0and neverEAGAIN, so the loop spins on the JS thread forever.FileReader::on_read_chunkwould normally returnfalseoncebuffered >= highwater_mark, but that check is gated on!reader_is_pollable(), so for pollable sources it always returnstrueand every 256 KiB chunk is appended toFileReader::bufferedwithout bound.Fix
Bound the loop and apply backpressure to pollable sources:
read_with_fn: after delivering one stack-buffer chunk, re-arm the poll (if the consumer wants more) and return instead of looping. Pipes, sockets and TTYs are unaffected in practice since they hitEAGAINbefore the cutoff; always-ready sources now yield once per chunk.FileReader::on_read_chunk: drop the!reader_is_pollable()gate so the highwater-mark check applies to pollable readers too. Returningfalsemakesread_with_fnreturn without re-arming, so an idle stream stops buffering at ~one chunk.PosixBufferedReader::has_pending_read: useis_watching()so a one-shot poll that has fired and not been re-armed counts as no pending read;on_pullthen takes the synchronous read path (which re-arms onEAGAIN) instead of parking on a dead poll.This keeps event-driven character devices (
/dev/input/*,/dev/hidraw*, tun/tap) on the pollable path whereEAGAINre-arms the poll as before.Verification
On stock bun the
/dev/urandomand/dev/zerocases wedge and are SIGTERM'd by the hang guard.[review] gate passed · iteration 3 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 3 rejected · iteration 3
evidence per changed file