BufferedReader: enforce the blob slice window at the read instead of after it

Merged
RO
3mo ago
+232-152

Follow-up to #39201, where moving the slice window into the reader was agreed as a separate PR.

Problem

  • Streaming a slice of a pipe, socket or stdin consumes bytes past the slice. With 0123456789 written to a child's stdin, for await (const c of Bun.stdin.slice(0, 3).stream()) delivers 012 and whoever reads stdin next gets nothing: the other 7 bytes were read and dropped. Same through a native sink (HTMLRewriter.transform(new Response(Bun.stdin.slice(0, 3)))). With 60000 bytes written, the stream leaves 0 of them in the pipe. Same on Windows.
  • Bun.stdin.slice(0, 0).stream() (any empty slice of a pollable source) never ends while the pipe stays open and silent; it ends only when a byte arrives, which it drops, or at EOF. Same through a native sink.
  • Bun.serve responding with Bun.file(p).slice(0, 2048) of a 4 KiB file issues two read()s per response (a 256 KiB request that returns the whole file, then an EOF probe) and ends the response from a queued task. Harmless for regular files, but it is the same after-the-fact window as above, implemented a second time.
  • Cause: the window is applied by the reader's parents after the bytes have been read. FileReader::on_read_chunk cuts the delivered chunk down (src/runtime/webcore/FileReader.rs:636); the only place that cut the read itself was on_pull's read_into branch (FileReader.rs:849), which is skipped whenever a poll is registered, i.e. for every pollable fd, and read_into is a stub on Windows. The read loop in src/io/PipeReader.rs reads up to its 256 KiB scratch buffer (fill_scratch, line 653) and the Windows reader hands libuv its whole spare capacity (get_read_buffer_with_stable_memory_address, line 1166); neither knows about the window. The empty-window hang is the same gap: the check lived on the read_into branch, and a pollable source parks on its poll before reaching it. FileResponseStream keeps a third copy of the window (src/runtime/server/FileResponseStream.rs:277) and needs the FileResponseStreamEof task to end itself.

Fix

  • PosixBufferedReader and WindowsBufferedReader get a ReadLimit next to the pread offset, set with set_limit before starting. read_once (every POSIX read: loop, buffer and read_into) and the libuv buffer on Windows cut each read to what is left of it, and charging it down to zero is reported the way EOF already is (Stop::Eof / ReadState::Eof, then close and on_reader_done). A limit that starts at zero makes start register no poll and issue no read; the parent's first read request (read, read_into, Windows unpause) reports EOF without touching the fd.
  • FileReader passes the blob's size to the reader at start and FileResponseStream passes the body length; both now treat the end of the window exactly like EOF, because that is what the reader reports. Deleted: FileReader's total_readed / window_remaining / consume_window / end_at_window, FileResponseStream's max_size and the FileResponseStreamEof task (tag, dispatch arm, Taskable impl), and Chunk::truncate, which only existed for this.
  • FileResponseStream implements the reader's ref_ / deref (as FileReader does). The chunk that fills the window is now followed by on_reader_done, which releases the stream's owning ref, while the reader's read loop (POSIX) or libuv completion (Windows, which re-checks the reader's flags after delivering a data read) is still on the stack with the reader embedded in the stream; the pin keeps the stream alive until those frames return. This is the job the queued task used to do.
  • Why this is the right place: the window is a bound on how many bytes may be taken out of the source, and on anything that is not seekable the bytes a read returns are gone, so the bound has to be applied to the read, and only the reader knows whether a read is needed at all. Reporting it as EOF is correct because for this reader the source does end there, and every consumer already handles EOF (real EOF and maxBuffer exhaustion end streams through the same path), which is what lets the two parent implementations (three code paths, two of which #39201 had to bring back into agreement) go away. For regular files the delivered bytes are unchanged; only the reads get shorter.
  • Verified with bun bd test test/js/bun/util/bun-stdin-slice.test.ts test/js/bun/http/serve-file-slice-read-error.test.ts. The five new bun-stdin-slice cases run the slice in a grandchild sharing the child's stdin and then read what is left: a write larger than the window and an empty window with nothing written, each via stream() and via HTMLRewriter, plus a window filled by a write larger than its remainder. With src/ stashed, the three over-read cases get "" instead of 3456789 / 456 back and the two empty-window cases time out; with the fix all pass on Linux and on a Windows x64 debug build (the block is not Windows-skipped; the same protocol reproduces the over-read with the current Windows canary). serve-file-slice-read-error.test.ts kept its ptrace read counter: 3 responses now take 3 read()s and the EIO injected into the second one fails that response alone (DONE 2); without the fix it is 6 reads and DONE 3.
  • Also run with the fix: blob.test.ts (the #39201 window cases: read_into, sink, Windows on_read_chunk, empty window on a regular file), bun-serve-file, bun-serve-static, serve-directory-routes (empty files give FileResponseStream a zero length), bun-file*, spawn.test.ts, spawn-maxbuf (the budget shares read_once / on_read), spawn-streaming-stdout, spawn-stream-serve, spawn-stdout-filereader-gc-uaf, streams.test.js, html-rewriter*, body*.test.ts, process-stdin, bun-write; on Windows additionally blob, bun-serve-file, bun-serve-static, spawn-maxbuf, bun-file*. cargo fmt clean.

Background

  • BufferedReader (src/io/PipeReader.rs, one POSIX and one libuv implementation behind a cfg alias) reads an fd on behalf of a parent embedded around it and reports through a vtable: on_read_chunk per read, then on_reader_done at EOF or on_reader_error. FileReader is the parent behind a file-backed ReadableStream (Bun.file().stream(), new Response(file).body, the stream a native sink such as HTMLRewriter or a fetch body pulls from); FileResponseStream is the parent that writes a Bun.serve file body to the socket.
  • A file blob carries an offset and a size; for a slice these describe the window. ReadableStream::from_blob_copy_ref copies them into FileReader as start_offset / max_size; Bun.serve computes the body's offset and length from the blob (or a Range header) and passes them to FileResponseStream. The offset was already the reader's job (start_file_offset); this PR gives it the length as well.
  • On POSIX a pollable fd (pipe, socket, tty) is read when its poll fires, from the reader's own loop; a regular file is read synchronously when the parent asks (read, or read_into straight into the JS pull buffer). FileReader::on_pull only uses read_into when no poll is registered, which is why a window applied there never affected pollable sources. On Windows every read completes through libuv into a buffer the reader allocates up front, so that allocation is where a read's size is decided.
  • ref_ / deref on the parent vtable are taken by the reader around a read dispatch (ParentKeepAlive) because the reader touches its own fields after calling into the parent; a parent whose on_reader_done can free it needs them so that free happens after the dispatch returns. maxBuffer (MaxBuf) is the reader's other byte bound; the limit is charged and reported alongside it.
Related open PRs
  • #39257 (alii) fixes the empty-window hang inside FileReader with the helpers this PR deletes; if it lands first this PR will be rebased to drop those two guards (the reader covers them) and keep its tests, which pass here as well. The over-read is not covered there.
  • #31680 and #33510 are about the buffered (.text() / .arrayBuffer()) path in blob/read_file.rs, which does not use BufferedReader; #31680's FileReader.rs hunk was made unnecessary by #39201.
  • #34185 wanted FileResponseStream's remaining count to tell a short file from a complete one; with this PR that is the bytes it has written versus the length it was given.