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

Remove impl Sync for FuturesUnordered #2051

Closed

Conversation

okready
Copy link
Contributor

@okready okready commented Jan 24, 2020

FuturesUnordered was previously modified to use interior mutability for
the len and head_all fields using Cell, but its Sync implementation was
left intact. This implementation is no longer valid, so it has been
removed.

Fixes #2050.

FuturesUnordered was previously modified to use interior mutability for
the len and head_all fields using Cell, but its Sync implementation was
left intact. This implementation is no longer valid, so it has been
removed.

Fixes rust-lang#2050.
@seanmonstar
Copy link
Contributor

I suspect code already depends on this bound, so it may be better to add the synchronization in instead of removing this bound, otherwise people's code will stop compiling in a patch update.

@okready
Copy link
Contributor Author

okready commented Jan 24, 2020

Good point. I realize simply wrapping the internals in a Mutex isn't ideal, but FuturesUnordered::push is a bit non-trivial, so it seems like a reasonable choice for now without requiring any major rewrites, and it shouldn't slow down polling or other &mut self methods since they can simply call get_mut on the mutex. Any objections?

@okready
Copy link
Contributor Author

okready commented Jan 24, 2020

Disregard that, I didn't realize Mutex (the thread type, not the futures type) is only in std.

@okready okready closed this Jan 24, 2020
@Pauan
Copy link

Pauan commented Jan 25, 2020

AtomicUsize is available in core (and it should be used anyways, because it's faster and smaller than Mutex).

@okready
Copy link
Contributor Author

okready commented Jan 25, 2020

Fixing push to work concurrently using atomics seems straightforward (len and head_all can be briefly out-of-sync without things breaking). There's also the interaction between push and iter though. iter simply grabs the current head_all and len values, but they might not match up if head_all and len aren't kept in sync with each other.

We could just ignore the len field when creating an IterPinRef and either compute the length manually when creating the iterator or lazily when calling size_hint. It could be costly for large FuturesUnordered lists, but it would avoid having to do any additional synchronization between the two fields.

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.

FuturesUnordered has an incorrect Sync impl
3 participants