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
12 changes: 12 additions & 0 deletions src/runtime/webcore/FileReader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,18 @@ impl Lazy {
}
}

// Off Linux a pipe reader that wakes after another registration took the bytes blocks in `read`.
#[cfg(unix)]
let two_can_poll = cfg!(any(target_os = "linux", target_os = "android"))
|| this.file_type == FileType::Socket;
// An event loop keeps one registration per fd number, so a reader that polls a stdio fd takes a dup.
#[cfg(unix)]
let fd = if two_can_poll && this.pollable && fd.stdio_tag().is_some() {
Comment thread
robobun marked this conversation as resolved.
Resolved
sys::dup_at_least(fd, 3).unwrap_or(fd)
Comment on lines +249 to +250

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.

🔴 Programs with a live process.stdin listener now silently stop receiving input once any other stream over fd 0 starts, where the base raised EEXIST. src/runtime/webcore/FileReader.rs:249 gives each pollable stdio reader its own dup, so two readers hold two epoll registrations on one pipe and whichever wakes first drains it (src/io/PipeReader.rs:690); the other gets EAGAIN and re-arms. On Linux the newer registration wakes first, so the earlier consumer gets nothing and no error. Fix: a second live reader over the same stdio file must either fail with a named error or get a copy of the bytes through one shared registration, not race for them. The description lists this as a decision still open; this is the cost of the sharing option. [also at: src/runtime/webcore/FileReader.rs:249 - After merging, a program listening on process.stdin can silently receive nothing once any other code opens a stream over fd 0; today that second stream fails loudly with EEXIST.]

Why this was flagged

A program registers process.stdin.on("data", ...) (reader A), then later something starts Bun.file(0).stream(), Bun.stdin.stream(), a Response(Bun.file(0)).body or a fetch upload body (reader B) while A is still listening. On the base branch B's register_with_fd (src/io/PipeReader.rs:547) fails with EEXIST from epoll_ctl and B's stream errors, so A keeps every byte and the program sees a loud error. After this change src/runtime/webcore/FileReader.rs:249-250 hands B dup_at_least(fd, 3), so A and B each hold an EPOLLONESHOT registration on the same open file description. One write readies both; the reader Bun services first calls read_nowait (src/io/PipeReader.rs:690) and takes the whole pipe, the other gets EAGAIN, maps it to Stop::WouldBlock (src/io/PipeReader.rs:728) and re-arms. Linux wakes the most recently added epoll entry first, so B (added later) is serviced first on every write and A's listener receives nothing, with no error or warning on either side.

Verification: The PR description's "Decision needed" section and its table row record exactly this outcome, and the new test at test/js/web/streams/streams.test.js:3010-3023 pins only that the concatenated text of both readers equals the input ("each byte goes to one"), not that the earlier listener keeps receiving.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is correct, and it is the question at the top of the description. I measured the same result. On a release build of this branch the later stream takes all 35 bytes from a listening process.stdin in 5 of 5 runs, with no error. On main process.stdin keeps all 35 and the stream rejects with EEXIST: file already exists, epoll_ctl.

The costs of each answer, as far as I measured or can read from the code:

  • Share (this PR). The later reader usually wins, with no error. Two Bun.file(fd > 2).stream() readers behave this way on main.
  • Refuse with a named error. The earlier reader keeps every byte. A stream that starts after process.stdin.pause() or rl.close() is refused too, because that reader is paused and not closed. The second bun test --isolate file that reads process.stdin keeps failing, with a clearer error. kqueue reports no collision, so a refusal needs a per-VM record of which reader polls fd 0, 1 or 2. The first shape of this change had that record. I can put it back with a refusal in place of the dup.
  • A copy for each reader through one registration. A reader that is left without cancel() then holds every later byte in memory, or it stops the other readers.

I leave this thread open for a maintainer to choose.

} else {
fd
};

this.fd = fd;

Ok(this)
Expand Down
103 changes: 101 additions & 2 deletions test/js/node/process/process-stdin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,85 @@ test.concurrent("pause() and resume() churn while data is in flight never destro
expect(exitCode).toBe(0);
});

// A paused process.stdin takes its fd out of the event loop and puts it back on
// resume(). That fd was fd 0 itself, so a stream that started during the pause
// registered fd 0 too and the resume failed with EEXIST from epoll_ctl. Windows
// reads a stdin pipe through libuv and is not covered here.
test.skipIf(isWindows).concurrent("resume() works after another stream read stdin during the pause", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const chunk = data => console.log("chunk " + Buffer.from(data).toString());
const ends = [];
const end = name => {
if (ends.push(name) === 2) console.log(ends.sort().join(", "));
};
process.stdin.on("error", error => {
console.log("process.stdin error " + error.code + " " + error.syscall);
process.exit(1);
});
process.stdin.on("end", () => end("process.stdin end"));
process.stdin.once("data", first => {
chunk(first);
process.stdin.pause();
// pause() stops reading on the next tick. The stream starts after that.
setImmediate(async () => {
const reader = Bun.file(0).stream().getReader();
console.log("stream started");
try {
for (let resumed = false; ; ) {
const { done, value } = await reader.read();
if (done) break;
chunk(value);
if (!resumed) {
resumed = true;
process.stdin.on("data", chunk).resume();
}
}
end("stream end");
} catch (error) {
console.log("stream error " + error.code + " " + error.syscall);
process.exit(1);
}
});
});
`,
],
stdin: "pipe",
stdout: "pipe",
stderr: "pipe",
env: bunEnv,
});
const stderr = proc.stderr.text();
const reader = proc.stdout.getReader();
const decoder = new TextDecoder();
let stdout = "";
// "two" arrives during the pause, so the stream gets it. Either reader can get "three".
stepping: for (const [input, line] of [
["one", "stream started\n"],
["two", "chunk two\n"],
["three", "chunk three\n"],
]) {
proc.stdin.write(input);
await proc.stdin.flush();
while (!stdout.includes(line)) {
const { value, done } = await reader.read();
if (done) break stepping;
stdout += decoder.decode(value, { stream: true });
}
}
await proc.stdin.end();
for (let r; !(r = await reader.read()).done;) stdout += decoder.decode(r.value, { stream: true });

expect({ stdout, stderr: await stderr, exitCode: await proc.exited }).toEqual({
stdout: "chunk one\nstream started\nchunk two\nchunk three\nprocess.stdin end, stream end\n",
stderr: "",
exitCode: 0,
});
});

// The native FileReader source over a pollable pipe used to drain the fd to
// EAGAIN regardless of JS demand, so an idle consumer still ingested the whole
// pipe into an internal buffer. The kernel pipe buffer filling up is the
Expand Down Expand Up @@ -596,17 +675,37 @@ describe.skipIf(isWindows)("pipe backpressure", () => {

// Stopping unregisters a fired one-shot poll without a syscall, which leaves
// its disarmed registration in the kernel. Cancelling the stream must still
// take it out: fd 0 stays open, and the next poll on it would hit EEXIST.
test.concurrent("a reader cancelled at the highwater backstop frees fd 0 for the next reader", async () => {
// take it out. The reader polls a dup of fd 0, and epoll keeps a registration
// for as long as the file is open, which it is through fd 0. The dup of the
// next reader gets the same number once the first is closed, and would hit EEXIST.
test.concurrent("a reader cancelled at the highwater backstop frees its fd for the next reader", async () => {
const { second } = await run(`
const { getEventLoopStats } = require("bun:internal-for-testing");
const { fstatSync } = require("node:fs");
const stdin = fstatSync(0);
// Whether a descriptor other than fd 0 is open on stdin.
function hasDup() {
for (let fd = 3; fd < 64; fd++) {
try {
const { dev, ino } = fstatSync(fd);
if (dev === stdin.dev && ino === stdin.ino) return true;
} catch {}
}
return false;
}
// Not top-level await: an unsettled entry-module promise would keep the process alive by itself.
(async () => {
const reader = Bun.stdin.stream().getReader();
await reader.read();
// The parent keeps the pipe full, so the reader soon stops and lets go of the loop.
while (getEventLoopStats().loopActive) await new Promise(resolve => setImmediate(resolve));
await reader.cancel();
// The reader closes its dup on the thread pool.
const deadline = performance.now() + 2000;
while (hasDup()) {
if (performance.now() > deadline) throw new Error("the cancelled reader still holds a descriptor on stdin");
await new Promise(resolve => setImmediate(resolve));
}
const next = await Bun.file(0).stream().getReader().read();
process.stdout.write(JSON.stringify({ second: (next.value?.length ?? 0) > 0 }));
process.exit(0);
Expand Down
Loading
Loading