Skip to content
15 changes: 13 additions & 2 deletions src/io/PipeReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,12 @@
}

// Exists for consistently with Windows.
pub fn has_pending_read(&self) -> bool {
matches!(&self.handle, PollOrFd::Poll(poll) if poll.is_registered())
// `is_watching()` (registered && !needs-rearm) rather than
// `is_registered()`: a one-shot poll that has fired but not been
// re-armed will not deliver another callback, so callers that skip
// `read()` on "pending" must not be told one is in flight.
Comment thread
robobun marked this conversation as resolved.
matches!(&self.handle, PollOrFd::Poll(poll) if poll.is_watching())

Check failure on line 465 in src/io/PipeReader.rs

View check run for this annotation

Claude / Claude Code Review

has_pending_read() change enables nested reader().read() re-entrancy from on_pull while outer read_with_fn is on the stack

Switching `has_pending_read()` from `is_registered()` to `is_watching()` opens a new re-entrancy path: inside the poll callback (one-shot poll has `NeedsRearm` set), a re-entrant `on_pull()` reached via `p.run()`'s microtask/nextTick drain now sees `has_pending_read()==false` and calls `self.reader().read()` — a nested `read_with_fn` on the same fd while the outer `read_with_fn` is still on the stack. If the nested read hits EOF, `on_reader_done()` runs (which does **not** set `FileReader::done`

Check warning on line 465 in src/io/PipeReader.rs

View check run for this annotation

Claude / Claude Code Review

has_pending_read() semantics change leaves stale comment + dead watch() call in FileReader::on_pull

Changing `has_pending_read()` from `is_registered()` to `is_watching()` makes the comment at `FileReader::on_pull` (src/runtime/webcore/FileReader.rs:~958) — *"has_pending_read() tracks the registration flag, not the kernel arm state"* — factually wrong, and the `self.reader().watch()` call it justifies is now a guaranteed no-op on both POSIX and Windows. Per REVIEW.md ("Delete dead code in the same PR that makes it dead" / "update every consumer atomically"), that block and its comment should b
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
}

pub fn watch(&mut self) {
Expand Down Expand Up @@ -1912,7 +1916,14 @@
// grows by `amount_result` every chunk and never resets, so a 1 GB
// `cat` holds 1 GB resident instead of ~64 KB. Clear it here, after
// the streaming consumer has finished with `slice`.
if should_continue && has_more != ReadState::Eof && self.vtable.is_streaming_enabled() {
// `should_continue` no longer gates the clear: FileReader may say
// stop at its highwater mark while uv keeps delivering, and leaving
// `_buffer` uncleared would double-buffer (here + FileReader.buffered).
// Parents that want the reader paused call `reader().pause()`
// themselves; stopping here could free a parent whose caller still
// holds `this` (FileResponseStream on abort).
Comment thread
robobun marked this conversation as resolved.
let _ = should_continue;
if has_more != ReadState::Eof && self.vtable.is_streaming_enabled() {
self._buffer.clear();
}

Expand Down
25 changes: 20 additions & 5 deletions src/js/internal/streams/native-readable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ interface NativePtr {
pull: (view: any, closer: any) => any;
updateRef: (ref: boolean) => void;
cancel: (error: any) => void;
setFlowing?: (flowing: boolean) => void;
}

let debugId = 0;
Expand Down Expand Up @@ -122,10 +123,14 @@ function getRemainingChunk(stream: NativeReadable, maxToRead?: number) {

function read(this: NativeReadable, maxToRead: number) {
$debug(`[${this.debugId}] read${this[kPendingRead] ? ", is already pending" : ""}`);
var ptr = this.$bunNativePtr;
// Readable called `_read`, so it wants data: make sure the native reader is
// not paused from a previous `push()===false` (readStart, like net.Socket).
// Runs even when a pull promise is outstanding so that promise can resolve.
Comment thread
robobun marked this conversation as resolved.
if (ptr) ptr.setFlowing?.(true);
if (this[kPendingRead]) {
return;
}
var ptr = this.$bunNativePtr;
if (!ptr) {
$debug(`[${this.debugId}] read, no ptr`);
this.push(null);
Expand All @@ -139,13 +144,13 @@ function read(this: NativeReadable, maxToRead: number) {
this[kHighWaterMark] = Math.min(this[kHighWaterMark], result);
}
if ($isTypedArrayView(result) && result.byteLength > 0) {
this.push(result);
pushAndCheck(this, result);
}
const drainResult = ptr.drain();
this[kConstructed] = true;
$debug(`[${this.debugId}] drain result: ${drainResult?.byteLength ?? "null"}`);
if ((drainResult?.byteLength ?? 0) > 0) {
this.push(drainResult);
pushAndCheck(this, drainResult);
}
}
const chunk = getRemainingChunk(this, maxToRead);
Expand Down Expand Up @@ -196,12 +201,22 @@ function handleResult(stream: NativeReadable, result: any, chunk: Buffer, isClos
}
}

// `push()` returning false means the Readable's buffer is at/above hwm (or
// the consumer paused); stop the native reader so kernel backpressure reaches
// the writer (readStop, like net.Socket). The next `_read()` re-enables it.
Comment thread
robobun marked this conversation as resolved.
function pushAndCheck(stream: NativeReadable, chunk: any) {
if (!stream.push(chunk)) {
const ptr = stream.$bunNativePtr;
if (ptr) ptr.setFlowing?.(false);
}
}
Comment thread
robobun marked this conversation as resolved.

function handleNumberResult(stream: NativeReadable, result: number, chunk: any, isClosed: boolean) {
if (result > 0) {
const slice = chunk.subarray(0, result);
chunk = slice.byteLength < chunk.byteLength ? chunk.subarray(result) : undefined;
if (slice.byteLength > 0) {
stream.push(slice);
pushAndCheck(stream, slice);
}
}

Expand All @@ -216,7 +231,7 @@ function handleNumberResult(stream: NativeReadable, result: number, chunk: any,

function handleArrayBufferViewResult(stream: NativeReadable, result: any, chunk: any, isClosed: boolean) {
if (result.byteLength > 0) {
stream.push(result);
pushAndCheck(stream, result);
}

if (isClosed) {
Expand Down
17 changes: 11 additions & 6 deletions src/runtime/webcore/FileReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -794,14 +794,19 @@ impl FileReader {
}

// No JS read is waiting; stop at the highwater mark. onPull restarts.
//
// `started` gates the backstop: a `from_pipe()` reader for non-lazy
// `Bun.spawn` is already reading when it arrives here, and throttling
// before any consumer has attached deadlocks a child that alternates
// stdout/stderr writes while the caller only awaits one of them.
Comment thread
robobun marked this conversation as resolved.
// SAFETY: see `reader_buffer` decl.
let reader_buffer_len = unsafe { (*reader_buffer).len() };
let ret = self.flowing.get()
&& !matches!(
self.read_inside_on_pull.get(),
ReadDuringJSOnPullResult::Temporary(_)
)
&& self.buffered.get().len() + reader_buffer_len < self.highwater_mark;
let ret = !matches!(
self.read_inside_on_pull.get(),
ReadDuringJSOnPullResult::Temporary(_)
) && (!self.started.get()
|| (self.flowing.get()
&& self.buffered.get().len() + reader_buffer_len < self.highwater_mark));
close_if_needed!();
ret
}
Expand Down
62 changes: 62 additions & 0 deletions test/js/node/child_process/child_process.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,68 @@ describe.skipIf(!isPosix)("stdout pipe backpressure", () => {
});
});

// child.stdout.pause() must stop the native reader so the kernel pipe fills
// and the child blocks on write. Previously, once the stream had flowed even
// once the native FileReader kept the poll armed (or uv_read_start active on
// Windows) regardless of JS state, so the child wrote its entire output into
// the parent's heap and 'data' kept firing after #handleOnExit resumed it.
it("child.stdout.pause() after flowing stops native reads and blocks the child", async () => {
// 20 MB: well above any kernel socket buffer, small enough to drain fast
// once resumed on ASAN.
const SIZE = 20 * 1024 * 1024;
const writer = `const c=Buffer.alloc(1<<20,97);let w=0;(function f(){while(w<${SIZE}){const n=Math.min(c.length,${SIZE}-w);w+=n;if(!process.stdout.write(c.subarray(0,n))){process.stdout.once('drain',f);return}}})()`;
const c = spawn(bunExe(), ["-e", writer], {
stdio: ["ignore", "pipe", "ignore"],
env: bunEnv,
});
try {
let events = 0;
let bytes = 0;
let eventsAfterPause = 0;
const { promise: firstData, resolve: gotFirst, reject: failFirst } = Promise.withResolvers<void>();
c.on("error", failFirst);
c.on("close", () => failFirst(new Error("child closed before first 'data'")));
c.stdout!.on("data", (d: Buffer) => {
events++;
bytes += d.length;
if (events === 1) {
c.stdout!.pause();
gotFirst();
} else {
eventsAfterPause++;
}
});
await firstData;

expect(c.stdout!.isPaused()).toBe(true);

// Give the eager-read path every chance to over-buffer: without the fix
// the native reader has already drained most of SIZE and the child is
// racing to exit. Deadline-polled; breaks early if the bug is present.
const deadline = Date.now() + 1000;
while (c.exitCode === null && Date.now() < deadline) {
await new Promise(r => setImmediate(r));
}

// Core assertion: pause() applied backpressure, so the child cannot have
// finished writing SIZE bytes and is blocked on a full pipe.
expect(c.exitCode).toBeNull();
expect(eventsAfterPause).toBe(0);
expect(c.stdout!.isPaused()).toBe(true);
// At most a few pipe-buffer-sized chunks were delivered before pause took
// effect, never the whole output.
expect(bytes).toBeLessThan(SIZE);

// Resuming delivers every byte and the child exits cleanly.
c.stdout!.resume();
await once(c, "close");
expect(bytes).toBe(SIZE);
expect(c.exitCode).toBe(0);
} finally {
c.kill();
}
});

// When spawn fails (ENOENT, bad cwd, etc.) the ChildProcess emits 'error' and
// 'close' but never 'exit'. The abort listener on options.signal was only
// removed on 'exit', so every failed spawn against a shared AbortSignal leaked
Expand Down
Loading