Future type returned by trait methods should impl Sync

[kevinmehall]

The methods generated by this library return Pin<Box<dyn Future + Send + 'async>> but leave out Sync.

As discussed on this internals thread, every type that may be used in async-await should be Sync. It may seem meaningless for a Future to impl Sync since its only method (poll) takes &mut self so &Future is useless, but the problem is that as an auto trait, the !Sync propagates to anything that owns the Future.

As a concrete example, I ran into this while trying to make a Hyper server that concatenates multiple objects from Amazon S3 into an HTTP response. rusoto_s3 uses async-trait to define its S3 trait. S3::get_object returns Pin<Box<dyn Future + Send>>, without Sync. When combining these futures into a stream, the resulting Stream therefore does not impl Sync. However, hyper::Body::wrap_stream has a Sync bound on its argument because the stream it is passed ends up wrapped inside a Request struct that needs to be Sync so that it can be borrowed across an await.

[problame]

I am working on this

[Matthias247]

As discussed in the linked topic, the Sync requirement is mainly a Hyper issue - which will certainly be fixed at one point in time. In general Futures don't need to be Sync, since they are only consumed by one certain caller.

rusoto_s3 uses async-trait to define its S3 trait. S3::get_object returns Pin<Box<dyn Future + Send>>, without Sync. When combining these futures into a stream, the resulting Stream therefore does not impl Sync.

And that's totally fine! Trying to make everything Sync will just get us into a painful situation where we need to add mutex on every layer, make things slow, and barely possible to implement them by hand.

However, hyper::Body::wrap_stream has a Sync bound on its argument because the stream it is passed ends up wrapped inside a Request struct that needs to be Sync so that it can be borrowed across an await.

Yes, that one is the issue

[problame]

However, hyper::Body::wrap_stream has a Sync bound on its argument because the stream it is passed ends up wrapped inside a Request struct that needs to be Sync so that it can be borrowed across an await.

That's exactly the case we ran into, and the reason why I opened #96 . I agree it's not pretty, but I don't see another solution ATM.

[Matthias247]

I think you can wrap the Stream with one that provides synchronization through a Mutex. That's ugly - but since the Mutex is non-contended it won't hurt the performance too much. If you feel adventurous and really really need the performance you can also wrap it in a type which fakes Sync (unsafe impl Sync for Wrapper) and have an unsafe method to create it.

Here is another discussion around the same topic: https://github.com/Matthias247/futures-intrusive/issues/23

[kevinmehall]

To link together more related discussions, there is also a proposal stemming from that same internals thread to define a type that behaves like a compile-time-checked mutex to make things Sync without allocation or overhead: https://github.com/rust-lang/rust/pull/71529, and discussion about using it to remove the Sync bound in Hyper here: https://github.com/hyperium/hyper/pull/2187

Trying to make everything Sync will just get us into a painful situation where we need to add mutex on every layer, make things slow, and barely possible to implement them by hand.

If the convention were for all Future / Stream implementations to be Sync, very few of them would need a Mutex or StaticMutex. These traits' methods take &mut, so they have no reason to use a Cell, and code using raw pointers can safely declare their types Sync for the same reason. The !Sync mostly comes from omitting the + Sync when creating trait objects.

For the sake of interoperability, it seems like either (A) all Future/Stream implementers should be Sync, or (B) any API that accepts these types should not require Sync, and the ecosystem needs to agree on one of those. At the time I opened this issue, the conclusion from that thread seemed to be to make everything Sync + Send. You and others have since argued against that on the grounds of Sync being unnecessary.

The part I didn't consider is that this change is not progress unless everyone agrees with solution (A). While Rusoto exposes its public API via async-trait and implements those traits for types it defines itself, traits in general impose their return type bounds on methods in trait implementations. So we have to pick one of solutions (A) or (B) above when it comes to traits; we can't try for both.

I'm not sure which solution is ideal, but I hope the ecosystem can converge on one. Library users should not have to think about Sync when it doesn't actually mean anything for Future/Stream and implement their own mutex wrappers in order to make libraries work together. Feel free to close this for now, as this is a broader issue than this one library and not necessarily the right place for this discussion.

[jocutajar]

Yes please. If we can do it with classic traits, let's do it with async ones too :)

Here's a simple example:

use async_trait; // 0.1.36

fn test_ok<T:Classic>(tested:T) {
    is_sync(tested.sync_ok());
}
fn test_nok<T:Modern>(tested:T) {
    is_sync(tested.sync_nok());
}

fn is_sync<T: Sync>(_tested: T) {}

#[async_trait::async_trait]
trait Modern {
    type Result;
    async fn sync_nok(&mut self) -> Self::Result;
}

trait Classic {
    type Result;
    fn sync_ok<'s, 'f>(
        &'s mut self,
    ) -> Box<dyn std::future::Future<Output = Self::Result> + 'f + Send + Sync>
    where
        's: 'f;
}

[jocutajar]

Similarly, I may desire that the trait produces a future with static lifetime. Perhaps an additional attribute over the async fn could be used to optionally specify extra traits. Something similar to pin_project's #[pin]:

#[async_trait::async_trait]
trait Modern {
    type Result;
    #[future_is['static + Sync]]
    async fn sync_nok(&mut self) -> Self::Result;
}

trait Classic {
    type Result;

    fn sync_static(
        &mut self,
    ) -> Box<dyn std::future::Future<Output = Self::Result> + 'static + Send + Sync>;
}

[dignifiedquire]

Any update here? This is quite painful to work around atm

[dtolnay]

I would prefer not to change this in this crate. But it would be reasonable for somebody else to maintain a more fully featured macro for async traits that handles this use case.

[jocutajar]

It's coming folks :) https://blog.rust-lang.org/inside-rust/2022/11/17/async-fn-in-trait-nightly.html#when-it-does-happen just where to grab some patience :)