Fix FIFO data loss (#8695) #8747
Conversation
argosphil
force-pushed
the
issue-8695-002
branch
from
f5d175a to
41cfb2a
Compare
|
Added a test which detects #8695 . |
|
Looks like |
Under certain circumstances, we would read data from FIFOs without a sensible place to put it, leading to dropped chunks in async iteration over Bun.stdin.stream(). Fixes oven-sh#8695.
This test seems to fail reliably with the current bun release. Uses
stderr rather than stdout because bun-debug currently outputs debug
information to stdout and I didn't want to filter it.
The test simply counts to 20, taking 100 ms for each write, while the
reader consumes chunks and takes one second to asynchronously process
each one.
Due to the timing-sensitive nature of the tested code, it does take 3
seconds to run, which I think is acceptable.
Output with current bun release:
490 | let text = "";
491 | for await (const chunk of producer.stderr) {
492 | text += [...chunk].map(x => String.fromCharCode(x)).join("");
493 | await new Promise(r => setTimeout(r, 1000));
494 | }
495 | expect(text).toBe("0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17\n18\n19\n20\n");
^
error: expect(received).toBe(expected)
Expected: "0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17\n18\n19\n20\n"
Received: "0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n11\n12\n13\n14\n15\n16\n17\n18\n19\n"
at .../bun/test/js/bun/io/bun-write.test.js:495:3
(fail) timed output should work [3022.97ms]
This should fix failures of the new test in CI because of escape sequences injected by console.error. Once bun-debug is fixed to print debug output to stderr, this test will need adjusting.
argosphil
force-pushed
the
issue-8695-002
branch
from
93691f7 to
bfef103
Compare
|
We have a big WIP PR (#8456) that's supposed to fix a lot of the stuff related to reading from pipes and spawning processes, but I just verified that this issue can be reproduced there, too. I think it makes sense to try to fix it there immediately, I will take a look tomorrow. |
Thanks for checking! I think the test from this PR might be salvageable, but feel free to close it if you prefer, I can open a new PR once the dust has settled. |
|
Yeah the test is good, I'll move it over to the new PR so that we make sure to cover it there |
|
Okay, closing this, then. |
Under certain circumstances, we would read data from FIFOs without a sensible place to put it, leading to dropped chunks in async iteration over Bun.stdin.stream().
Fixes #8695.
What does this PR do?
This changes
FIFO.readyso it doesn't read unconditionally from the FIFO's FD. Instead, we only do so if there is a pending Promise or handler that will receive the data.How did you verify your code works?
This issue is sensitive to timing and I don't know how to write automated tests for it.I wrote a test.
I included a test for the new code, or existing tests cover it
I ran my tests locally and they pass (
bun-debug test test-file-name.test)I included a test for the new code, or an existing test covers it
I wrote TypeScript/JavaScript tests and they pass locally (
bun-debug test test-file-name.test)