Repository navigation
process.stdout: an empty write() calls back after the queued chunks - #45319
dylan-conway wants to merge 2 commits into
Conversation
…ks queued before it
|
Updated 3:53 AM PT - Oct 15th, 2026
❌ @dylan-conway, your commit 4518228 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 45319That installs a local version of the PR into your bun-45319 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughEmpty writes now pass through sink-specific handling. FileSink can return a pending write operation for an empty write. WriteStream tracks write promises to suppress repeated drain events. Tests cover empty writes, stream-pump output, and stdout callback ordering. ChangesFileSink write and drain behavior
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: empty process.stdout writes now complete after queued chunks. |
| Description check | |
The description includes both required sections. It explains the problem, cause, fix, scope, behavior changes, and verification results with relevant test coverage and limitations. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the kDrainedWrite identity check could swallow 'drain' on a later backpressure episode — it cannot, since Pending::run (streams.rs:433) replaces the future with None so the next backed-up write mints a fresh promise. I also checked the Writable::Pending(self.pending.as_ptr()) return in write_empty: it is the same shape as the existing to_result paths (FileSink.rs:1599, 1624) and resolves through WritableFuture::Promise { strong } to the same JS promise object, so the toBe(first) assertion is backed by the implementation rather than coincidence.
Extended reasoning...
The change touches the generic JS sink write binding, FileSink's empty-write behavior, and the fs WriteStream drain emission, with tests in both the FileSink and process stdio suites; it touches no auth, crypto, or injection surface. The inline findings already call for a human look; this note only records two adjacent concerns that were traced and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/internal/fs/streams.ts— Aprocess.stdout.write(chunk, cb)that is not backpressured still runscbsynchronously insidewrite(), so after this PR the callback timing matches Node only for the backpressured case. writeFast at streams.ts:707-709 callscb(null)directly when the sink returns a number; Node's Writable defers every callback withprocess.nextTick(afterWriteTick). The PR's own table fixes "cb2 runs inside write()" for the empty chunk but leaves it for every non-full-pipe write, sowrite("x", () => write("x", ...))recurses on the stack in Bun and not in Node. Fix: defer the non-pending callback withprocess.nextTick(cb, null)in writeFast (as underscoreWriteFast already does at streams.ts:638), and cover it with a test that asserts the callback runs afterwrite()returns.Why this was flagged
Trigger: any
process.stdout.write(data, cb)orprocess.stderr.write(data, cb)while the fd is not backed up, entering writeFast (streams.ts:670) because ProcessObjectInternals.ts:55 and tty.ts:116 construct stdio with$fastPath: trueand an fd, which setsthis.write = writeFastat streams.ts:480. fileSink.write returns a number, and streams.ts:708 invokescb(null)synchronously beforewrite()returns. Node always calls the callback asynchronously (onwrite with sync=true schedules afterWriteTick via process.nextTick), so a callback that re-enters write recurses unboundedly in Bun, and code that sets state afterwrite()returns observes the callback before that state is set. The siblingunderscoreWriteFastat streams.ts:638 already defers withprocess.nextTick(cb, null), so the two fast paths disagree. This is unchanged from the base branch, but the PR claims Node-equivalent callback ordering forprocess.stdout.write(chunk, cb)and changed the adjacent line (streams.ts:696-697) without fixing the sibling branch. Remedy:process.nextTick(cb, null)at streams.ts:708.Verification: pre-existing. Trigger: any
process.stdout.write(data, cb)while the sink is not backed up. writeFast at src/js/internal/fs/streams.ts:690-710 runscb(null); return true;synchronously at lines 708-709. The lines are byte-identical in the base, so this is pre-existing, not a regression. The new test (process-stdio.test.ts) only exercises the full-pipe path.
| /// `write()` of an empty chunk, which never reaches the `write_*` methods. | ||
| fn write_empty(&mut self) -> streams::result::Writable { | ||
| streams::result::Writable::Temporary(0) |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: A direct-stream pull that awaits controller.write("") on a backed-up HTTP response or S3/fetch upload still resolves at once, while the same call on a FileSink now parks until drain. The trait default at Sink.rs:374-376 answers Temporary(0) without asking the sink, and only FileSink.rs:1502 overrides it. HTTPServerWritable (streams.rs:1386-1397) and NetworkSink (streams.rs:2448-2457) both hold a pending promise for JS controllers exactly like FileSink. Fix: make an empty write consult the pending slot in every sink that can be backed up (HTTPServerWritable, NetworkSink), or implement it once in the shared write() from the sink's pending state.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A user Bun.serve type: "direct" ReadableStream, or a direct stream feeding an S3/fetch upload, calls await controller.write("") while the socket is backed up (streams.rs:1387 has_backpressure). The generic host write() at Sink.rs:548-551 and Sink.rs:568-571 calls write_empty(), which for those sinks is the trait default at Sink.rs:374-376 returning Temporary(0). The caller resolves immediately and treats the sink as flushed, even though those sinks park JS controllers on self.pending (streams.rs:1395-1397, streams.rs:2455-2457) exactly as FileSink does. On the base branch all sinks answered 0; after this PR FileSink parks and the others do not, so the same code behaves differently per sink. The dismissal relied on the PR description scoping the change; REVIEW.md requires fixing the sibling sites (every JsSinkType with a pending promise) in the same PR. Remedy: override write_empty in HTTPServerWritable and NetworkSink to return Writable::Pending(&mut self.pending) when pending.state == Pending, or hoist that check into JSSink<T>::write.
Verification: Trigger: a type: "direct" stream calls controller.write("") while the destination is backed up. The write_empty default at Sink.rs:375-377 is Temporary(0) without consulting the sink. Only FileSink.rs:1502-1507 overrides it; HTTPServerWritable (streams.rs:1386-1398) and NetworkSink (streams.rs:2448-2458) park non-empty writes the same way, so there an empty chunk returns 0 during backpressure. Pre-existing.
There was a problem hiding this comment.
Left as it is on purpose. On FileSink the 0 was a visible defect because node:fs streams read a number as "flushed" and run the write callback. These two sinks have no such consumer: an empty chunk adds no bytes to hold back and end() still flushes, so nothing is lost. Returning the promise whenever the slot is Pending is also the shape that hung the pump on FileSink (other thread), and writable_result(0) / backpressure_result(0) would overwrite the value the outstanding write's promise resolves to. The exclusion is stated in the PR description.
…so a piped stream with an empty chunk no longer hangs
There was a problem hiding this comment.
The is_backed_up() gate and the constructor-seeded kDrainedWrite slot in 4518228 address both points from the previous push. Beyond the new inline findings, I also checked whether deduping 'drain' on promise identity could swallow a later legitimate 'drain' across separate backpressure episodes: WritablePending::run (streams.rs:424-433) replaces the future with None, so the next backpressured write gets a fresh promise and the dedupe only collapses chunks that share one promise.
Extended reasoning...
The second push changed write_empty in src/runtime/webcore/FileSink.rs to route through to_result(WriteResult::Pending(0), 0) only while the writer is backed up, and seeded the new slot in the WriteStream constructor; the remaining inline findings concern the pending-slot result overwrite and the backed-up flag after an asynchronous error, which are new to this version. The drain-dedupe check above was ruled out from the pending-future reset in src/runtime/webcore/streams.rs.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
| fn write_empty(&mut self) -> streams::Writable { | ||
| if self.done.get() || !self.writer.get().is_backed_up() { | ||
| return streams::Writable::Temporary(0); | ||
| } | ||
| self.to_result(WriteResult::Pending(0), 0) | ||
| } |
There was a problem hiding this comment.
🔴 A writer whose pipe broke can now see the broken-pipe error replaced by a successful byte count, so process.stdout.write callbacks get null instead of EPIPE. When a non-empty write hits a synchronous EPIPE behind a backpressured write, to_result at FileSink.rs:1595-1599 parks Err in the pending slot and leaves done false and the writer backed up. An empty write in the same tick then runs write_empty at FileSink.rs:1506, whose to_result Pending arm at FileSink.rs:1620-1623 overwrites that Err with Owned(consumed). Fix: when the pending slot is already Pending, return Writable::Pending(self.pending.as_ptr()) without touching p.result (or skip the overwrite when the result is Err), so the queued error still rejects the shared promise as it does on the base branch.
Why this was flagged
Entry point: sink.write(big) returns pending promise P while the pipe is full; the reader closes; sink.write("y") appends to outgoing and try_write returns EPIPE (PipeWriter.rs:1012, PipeWriter.rs:138). FileSink.rs:1595-1599 stores Writable::Err(err) in the slot and hands back P; done is not set and backed_up is still true. Still in the same tick, sink.write("") reaches write_empty at FileSink.rs:1502-1507: done is false and is_backed_up() is true, so it calls to_result(Pending(0), 0), and the Pending arm at FileSink.rs:1620-1623 sets p.result = Owned(p.consumed), discarding the error. run_pending then resolves P with the byte count, and the later on_error at FileSink.rs:473 does nothing because the slot is already Used. The caller awaiting P, and every stdout callback behind it, sees success for bytes that were never delivered. On the base branch the empty write returned 0 (old Sink.rs:551/570) without touching the slot, so P rejected with EPIPE.
Verification: Triggered when a non-empty write hits a synchronous EPIPE while an earlier write's promise is still pending and an empty chunk is written before the deferred settle runs. write_empty reaches the Pending arm (FileSink.rs:1620-1623), which does p.result = streams::Writable::Owned(p.consumed), overwriting the parked Err. On the base branch the empty write returned the literal 0 (old Sink.rs:551/570).
| fn write_empty(&mut self) -> streams::Writable { | ||
| if self.done.get() || !self.writer.get().is_backed_up() { | ||
| return streams::Writable::Temporary(0); | ||
| } | ||
| self.to_result(WriteResult::Pending(0), 0) | ||
| } |
There was a problem hiding this comment.
🔴 Callers who write an empty chunk to a FileSink after a backpressured write failed asynchronously (reader closed, EPIPE) now get a promise that never settles, and the sink can leak. The is_backed_up() check at FileSink.rs:1503 reads a flag that _on_error (PipeWriter.rs:822) never clears, so after the writer has closed, to_result(Pending(0)) creates a fresh promise no callback will ever run. It also re-takes the keep-alive ref at FileSink.rs:1604-1607 when on_close already released it. Fix: park an empty write only while bytes are still outstanding on a live writer (e.g. also require has_pending_data() and not is_done, or clear backed_up in _on_error), returning 0 otherwise as the base does.
Why this was flagged
Entry point: a user holds a FileSink with a write parked on a full socket or pipe, the reader closes, and later the user calls sink.write("") or sink.write(new Uint8Array(0)). The poll drains at PipeWriter.rs:180-203, try_write fails with EPIPE, on_error at PipeWriter.rs:822-830 sets is_done but leaves backed_up (set true at PipeWriter.rs:776 by the earlier Pending) untouched, then FileSink::on_error (FileSink.rs:463-487) rejects the parked promise and close() runs on_close, which releases the keep-alive ref at FileSink.rs:554. The empty write then reaches write_empty at FileSink.rs:1502-1507: done is false and is_backed_up() is still true, so to_result(WriteResult::Pending(0), 0) runs: ref() is taken at FileSink.rs:1604-1607 with no on_close left to release it, and WritablePending::promise (streams.rs:399-418) allocates a new promise. No on_write/on_error can arrive on a closed writer, so that promise never settles. On the base branch the empty write returned the literal 0 (old Sink.rs:551/570). The native leak test at test/js/bun/util/filesink.test.ts:507 does not cover this because it never writes again after the rejection.
Verification: The async error path src/io/PipeWriter.rs:822-830 sets is_done = true but never touches backed_up, set true at src/io/PipeWriter.rs:776. FileSink::on_error (FileSink.rs:463-487) never sets self.done. write_empty (FileSink.rs:1502-1507) so calls to_result(WriteResult::Pending(0), 0), taking self.ref_() again and creating a fresh promise nothing settles. On the base branch Sink.rs:551/570 returned the literal 0.
| expect(first).toBeInstanceOf(Promise); | ||
| expect(second).toBe(first); | ||
| expect(await second).toBe(size); | ||
| expect(sink.write(empty)).toBe(0); |
There was a problem hiding this comment.
🟡 nit (optional): the done clause of write_empty (FileSink.rs:1503) has no test; deleting it leaves every new test green. The socketpair test asserts 0 only while idle, never after end() returned a pending promise, which is the one state where done decides the answer. Fix: after a backpressured write, call sink.end() (it returns the pending promise) and assert sink.write(empty) is 0 before awaiting it, so the ended-sink contract the PR states is covered.
Why this was flagged
FileSink.rs:1502-1507 returns Temporary(0) when self.done.get() is true, otherwise falls through to is_backed_up(). done becomes true while is_backed_up() is still true only in the end paths (end_from_js Pending arm at FileSink.rs:1417-1424, end() at FileSink.rs:1319-1326, on_auto_flush Err arm at FileSink.rs:964). The new socketpair test (test/js/bun/util/filesink.test.ts:276-308) checks sink.write(empty) is 0 at line 286 (idle) and line 299 (after the backpressured promise resolved), and only then calls sink.end() in finally. The FIFO pump test (lines 1274-1302) never ends the sink while backed up either. With the self.done.get() || clause removed, both tests still pass, so the ended-sink behaviour the description promises is unverified; REVIEW.md asks that each load-bearing clause of a fix break at least one test.
Verification: src/runtime/webcore/FileSink.rs:1502-1507 returns Temporary(0) when self.done.get() || !self.writer.get().is_backed_up(). The done clause only decides the answer when done is true while the writer is still backed up. No new test reaches it: sink.end() is only called in finally (line 303) with no subsequent write(empty). So deleting self.done.get() || leaves every test green.
| fn write_empty(&mut self) -> streams::Writable { | ||
| if self.done.get() || !self.writer.get().is_backed_up() { |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: a small process.stdout.write(chunk, cb) behind a short write whose refused tail is under 4 KiB still returns true and runs cb at once, and the earlier write's promise resolves while that tail sits unwritten; exiting from cb loses it. PipeWriter.rs:989-1003 buffers the chunk and reports Drained, which clears backed_up (PipeWriter.rs:776) and makes FileSink.rs:405-428 settle the pending promise. The is_backed_up() guard at FileSink.rs:1503 is then false for a following write("") as well. Fix: while refused bytes are queued, a buffered write must not report Drained or settle the slot; report Pending (or gate the settle at FileSink.rs:405 on !has_pending_data) so write() returns the promise for "" and non-empty chunks alike.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: process.stdout on a non-TTY pipe or socket; a write is refused partway leaving a tail smaller than CHUNK_SIZE (4096, PipeWriter.rs:746) in outgoing, e.g. write(Buffer.alloc(65536 + 1000)) to an empty 64 KiB pipe. The next write("y", cb) enters PipeWriter.rs:989: should_buffer(1) is true (1000 + 1 < 4096), so it appends and calls parent_on_write(1, WriteStatus::Drained) at PipeWriter.rs:998, which sets backed_up=false at PipeWriter.rs:776 and enters FileSink::on_write. There status is Drained so the early return at FileSink.rs:401 is skipped; was_pending is true, so FileSink.rs:423-428 settles the first write's promise while 1001 bytes are still only in outgoing. write() then returns Wrote(1) -> 1, writeFast at streams.ts:709 calls cb synchronously and returns true. process.exit(0) inside cb exits before the tail is flushed. On the base branch the same happens; after one such buffered write the new guard at FileSink.rs:1503 sees is_backed_up()==false and write("", cb) again calls back at once with the tail queued. The PR's 1 MiB test leaves a tail far above 4 KiB, so it never reaches this path.
Verification: pre-existing — the base already fails the same way by the same route for a non-empty small write, and the PR's new is_backed_up() guard inherits the hole. PipeWriter.rs:989-1003 appends to outgoing, calls parent_on_write(1, Drained) — which sets backed_up = false at line 776 — and returns Wrote(1). None of PipeWriter.rs:776/815/989-1003 or FileSink.rs:401-428 are touched by the diff.
This branch has not been deployed
2 participants
What does this PR do?
process.stdout.write("", cb)rancbat once even while earlier chunks were still waiting for a full pipe, so exiting from the callback lost them:Cause.
FileSink.write()returns a promise while the sink is backed up and a number otherwise. The sharedwrite()binding answered0for an empty string or buffer without asking the sink, andprocess.stdoutreads a number as "flushed".Fix. An empty chunk now asks the sink. A backed up
FileSinktreats it like any other chunk written then: same promise, same resume signal for a piped stream. An idle or endedFileSinkstill returns0.Chunks written while the sink is backed up share one promise, and each of them emitted its own
'drain'. The empty write would have joined in, so'drain'is now emitted once per promise, which also fixes the non-empty case.write(big, cb1); write(chunk, cb2)to a full pipe:chunk"",Buffer.alloc(0)true,cb2runs insidewrite()false;drain, cb1, cb2"\n"drain, cb1, drain, cb2drain, cb1, cb2Not changed. The HTTP response and S3 sinks still return
0for an empty chunk. Nothing reads that number as "flushed" there, andend()flushes.How did you verify your code works?
process-stdio.test.ts: the three new cases fail on 1.4.3 and pass withbun bd test. With only the'drain'change undone, all three fail again.filesink.test.ts: the new tests pass withbun bd test(the file cannot load on a release build). A stream with an empty chunk piped into a FIFO completes; it hangs ifFileSinkhands out the promise while it is merely unsettled rather than backed up.process.stderr, and a'drain'listener that writes again give node v26.7.0's output.