async-io
async-io copied to clipboard
Fix race condition in `fill_read_buffer`
fill_read_buffer has a race condition which can potentially lead to data loss. Consider the following sequence of events:
- init
- Fiber 1 issues a
readof 16K bytes. This invokesfill_read_bufferwith a size ofBLOCK_SIZE(say, 64K) bytes. @read_bufferis currently empty, so we call@io.read_nonblock(size, input_buffer, exception: false)- Suppose the underlying
@iois a socket, and the socket is empty. Then,@io.read_nonblockwill yield (on stable-v1 because of theasync_sendinstrumentation, and on main because of theio_readhook inrb_fiber_scheduler_io_read_memoryin the VM, leading toio_waitwhen the read would've blocked. - Suppose Fiber 2 issues a
readof 16K bytes. This invokesfill_read_bufferwith a size ofBLOCK_SIZE(64K) bytes. - Suppose the read succeeds, it consumes 16K bytes, leaving 48K bytes in
@read_buffer - Fiber 1 resumes, and retries
@io.read_nonblock(size, input_buffer, exception: false) - The read succeeds. But read_nonblock nukes the contents of
input_bufferleading to a data loss of 48K bytes.
This PR fixes this race condition by using a fresh buffer every time
cc @fables-tales
Types of Changes
- Bug fix.
Contribution
- [ ] I added tests for my changes.
- [ ] I tested my changes locally.
- [X] I agree to the Developer's Certificate of Origin 1.1.
In principle I agree with your assessment and the proposed fix. However, I question the premise - in what scenario do we read from the same stream using two fibers and how is any kind of correctness maintained? I think the expectation is only one fiber uses a stream - otherwise without any other synchronisation, I'm not sure what we are expecting.