This repository was archived by the owner on Aug 29, 2024. It is now read-only.
Fix race condition in fill_read_buffer#69
Closed
maruth-stripe wants to merge 1 commit into
Closed
Conversation
Member
|
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. |
Member
|
I am planning to archive this code base. The relevant code is moved to Feel free to create a new issue/pull request there if you feel strongly about this problem, we can continue the discussion. Thanks for your efforts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fill_read_bufferhas a race condition which can potentially lead to data loss. Consider the following sequence of events: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)@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.readof 16K bytes. This invokesfill_read_bufferwith a size ofBLOCK_SIZE(64K) bytes.@read_buffer@io.read_nonblock(size, input_buffer, exception: false)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
Contribution