Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Make sure to dup Array in Channel.select_impl #12827

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Dec 8, 2022

This patch fixes a bug in Channel.select_impl. The method duplicates the incoming collection ops as ops_locks for sorting. ops is an Indexable and can be an Array. Using #to_a is insufficient for ensuring duplication because Array#to_a returns self.
As a result, the original ops would be modified through sorting if its an Array. This is an unintended side-effect.
Furthermore, the returned index which indicates the position of the action that was matched would correspond to the index in the sorted array and thus not correspond to the original indices.

This bug probably does not have a huge effect unless you're using the internal Channel.select API directly. Public uses are for the implementation of the select keyword, but that's using a Tuple type.
Array is typically used as a parameter to Channel.receive_first and Channel.send_first, though. Those methods do not return the index (ref #10411). The side-effect of mutating is also irrelevant because the list of actions is only created in the implementation of these methods from a list of Channel.

For the latter use case, there is no need to duplicate the array. So I think we could introduce an optimization to avoid that, by extracting from select_impl a method which receives both ops and ops_locks (which can the point to the same collection).

@beta-ziliani beta-ziliani added this to the 1.8.0 milestone Jan 13, 2023
@straight-shoota straight-shoota merged commit 4e08bbc into crystal-lang:master Jan 14, 2023
@straight-shoota straight-shoota deleted the fix/channel-select-array branch January 14, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants