Ensure single reader and writer to system fd on Unix#16209
Merged
straight-shoota merged 5 commits intocrystal-lang:masterfrom Dec 1, 2025
Merged
Conversation
98d7872 to
904dd95
Compare
Collaborator
Author
|
I split the fdlock in two different commits (refcount then serial R/W) that outline the different steps for merging as individual PRs. |
ysbaddaden
commented
Oct 14, 2025
ysbaddaden
commented
Oct 14, 2025
ysbaddaden
added a commit
to ysbaddaden/crystal
that referenced
this pull request
Oct 18, 2025
ysbaddaden
added a commit
to ysbaddaden/crystal
that referenced
this pull request
Oct 24, 2025
5 tasks
904dd95 to
6be2dd7
Compare
This was referenced Oct 28, 2025
6be2dd7 to
b92814a
Compare
Serializes reads and writes so we can assume any IO object will only have at most one read op and one write op. The benefits are: 1. it avoids a race condition in the polling event loops: - Fiber 1 then Fiber 2 try to read from fd; - Since fd isn't ready so both are waiting; - When fd becomes ready then Fiber 1 is resumed; - Fiber 1 doesn't read everything and returns; - Fiber 2 won't be resumed because events are edge-triggered; 2. we can simplify the UNIX event loops (epoll, kqueue, io_uring) that are guaranteed to only have at most one reader and one writer at any time.
b92814a to
72507a7
Compare
Collaborator
Author
Collaborator
Author
|
Usages are still restricted to I'll prepare a third PR that will:
|
straight-shoota
approved these changes
Nov 27, 2025
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This patch extends the fdlock to serialize reads and writes by extending the reference counted lock with a read lock and a write lock, so taking a reference and locking acts as a single operation instead of two (1. acquire/release the lock; 2. take/return a reference). This avoids a race condition in the polling event loops:
fd;fdisn't ready, both fibers start waiting;fdbecomes ready then Fiber 1 is resumed;With the read lock, fiber 2 will wait on the lock then be resumed by fiber 1 when it returns. A concrete example is multiple fibers waiting to accept on a socket where fiber 1 would keep handling connections, while fiber 2 sits idle.
The other benefit is that it can help to simplify the evloops that will now only deal with a single reader + single writer per
IOand is required for the io_uring evloop (the MT version requires it).NOTE: While this patch only serializes reads/writes on UNIX at the
Crystal::System, which is where the bugs are, wemay want to movewill move it into stdlib for all targetsat some point, for example to serialize reads and writes aroundSee #16289 (comment)IO::Buffered.Depends on #16288 and #16289.
Required by #16264.