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 pin violation in much of concurrentstream #187

Merged
merged 3 commits into from
Jun 9, 2024

Conversation

conradludgate
Copy link
Contributor

Fixes #182

I developed the crate futures-buffered a couple years ago with the intent of being a more efficient 'FuturesUnordered' type than the one in futures_util.

The core of the unsoundness in #182 are the pin-violations caused by FutureGroup slab reallocs. futures-buffered solves this by using a 'triangular array' pattern, where each subsequent realloc is actually a new slab.

Note: the unsoundness still exists in FutureGroup. futures-buffered does not return keys for entries (although that is not a bad idea) so the API is not compatible.

A quick soundness fix for FutureGroup would be to make Stream require F: Unpin, albeit this is a breaking change.

Even if you do not merge this change, I hope it serves as a nice inspiration for you :)

Copy link
Owner

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great, thank you so much! - I'd like to eventually update our own {Future,Stream}Group types to not have the issues. And actually would prefer to keep external deps to a minimum.

But I'm also recognizing that we shouldn't let perfect be the enemy of good, and this actually fixes our issues straight away - so I'm keen to accept this patch and then figure out where we go after that.

If you could revert deleting the methods in future_group, I think we should be good to merge! Thank you so much!

src/future/future_group.rs Show resolved Hide resolved
@yoshuawuyts yoshuawuyts merged commit 9246a94 into yoshuawuyts:main Jun 9, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConcurrentStream usage with tokio leads to ACCESS_VIOLATION
2 participants