feat(body): remove Sync bound for Body::wrap_stream

[rkuhn]

A stream wrapped into a Body previously needed to implement Sync so
that the Body type implements this autotrait as well (which is needed
due to limitations in async/await). Since a stream only offers one
method that is called with an exclusive reference, this type is
statically proven to be Sync already. In theory it should be fine to add
an unsafe impl Sync, but this commit instead adds a Mutex that is
never locked to prove that this approach is correct.

This makes it easier to construct response bodies for client code.

For more discussion on this see rust-internals.

[rkuhn]

@seanmonstar This is the follow-up to #2159, WDYT?

[seanmonstar]

This could be useful for users, definitely! Mutex does add overhead of an additional allocation (std::sync::Mutex boxes internally due to pthread_mutex_t requiring a stable address), and the poison stuff...

[rkuhn]

Well, now that it is proven, the alternative is to just unsafe Impl Sync (which could break later) or use the SyncWrapper from that rust-internals thread (that should compile to nothing). Which one would you prefer?

[rkuhn]

I decided to take a shot at the SyncWrapper: rust-lang/rust#71529

[seanmonstar]

We could have a SyncWrapper internal to hyper here. Along with comments reminding us what it's sane/safe.

[rkuhn]

@seanmonstar following some discussion on zulip I published sync_wrapper and updated this PR — is this okay?

[rkuhn]

Hmm, the test failure is not related to this change AFAICS, and I also cannot reproduce it when running the same command locally …

[seanmonstar]

Could we inline the type into the module? It seems small enough that we don't need to add the cost of downloading/compiling another dependency...

[rkuhn]

We sure could do that, but if we kept the docs then the increase in size of hyper wouldn’t be much less than the 6kB taken by sync_wrapper (compilation time basically not measurable — 150ms by itself for release). I’ll definitely use SyncWrapper for other purposes as well; let me know what you think.

[seanmonstar]

The type seems small enough to me to inline. There's costs to having a separate crate, some real (new network calls to download metadata and source, having to link libraries), and perceived (users get grumpy as the dependency count gets bigger).

[rkuhn]

Okay, will probably do it tomorrow.

[rkuhn]

I pushed the version which has the SyncWrapper in common.

[seanmonstar]

Wonderful!

[lnicola]

Note that this was a breaking change (see the PR linked above).

[seanmonstar]

How is this a breaking change? We've made the bounds less strict, so everything that matched before should match still.

[seanmonstar]

Sure, that would be a breaking change, but that's not what we did. We changed fn wrap_stream<T: Blah + Send + Sync>() - >Body to fn wrap_stream<T: Blah + Send> -> Body. The Body type is still Sync. Nothing should have broken.

[lnicola]

Right, I had to take another look. The issue is that this impl changed: https://github.com/hyperium/hyper/pull/2187/files#diff-b032ea26c55c1f4f1c2c32cd030e0bc0L405.

[rkuhn]

Hmm, the From instance now applies to more inputs, how can that be a problem?

[rkuhn]

Or asked differently: can you show some compiler error demonstrating what you mean by “breaking change”? That would make it easier for me to grasp.

[lnicola]

error[E0277]: the trait bound `hyper::Body: std::convert::From<std::boxed::Box<(dyn futures::Stream<Item = std::result::Result<_, _>> + std::marker::Send + std::marker::Sync + 'static)>>` is not satisfied
   --> src/main.rs:528:21
    |
528 |                     http_serve::serve(crf, &request)
    |                     ^^^^^^^^^^^^^^^^^ the trait `std::convert::From<std::boxed::Box<(dyn futures::Stream<Item = std::result::Result<_, _>> + std::marker::Send + std::marker::Sync + 'static)>>` is not implemented for `hyper::Body`
    | 
   ::: /home/me/.cargo/registry/src/github.com-1ecc6299db9ec823/http-serve-0.2.2/src/serving.rs:80:15
    |
80  |     B: Body + From<Box<dyn Stream<Item = Result<Ent::Data, Ent::Error>> + Send + Sync>>,
    |               ------------------------------------------------------------------------- required by this bound in `http_serve::serve`
    |
    = help: the following implementations were found:
              <hyper::Body as std::convert::From<&'static [u8]>>
              <hyper::Body as std::convert::From<&'static str>>
              <hyper::Body as std::convert::From<bytes::Bytes>>
              <hyper::Body as std::convert::From<std::borrow::Cow<'static, [u8]>>>
            and 4 others

[rkuhn]

Oh, this is a limitation of the Rust type system that I wasn’t aware of: the actually implemented From instance covers the needs of the one you ask for, so it should be fine, but rustc doesn’t seem to understand that.

[rkuhn]

Out of interest: why is this second part of the bound present? Shouldn’t B: Body allow you to do everything needed?

[lnicola]

I'm not sure, but it doesn't compile without them 😅. That crate is rather generics-heavy, and it's not impossible that some bounds are unnecessary.