GenericSemaphoreAcquireFuture isn't Sync

[dekellum]

This seems related to #3. Was Sync for this type just an oversight, or might it be safely added? I guess the question is, if concurrent access to a *mut ListNode<WaitQueueEntry? could be safe?

Current compile error:

the trait `std::marker::Sync` is not implemented for `*mut futures_intrusive::intrusive_singly_linked_list::ListNode<futures_intrusive::sync::semaphore::WaitQueueEntry>`
    = note: required because it appears within the type `futures_intrusive::intrusive_singly_linked_list::ListNode<futures_intrusive::sync::semaphore::WaitQueueEntry>`
    = note: required because it appears within the type `futures_intrusive::sync::semaphore::GenericSemaphoreAcquireFuture<'static, parking_lot::raw_mutex::RawMutex>`

[dekellum]

As I realize that Send is commonly sufficient for a Future, let me give some more context. I encountered the Sync bounds because I'm implementing custom Streams that internally use the Semaphore and need to support wrapping in a hyper::Body. As of v0.13.0 hyper requires such streams to be Sync: hyperium/hyper#1857. This requirement seems to have been added as an effective workaround for rust-lang/rust#57017 (which is now viewed as a sub-case of rust-lang/rust#59245).

As its unclear if hyper really needs Sync, I've experimented with a new-type around GenericSemaphoreAcquireFuture with unsafe impl Sync. This works in my testing but obviously gives me pause from a safety perspective. Safer solutions would be to de-optimize this wrapped case, by not using the Semaphore, or wrapping the *AcquireFuture in a (uncontended) Mutex to safely make it Sync.

[Matthias247]

Making Futures Send as described in #3 was required in order to make them usable in multithreaded executors, which move tasks (and thereby all Futures inside a task) between poll invocations due to work-stealing.

However there is no real use-case that requires them to be Sync. That would enabling calling Future::poll() from multiple threads at once, which would be quite weird for most Futures. From a user perspective they are mostly expected to call semaphore.acquire(1).await; - which involves no explicit multithreading.

GenericSemaphoreAcquireFuture seems to be actually thread-safe in its current implementation. However I am still not looking forward to make it actually Sync, because

• other Futures in the library are not thread-safe, since they also perform operations outside of the section which is synchronized through the synchronization primitives Mutex. And all Futures in the library should behave consistently.
• it would introduce additional complexity, and make it harder to change the Futures implementation in the future.
• as mentioned, I do not really see a use-case for it

I think it rather be good to ask on the hyper repository whether that bound is really required. According to rust-lang/futures-rs#1752 it might be able to resolve it by using another Box. A Sync requirement on a Stream also seems to prevent other common usages - e.g. of using a buffered reader. I'm seeing that tokio::io::BufReader is actually marked as Sync, but I don't understand how it actually could be - given that it modies it's internal state in each read call. Maybe it has something do to with pinning, but that is beyond me at the moment.

[dekellum]

Thanks. Yes, the use case I describe is situational and hopefully temporary, though I suspect hyper won't change its Sync bound until fixes are made in rustc, and the path forward with those isn't extremely clear.

Since Sync is an auto trait, I'm not sure users could reasonably expect consistency across all Futures. However, to unsafe impl Sync for this specific Future requires proving it safe and I'm similarly not sure it worth it.

I've implemented a Sync wrapper in safe code here and so far I haven't measured any significant performance difference for my use case.

[Matthias247]

Since Sync is an auto trait, I'm not sure users could reasonably expect consistency across all Futures.

The "consistency" statement was just about Futures which are produced inside this library, which
follow the same patterns.

I've implemented a Sync wrapper in safe code here and so far I haven't measured any significant performance difference for my use case.

I think that's fine to do for a particular use-case! I guess it would show up in the Mutex benchmark that is part of the repo, because even uncontended atomics ops are not free. But it's unlikely that it would cause a real issue in a particular application (although it obviously depends on what you use the Semaphore for).

[dekellum]

Thanks. I think for my use case as described above, an uncontended mutex lock is unlikely to be detectable in the larger performance implications of using the Semaphore. If the Sync need proves to be more common then just my case, then you might consider including Sync wrapper types here?

Feel free to close this if you think there is nothing more actionable. Or keep it open so that others can find it?

[Matthias247]

If the Sync need proves to be more common then just my case, then you might consider including Sync wrapper types here?

Sure!

Feel free to close this if you think there is nothing more actionable. Or keep it open so that others can find it?

Thanks! I will close it. I think for visibility reasons there is not really an ideal time to close things. So one can do it as well immediately, in order to avoid the backlog from growing.

[dekellum]

For those interested, see relevant followup in: http://gravitext.com/2020/01/13/blocking-permit.html