Why adding `+ Sync` bound asks for `Clone`?

[Geobomatic]

I'm working on async support in oxide-auth and the maintainer chose to put #[async_trait(?Send)] on the async version of essentials traits.

When trying to port my app to these async traits, I was bite by "cannot be sent between threads safely" and it is expected.

So I'm in the process of removing ?Send from these traits, which of course leads to a wall of errors (which induce another kind of fear with concurrency…).

I don't have a small example code but I've pushed what I've done here and commented with the current state:
https://github.com/Geobert/oxide-auth/pull/1

I know it's a big chunk to digest but I'm really stuck

Thanks for any insight

[kornel]

You can make a function:

fn is_send<T: Send>(x: T) {x}

and wrap expressions in it:

let hi = is_send("hello");

It won't compile if the expression isn't Send, so you can use divide and conquer to find the culprit.

[Geobomatic]

Thank you for the advice!

I think I understand the error better but still can't find a fix for it. The body field is the issue here:

struct WrappedRequest<'a, R: WebRequest + Send + Sync + 'a> {
    /// Original request.
    request: PhantomData<R>,

    /// The query in the url.
    body: Cow<'a, dyn QueryParameter + 'static>,

    /// The authorization tuple
    authorization: Option<Authorization>,

    /// An error if one occurred.
    error: Option<FailParse<R::Error>>,

    /// The credentials-in-body flag from the flow.
    allow_credentials_in_body: bool,
}

As it's a trait object, it can't says if it's Sync or not.

What's puzzling me, it that adding a bound + Sync to it trigger this error:

error[E0277]: the trait bound `(dyn oxide_auth::endpoint::query::QueryParameter + std::marker::Sync + 'static): std::clone::Clone` is not satisfied
  --> oxide-auth-async\src\endpoint\access_token.rs:47:5
   |
47 |     body: Cow<'a, dyn QueryParameter + Sync + 'static>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::clone::Clone` is not implemented for 
`(dyn oxide_auth::endpoint::query::QueryParameter + std::marker::Sync + 'static)`
   |
   = note: required because of the requirements on the impl of `std::borrow::ToOwned` for `(dyn oxide_auth::endpoint::query::QueryParameter + std::marker::Sync + 'static)`
   = note: required because it appears within the type `std::borrow::Cow<'a, (dyn oxide_auth::endpoint::query::QueryParameter + std::marker::Sync + 'static)>`
   = note: only the last field of a struct may have a dynamically sized type

And I can't add + Clone as it's not an auto trait. This is where I'm stuck for 2 days now

[Geobomatic]

I've edited the post title to reflect the current issue

[bjorn3]

The trait object inside Cow is missing the ToOwned bound, which is required by Cow.

[Geobomatic]

Thank you for your answer

We can't add ToOwned though:

error[E0225]: only auto traits can be used as additional traits in a trait object
  --> oxide-auth-async\src\endpoint\access_token.rs:47:40
   |
47 |     body: Cow<'a, dyn QueryParameter + ToOwned + 'static>,
   |                       --------------   ^^^^^^^
   |                       |                |
   |                       |                additional non-auto trait
   |                       |                trait alias used in trait object type (additional use)
   |                       first non-auto trait
   |                       trait alias used in trait object type (first use)

[kornel]

I don't think Cow can even exist with a dyn object of unknown size. These are not cloneable, and Cow is a Clone-on-Write type.

Use a generic parameter, or don't bother with the whole Cow and temporarily-borrowed objects which will be a massive pain to send to another thread (probably impossible in practice, as I haven't seen a scoped async executor).

You can use Arc<dyn> to have cheaply cloneable sendable dynamic type.

[Geobomatic]

@kornel

I don't think Cow can even exist with a dyn object of unknown size.

That's what's causing me headache atm: how come it did compile without + Sync in the first place with this declaration?

I'll try your Arc suggestion, thank you!

[kornel]

Is QueryParameter a type or a trait? If it's a type, then it compiled becuse it was a fixed-size object with a known type. By adding dyn you've made it unknown-sized object of unknown type (plus a type error, because dyn is for traits only).

[Geobomatic]

pub unsafe trait QueryParameter

And I precise that the dyn was there for a long time and it compiled just fine. By replacing #[async_trait(?Send)] (which made the whole async code unusable for multi-thread) by #[async_trait] many missing bounds appeared logically. That's when all this mess began

[kornel]

Well, then I don't know how it could have worked previously

[trentj]

Perhaps you have impl ToOwned for dyn QueryParameter (possibly + 'static) somewhere? That would make the original work. The problem being that dyn QueryParameter and dyn QueryParameter + Sync are not interchangeable, and dyn QueryParameter + Sync does not implement ToOwned.

Maybe you could implement ToOwned for all combinations of +Send and +Sync?

[Geobomatic]

Of course… you're totally right, that's it!

impl ToOwned for dyn QueryParameter {
    type Owned = NormalizedParameter;

    fn to_owned(&self) -> Self::Owned {
        self.normalize()
    }
}