Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion src/js/builtins/ProcessObjectInternals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,32 @@ export function getStdioWriteStream(
stream.fd = fd;

const underlyingSink = stream[require("internal/fs/streams").kWriteStreamFastPath];
$assert(underlyingSink);
if (!underlyingSink && !stream._writableState.autoDestroy) {
// No FileSink: Writable parks every write after a failed one until destroy, so a failed write resets the stream.
const failures: unknown[] = []; // in order: a corked batch fails one write per chunk
const write = stream._write;
const destroy = stream._destroy;
stream._write = function (chunk, encoding, cb) {
write.$call(this, chunk, encoding, err => {
if (err) {
failures.push(err);
this._writableState.autoDestroy = true;
}
cb(err);
});
};
stream._destroy = function (err, cb) {
if (!err || err !== failures[0]) return destroy.$call(this, err, cb);
failures.shift();
if (failures.length === 0) this._writableState.autoDestroy = false;
this._undestroy();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Users whose stdout resets after a failed write that left more writes buffered get an end(cb) that never calls back; on the base cb got the EPIPE. The reset at src/js/builtins/ProcessObjectInternals.ts:112 keeps the old pendingcb: errorBuffer (src/js/internal/streams/writable.ts:670-687) fails each buffered write's callback without decrementing it, and nothing later does, so finishMaybe (writable.ts:882) never reaches finish. 'finish' and finished() never fire either. Fix: after the reset pendingcb must count only writes still outstanding, e.g. decrement it in errorBuffer per flushed write; add a test that ends the stream after the failed-writes scenario. This differs from the posted errorEmitted finding: its fix (bypassing cb) is in and does not touch pendingcb.

Why this was flagged

Trigger: process.stdout or process.stderr with no FileSink on a pipe or socket, the reader gone, and two or more write() calls in one synchronous run. The first write fails synchronously in underscoreWriteSync and onwrite sets kErrored, so every later write in that tick is buffered by writeOrBuffer (writable.ts:503-509) after _write did state.pendingcb++ (writable.ts:453). Next tick onwriteError decrements pendingcb once for the failed write, then errorBuffer (writable.ts:670-687) invokes each buffered callback with the EPIPE and resets the buffer without touching pendingcb. errorOrDestroy then destroys, the wrapper at ProcessObjectInternals.ts:108-118 calls _undestroy(), which resets ended, errored and closed but not pendingcb. A later end(cb) sets kEnding, finishMaybe skips finish because state.pendingcb === 0 is false (writable.ts:882); no later path decrements pendingcb, so 'finish' never emits, cb never runs, and finished() never settles. On the base branch the stream stays errored, so end(cb) delivers cb(state.errored) next tick (writable.ts:806-807). None of the new tests combine buffered failed writes with end().

Verification: Trigger: a no-FileSink process.stdout/stderr on a pipe/socket whose reader is gone, two or more write() calls before the reset lands, then end(cb). errorBuffer (writable.ts:670-687) never touches pendingcb; undestroy (destroy.ts:171-183) does not reset pendingcb, so finishMaybe's if (state.pendingcb === 0) (line 882) is false. Base: end(cb) runs process.nextTick(cb, state[kErroredValue]) (writable.ts:806-807).

// Not through cb: that marks the reset stream errorEmitted a tick later, and end() then never finishes.
process.nextTick(() => {
this.emit("error", err);
this.emit("close");
});
};
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return [stream, underlyingSink];
}

Expand Down
39 changes: 28 additions & 11 deletions src/js/internal/fs/streams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import type { FileSink } from "bun";
const { Readable, Writable, finished } = require("node:stream");
const fs = require("node:fs");
const { read, write, fsync, writev } = fs;
const { read, write, writeSync, fsync, writev } = fs;
const { FileHandle, kRef, kUnref, kFd } = (fs.promises as any).$data as {
FileHandle: { new (): FileHandle };
readonly kRef: unique symbol;
Expand Down Expand Up @@ -457,17 +457,17 @@ function WriteStream(this: FSStream, path: string | null | undefined, options?:
this.pos = start;
}

// A writer cannot be opened for every fd a caller may hand us -- a read-only
// descriptor is the common case, and node's tty.WriteStream accepts one. Fall
// back to the general path there, which surfaces the failure at write time the
// way node does, rather than throwing from the constructor.
// No writer for this fd (fd limit, macOS /dev/tty): write synchronously, so a bad fd fails at write time as in node.
let fastWriter;
if (fastPath && fd != null) {
try {
fastWriter = Bun.file(fd).writer();
} catch {
fastPath = false;
this._write = underscoreWriteSync;
}
// Already-open fd (stdio): born constructed, like node's stdio streams, which allocate no construct TickObjects.
this._construct = undefined;
}

// Enable fast path
Expand All @@ -477,12 +477,6 @@ function WriteStream(this: FSStream, path: string | null | undefined, options?:
this._writev = undefined;
this.write = writeFast as any;
this._final = finalFast;
if (fd != null) {
// Already-open fd (stdio): skip the async _construct round-trip so the
// stream is born constructed, like node's stdio streams (net.Socket /
// tty.WriteStream), which never allocate construct TickObjects.
this._construct = undefined;
}
}

Writable.$call(this, options);
Expand Down Expand Up @@ -602,6 +596,29 @@ function _write(data, encoding, cb) {
}
writeStreamPrototype._write = _write;

function underscoreWriteSync(this: FSStream, data: Buffer, encoding: any, cb: (err?: any) => void) {
const length = data.length;
let offset = 0;
let retries = 0;
while (offset < length) {
let written: number;
try {
written = writeSync(this.fd, data, offset, length - offset);
} catch (err: any) {
if (err?.code !== "EAGAIN") return cb(err);
// A non-blocking fd with no room: wait for the reader, as a blocking write would.
Bun.sleepSync(1);
Comment thread
robobun marked this conversation as resolved.
continue;
}
// Like writeAll: an fd that takes no bytes several times in a row fails the write.
if (written > 0) retries = 0;
else if (++retries > 5) return cb(new Error("write failed"));
this.bytesWritten += written;
offset += written;
}
Comment thread
robobun marked this conversation as resolved.
cb(null);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

function underscoreWriteFast(this: FSStream, data: any, encoding: any, cb: any) {
let fileSink = this[kWriteStreamFastPath];
if (!fileSink) {
Expand Down
84 changes: 84 additions & 0 deletions test/js/node/process/process-stdio-no-sink-fixture.mjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading