FileReader: release the event loop while a pipe reader is stopped at its highwater mark

Merged
RO
2mo ago
+263-14

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.
Notes
  • 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).

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