fix(fs): WriteStream pending write fastpath
#16856
Conversation
|
Updated 4:23 AM PT - Jan 31st, 2025
❌ @dylan-conway, your commit 2de9cc2 has 2 failures in
🧪 try this PR locally: bunx bun-pr 16856 |
src/io/PipeWriter.zig
Outdated
| // TODO: | ||
| chunk_size: usize = 0, | ||
| // TODO: configurable? | ||
| const chunk_size: usize = 1024 * 4; |
There was a problem hiding this comment.
should be configurable, though it doesn't have to block this PR.
node:fs streams have this option. we can add an options argument into Bun.file().writer()
https://nodejs.org/api/fs.html#filehandlecreatewritestreamoptions
src/bun.js/webcore/streams.zig
Outdated
| @@ -3841,6 +3891,7 @@ pub const FileSink = struct { | |||
| pub fn deinit(this: *FileSink) void { | |||
| this.pending.deinit(); | |||
| this.writer.deinit(); | |||
| AutoFlusher.unregisterDeferredMicrotaskWithType(@This(), this, JSC.VirtualMachine.get()); | |||
There was a problem hiding this comment.
This needs to not run when it's the shell
src/bun.js/webcore/streams.zig
Outdated
| if (amt == 0) { | ||
| this.updateRef(false); | ||
| } else { | ||
| this.runPending(); |
There was a problem hiding this comment.
We want to run pending when:
done- If we wrote part of the data
Instead, we should runPending when !this.writer.hasPendingData()
If this leads to a timeout in tests, it's likely due to not draining microtasks since this is being called after the microtask queue is over. If the developer immediately calls write every time the write Promise fulfills, that could maybe lead to an infinite loop but we will have to see.
I think both of these branches if (amt == 0) and this one should be removed. And instead
What does this PR do?
Fixes out-of-order writes when
EAGAINis received.Closes #16823
How did you verify your code works?
Added a vite integration test