Jfm sync chan refactor #5289
Open
Jfm sync chan refactor #5289
+322
−63
Conversation
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
Format according to ols' odinfmt tool.
This fixes a flaw in the original implementation: the returned index is
actually useless to the caller.
This is because the index returned refers to the internal "candidate"
list. This list is dynamic, and may not have all of the input channels
(if they weren't ready according to chan.can_{recv,send}). That means
the index is not guaranteed to mean anything to the caller.
The fix introduced here is to return the index into the input slice
(recvs,sends) and an enum to specify which input slice that is.
If no selection was made, then (-1, .None) is returned to communicate as
much.
This follows the convention where non-blocking operations are prefixed with "try" to indicate as much. Since select_raw in it's current form doesn't block, it should be try_select_raw, and allow select_raw to name a blocking implementation.
Fixes a TOCTOU where the channel could be used between the call to
can_{recv,send} and {recv,send} causing an unexpected blocking
operation.
To do this we use the non-blocking try_{recv,send} and retry the check
in a loop. This guarantees non-blocking select behaviour, at the cost of
spinning if the input channels are highly contended.
Signed-off-by: Jack Mordaunt <[email protected]>
gingerBill
reviewed
Jun 6, 2025
|
|
||
| /* | ||
| Determines what operations `Chan` supports. | ||
| */ | ||
| Direction :: enum { | ||
| Send = -1, | ||
| Both = 0, | ||
| Both = 0, |
There was a problem hiding this comment.
Why was this formatting changed?
There was a problem hiding this comment.
It's my odinfmt from ols.
Can drop the formatting commit if the formatting is undesirable.
gingerBill
reviewed
Jun 6, 2025
| @@ -174,8 +180,15 @@ Example: | |||
| } | |||
| */ | |||
| @(require_results) | |||
| create_buffered :: proc($C: typeid/Chan($T), #any_int cap: int, allocator: runtime.Allocator) -> (c: C, err: runtime.Allocator_Error) | |||
| where size_of(T) <= int(max(u16)) { | |||
| create_buffered :: proc( | |||
There was a problem hiding this comment.
Why was this formatting changed?
gingerBill
reviewed
Jun 6, 2025
core/sync/chan/chan.odin
Outdated
Comment on lines
1260
to
1277
| if count == 0 { | ||
| return -1, .None | ||
| } | ||
|
|
||
| select_idx = rand.int_max(count) if count > 0 else 0 | ||
| candidate_idx := rand.int_max(count) if count > 0 else 0 | ||
|
|
||
| sel := candidates[candidate_idx] | ||
| if sel.is_recv { | ||
| status = .Recv | ||
| if !try_recv_raw(recvs[sel.idx], recv_out) { | ||
| continue | ||
| } | ||
| } else { | ||
| status = .Send | ||
| if !try_send_raw(sends[sel.idx], send_msgs[sel.idx]) { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
This appears to be the only change to the code.
There was a problem hiding this comment.
Two changes:
- name changed to try_select_raw
- toctou addressed by placing the body in a loop
Use a label to clarify the continue statements.
This makes the proc easier and safer to call by letting the caller nil out messages to skip sends.
This is necessary because we need to allow the test guarantee against a
rare condition: where a third-party thread steals a value between the
validity checks can_{send,recv} and the channel operation
try_{send,recv}.
This test harness ensures consistent non-blocking semantics and validates that we have solved the toctou condition. The __global_context_for_test is a bit of a hack to fuse together the test supplied proc and the executing logic in packaage chan.
|
I've pushed commits that add a test harness and some polish. To achieve the toctou test I introduced some global state (hidden behind a @gingerBill please let me know if you want me to revert the initial formatting commit (d28e54a) |
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 PR addresses #5287: renaming
select_rawtotry_select_rawand fixes the TOCTOU by looping until thecan_{send,recv}invariant holds.I recommend reviewing by commit to see each change in isolation.
TODO: I think this deserves a test harness. Currentlyselect_rawdoesn't seem to have a test.Tests added.