Semaphore::acquire returned Future lost its Sync

[dekellum]

The Future returned by tokio::sync::Semaphore::acquire was Sync prior to 0.2.14, but now isn't. This is a breaking change (it breaks my code) in 0.2.14 and 0.2.15.

For motivation as to why I've relied on Sync for this type:

I realize that Send is commonly sufficient for a Future. I encountered the Sync bounds because I've implemented 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).

Version

Since 0.2.14, including 0.2.15. Not an issue with 0.2.13 or earlier.

└── tokio v0.2.14

Platform

Linux klein 5.5.10-arch1-1 #1 SMP PREEMPT Wed, 18 Mar 2020 08:40:35 +0000 x86_64 GNU/Linux
rustc 1.43.0-nightly (564758c4c 2020-03-08)

Description

`std::cell::UnsafeCell<std::option::Option<std::task::Waker>>` cannot be shared between threads safely
   | 
  ::: /home/david/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.14/src/sync/semaphore.rs:65:36
   |
65 |     pub async fn acquire(&self) -> SemaphorePermit<'_> {
   |                                    ------------------- within this `impl std::future::Future`
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Sync` is not implemented for `std::cell::UnsafeCell<std::option::Option<std::task::Waker>>`
   = note: required because it appears within the type `tokio::loom::std::unsafe_cell::UnsafeCell<std::option::Option<std::task::Waker>>`
   = note: required because it appears within the type `tokio::sync::batch_semaphore::Waiter`
   = note: required because it appears within the type `tokio::sync::batch_semaphore::Acquire<'_>`
   = note: required because it appears within the type `for<'r, 's, 't0, 't1> {&'r tokio::sync::semaphore::Semaphore, tokio::sync::semaphore::Semaphore, &'s tokio::sync::batch_semaphore::Semaphore, tokio::sync::batch_semaphore::Semaphore, u16, tokio::sync::batch_semaphore::Acquire<'t0>, tokio::coop::CoopFuture<tokio::sync::batch_semaphore::Acquire<'t1>>, ()}`
   = note: required because it appears within the type `[static generator@DefId(15:3288 ~ tokio[2b19]::sync[0]::semaphore[0]::{{impl}}[0]::acquire[0]::{{closure}}[0]) 0:&tokio::sync::semaphore::Semaphore for<'r, 's, 't0, 't1> {&'r tokio::sync::semaphore::Semaphore, tokio::sync::semaphore::Semaphore, &'s tokio::sync::batch_semaphore::Semaphore, tokio::sync::batch_semaphore::Semaphore, u16, tokio::sync::batch_semaphore::Acquire<'t0>, tokio::coop::CoopFuture<tokio::sync::batch_semaphore::Acquire<'t1>>, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@DefId(15:3288 ~ tokio[2b19]::sync[0]::semaphore[0]::{{impl}}[0]::acquire[0]::{{closure}}[0]) 0:&tokio::sync::semaphore::Semaphore for<'r, 's, 't0, 't1> {&'r tokio::sync::semaphore::Semaphore, tokio::sync::semaphore::Semaphore, &'s tokio::sync::batch_semaphore::Semaphore, tokio::sync::batch_semaphore::Semaphore, u16, tokio::sync::batch_semaphore::Acquire<'t0>, tokio::coop::CoopFuture<tokio::sync::batch_semaphore::Acquire<'t1>>, ()}]>`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required for the cast to the object type `dyn std::future::Future<Output = tokio::sync::semaphore::SemaphorePermit<'_>> + std::marker::Send + std::marker::Sync`

[hawkw]

Yup, that's definitely a problem.

I think there needs to be an explicit impl of Sync for the batch_semaphore::Acquire future, like there is for Notified:

tokio/tokio/src/sync/notify.rs

Lines 135 to 136 in 6fa40b6

	unsafe impl<'a> Send for Notified<'a> {}
	unsafe impl<'a> Sync for Notified<'a> {}

We should also add compile tests asserting Send + Sync to prevent this from happening in the future. Thanks for the report!