FuturesUnordered has an incorrect Sync impl

[okready]

FuturesUnordered switched to using Cell for interior mutability of the len and head_all fields in 47f3ccb, but it still currently retains a Sync implementation. Since there isn't any synchronization performed when these fields are modified when calling the push method, the Sync implementation is invalid and should be dropped.

It looks like this was mentioned in #1669, but was overlooked in the actual commit.

[cramertj]

Good catch! I'd be happy to accept a PR introducing atomics to handle this case correctly, as discussed in #2051.

[okready]

#2054 has a couple variations on a solution. There are some shortcomings to both, mostly with regards to how to deal with providing the appropriate length for iterators, so there may be another solution that's more suitable.

[tmiasko]

IMHO it would make sense to reconsider if having Sync for FuturesUnordered
is actually useful, independently of fixing the UB itself.

Currently in FuturesUnordered there is no separation between object that is
used to spawn additional futures into it, and object that is used for polling.
Since polling requires unique access, it means there is little room to actually
take advantage of Sync without introducing other form of synchronization.

[cramertj]

Currently in FuturesUnordered there is no separation between object that is
used to spawn additional futures into it, and object that is used for polling.
Since polling requires unique access, it means there is little room to actually
take advantage of Sync without introducing other form of synchronization.

Yes, it was not intended for FuturesUnordered to be Sync. One option would be to remove the Sync bound, but that is a semver breaking change and so would have to be part of a new release that wouldn't be automatically available by current users. I think it's important to provide current users with an upgrade that provides soundness without including breaking changes.