Hyper: Don't use private traits in public APIs

Created on 6 Dec 2019 · 49Comments · Source: hyperium/hyper

For example, hyper master currently has this:

impl<I, B, S, E> Future for Connection<I, S, E>
where
    S: HttpService<Body, ResBody=B>,
    S::Error: Into<Box<dyn StdError + Send + Sync>>,
    I: AsyncRead + AsyncWrite + Unpin + 'static,
    B: Payload + 'static,
    B::Data: Unpin,
    E: H2Exec<S::Future, B>,

HttpService and H2Exec are both private traits, though. This makes it a huge pain for code working generically with connections to express the proper bounds in their code. You basically have to try to reverse engineer the impl bounds by bouncing off the compiler :(

B-rfc

Source

sfackler

👍3

Most helpful comment

As I've done similar spelunking to remain generic over Client and therefore Connect and Payload, I would also greatly appreciate fully public signatures, even if more immediately complex as in @sfackler's last.

That you are considering the now visible Unpin bounds seems to support the value just for hyper API maintenance.

dekellum on 6 Dec 2019

👍4

All 49 comments

The H2Exec bound in particular is a bit of a nightmare:

error[E0599]: no method named `graceful_shutdown` found for type `std::pin::Pin<&mut hyper::server::conn::Connection<I, server::idle::TrackedService<S>>>` in the current scope
   --> sorcery/src/server/idle.rs:163:19
    |
163 |         this.conn.graceful_shutdown();
    |                   ^^^^^^^^^^^^^^^^^ method not found in `std::pin::Pin<&mut hyper::server::conn::Connection<I, server::idle::TrackedService<S>>>`
    |
    = note: the method `graceful_shutdown` exists but the following trait bounds were not satisfied:
            `hyper::common::exec::Exec : hyper::common::exec::H2Exec<server::idle::TrackedFuture<<S as tower_service::Service<http::request::Request<hyper::body::body::Body>>>::Future>, _>`

sfackler on 6 Dec 2019

Same for the Connect trait. Building a custom connector doesn't seem to be possible ATM due to that trait now being private.

jedisct1 on 6 Dec 2019

It is possible - you just have to source dive and find the right blanket implementation you need to satisfy: https://github.com/hyperium/hyper/blob/master/src/client/connect/mod.rs#L203-L208

sfackler on 6 Dec 2019

HttpService should just be a trait alias so you should be able to do the same if you implement these bounds https://github.com/hyperium/hyper/blob/0dc89680cd386e17e581efb8608c07774eeee67d/src/service/mod.rs#L12

but yeah we could improve documentation...

LucioFranco on 6 Dec 2019

So, hyper uses these "private trait aliases" to try to make the where bounds more succinct. For instance, the HttpService trait is implemented for Services with the right types.

HttpService:

where
  S: HttpService<Body, ResBody=B>,

Service:

where
  S: Service<http::Request<Body>, Response = http::Response<B>>,
  S::Error: Into<Box<dyn std::error::Error + Send + Sync>>,

I assumed that seeing less "noise" would be helpful, and it also means less duplication of bounds inside the hyper codebase. If it really seems to make things worse, we can remove these "aliases" and just make the bounds lists bigger.

seanmonstar on 6 Dec 2019

👍1

It's fine to use the private traits bounds internally in hyper - the parts that matter to me are just the public interfaces, where it is important to be able to see what is in fact required to call the method.

sfackler on 6 Dec 2019

👍1

With the H2Exec and NewSvcExec traits, they could be "unwrapped" (it'd be quite messy), but they'd still show otherwise private types. Like E: H2Exec<<S::Service as Service<Body>>::Future, B> would be E: Executor<crate::proto::h2::server::H2Stream<<S::Service as Service<Body>>::Future, B>>.

The propagation of the futures for the executor stuff is just horrible, but I don't know a better way while still allowing !Send servers.

seanmonstar on 6 Dec 2019

You'd need to further unwrap them until they don't show private types. Anyone writing code that deals with hyper types in a generic context has to figure this stuff out anyway!

sfackler on 6 Dec 2019

👍1

This is what I arrived at after ~1 hour of source diving into hyper and bouncing off of compiler errors as the trait bounds for Connection's Future implementation:

impl<I, S, B> Future for Connection<I, S>
where
    S: Service<Request<Body>, Response = Response<B>>,
    S::Future: 'static + Send,
    S::Error: Into<Box<dyn Error + Sync + Send>>,
    I: AsyncRead + AsyncWrite + Unpin + 'static,
    B: http_body::Body + 'static + Send + Unpin,
    B::Data: Send + Unpin,
    B::Error: Into<Box<dyn Error + Sync + Send>>,

sfackler on 6 Dec 2019

👍1

Those Unpins on B and B::Data come through like 8 layers out of H2Exec.

sfackler on 6 Dec 2019

Oh, we can probably remove those Unpin needs actually!

seanmonstar on 6 Dec 2019

That you are considering the now visible Unpin bounds seems to support the value just for hyper API maintenance.

dekellum on 6 Dec 2019

👍4

Can we expose the traits publically but seal them so that they still need to be implemented via the real trait but can be used to make bounds match. I currently do this in tonic which isn't so bad but could be useful to get he httpservice trait out https://github.com/hyperium/tonic/blob/master/tonic/src/transport/server.rs#L215

The other idea is that maybe some of these more generic traits like HttpService could live outside of hyper?

LucioFranco on 6 Dec 2019

Can we expose the traits publically but seal them so that they still need to be implemented via the real trait but can be used to make bounds match.

That's how the current hyper release works with e.g. Payload: https://docs.rs/hyper/0.13.0-alpha.4/hyper/body/trait.Payload.html. It's still a bit weird though, since it can seem like it's impossible to create custom payloads until you look through the docs and realize you have to implement some other trait instead of Payload.

sfackler on 6 Dec 2019

Sounds like this could be an improved compiler diagnostics thing. Where you have a sealed trait it should tell you the possible trait you need to implement but I don't think we have that.

LucioFranco on 6 Dec 2019

@dekellum the issue with large trait bounds is that they just genearlly look scary and harder to use. I think this problem is more of a documentation issue and edcuation issue over ergonomics or anything else.

LucioFranco on 6 Dec 2019

Why are these traits sealed in the first place? Why does hyper care if someone implements Connect via Service or directly?

sfackler on 6 Dec 2019

I think the concern is to build an ecosystem of reusable parts. If we have users implement connectors via the connect trait and some via the service trait then then we create a split. If we use trait aliases then every implementation has to derived from tower_service::Service which allows a lot of flexibility and composability. At least that is how I see it. The HttpService idea comes from tower_http and connect comes from tower_make::MakeConnection.

LucioFranco on 6 Dec 2019

IMO _private_ types in a public signature are even more "scary". At best they seem like an oversight or unfinished work.

Could there be some separate public type aliases, with tests confirming compatibility with the internal, non-public types (which may span multiple crates)? Just rustdoc text is unlikely to improve confidence, particularly if it becomes out of date.

dekellum on 6 Dec 2019

👍2

IMO private types in a public signature are even more "scary". At best they seem like an oversight or unfinished work.

I strongly agree with this.

I was attempting to update https://github.com/ctz/hyper-rustls to the latest Hyper release on the master branch and the sealed types in trait bounds are _not_ fun to work with or reconstruct. A big portion of this were the unimplementable trait aliases in public signatures. If Hyper is to continue exposing these trait aliases, I'd _greatly_ prefer that they be unsealed despite them being a potential semver hazard.

If we have users implement connectors via the connect trait and some via the service trait then then we create a split.

Perhaps I don't fully understand, but if there is a blanket implementation of Connect for every tower_service::Service with the same generic bounds, there shouldn't be any incompatibilities resulting from some users choosing to implement Connect and others implementing tower_service::Service, correct?

davidbarsky on 7 Dec 2019

👍2

@dekellum what bounds did you use to be generic over Client? I'm hitting this pain point too.

hwchen on 10 Dec 2019

As of the last alpha it was like this:

pub fn request_dialog<CN, B>(
    client: &hyper::Client<CN, B>,
    rr: RequestRecord<B>,
    tune: &Tunables)
    -> impl Future<Output=Result<Dialog, FutioError>> + Send + 'static
    where CN: hyper::client::connect::Connect + Sync + 'static,
          B: hyper::body::Payload + Send + Unpin,
          B::Data: Unpin

But it appears there are changes on master and the latest release. I will update when I'm able to upgrade.

dekellum on 10 Dec 2019

@dekellum just wondering, does it need to be specifically &hyper::Client? Or could it be impl Service?

seanmonstar on 10 Dec 2019

It calls client.request() internally. Is that also supported on Service? If that was an option with a prior alpha, I missed it. If its an option now, I'll give it a try and report back. Any effective difference or advantage with that?

dekellum on 10 Dec 2019

when using impl Service for a client param, I get:

error[E0599]: no method named `request` found for type `impl hyper::service::Service` in the current scope
  --> src/reddit.rs:39:22
   |
39 |     let res = client.request(req).await?;
   |                      ^^^^^^^ method not found in `impl hyper::service::Service`

Although, I haven't finished going through all the compiler errors yet.

hwchen on 10 Dec 2019

It's slightly more involved to use a Service (gotta check Service::poll_ready and then you can Service::call(req)), but could maybe be more convenient of an API. People could wrap their clients in middleware, and testing is just passing some mocked service. But you don't need to switch if you don't want to.

seanmonstar on 10 Dec 2019

Edit: I see now that for my use case, just using hyper-tls is sufficient: https://docs.rs/hyper-tls/0.4.0/hyper_tls/struct.HttpsConnector.html . I’m not sure, but I feel like I had gone the generic route because of some example I found in the past. I still generally feel the pain of sealed traits though.

so, I'm pretty stuck at this point.

If I use impl Service as a type in the fn parameters, I get an error like:

error[E0277]: the trait bound `&hyper::client::Client<hyper_tls::client::HttpsConnector<hyper::client::connect::http::HttpConnector>>: tower_service::Service<http::request::Request<hyper::body::body::Body>>` is not satisfied
  --> src/main.rs:31:21
   |
31 |     let new_posts = fetch_reddit_new(&client, subreddit, n).await?;
   |                     ^^^^^^^^^^^^^^^^ the trait `tower_service::Service<http::request::Request<hyper::body::body::Body>>` is not implemented for `&hyper::client::Client<hyper_tls::client::HttpsConnector<hyper::client::connect::http::HttpConnector>>`
   | 
  ::: src/reddit.rs:15:14
   |
15 | pub async fn fetch_reddit_new(
   |              ----------------
16 |     client: impl Service<Request<hyper::Body>>,
   |                  ----------------------------- required by this bound in `reddit::fetch_reddit_new`
   |
   = help: the following implementations were found:
             <hyper::client::Client<C, B> as tower_service::Service<http::request::Request<B>>>

If I use Client as a parameter's type, I have a bunch of bounds I need to add, but get stuck on needing Connect, which is sealed.

Since it was pretty straightforward to have a Client generic over Connect in the past, it's a little surprising that it's not possible now (or requires some indirection I don't know yet).

Really looking forward to upgrading to hyper 0.13, so help figuring this out is definitely appreciated.

hwchen on 11 Dec 2019

I ended up with this for a struct that allows hyper Clients with different connectors:

pub struct Requester<S>
where
    S: Service<Uri> + Clone + Send + Sync + 'static,
    S::Response: Connection + AsyncRead + AsyncWrite + Send + Unpin + 'static,
    S::Future: Send + Unpin + 'static,
    S::Error: Into<Box<dyn std::error::Error + Send + Sync>>,
{
    client: hyper::Client<S, Body>,
}

jbg on 11 Dec 2019

👍3

I ended up forking hyper to unseal Connect because this is really too painful.

jedisct1 on 11 Dec 2019

Thanks for the pointer @jbg, as it seems my upgrade to the last alpha (signature above) is drastically incomplete for the hyper 0.13 release. I additonally need to be generic over B: HttpBody, so thus far I have this:

use hyper::service::Service;
use hyper::Uri;
use tokio::io::{AsyncRead, AsyncWrite};
use hyper::client::connect::Connection;

pub fn request_dialog<CN, B>(
    client: &hyper::Client<CN, B>,
    rr: RequestRecord<B>,
    tune: &Tunables)
    -> impl Future<Output=Result<Dialog, FutioError>> + Send + 'static
    where CN: Service<Uri> + Clone + Send + Sync + 'static,
          CN::Response: Connection + AsyncRead + AsyncWrite + Send + Unpin + 'static,
          CN::Future: Send + Unpin + 'static,
          CN::Error: Into<Box<dyn std::error::Error + Send + Sync + 'static>>,
          B: hyper::body::HttpBody + Send + Unpin + 'static,
          B::Data: Send + Unpin,
          B::Error: Into<Box<dyn std::error::Error + Send + Sync + 'static>>

Lots of Send, Sync, and Unpin in there. Also I thought hyper was trying to be agnostic to executor? But notice that AsyncRead and AsyncWrite traits (which I otherwise don't care much about) are tokio's.

dekellum on 11 Dec 2019

😄1

@dekellum technically, asyncread + asyncwrite are agnostic to an executor, they are more connected to a reactor. You can easily shim them over to futures-io traits as well.

As for the Send + Sync bounds, you need to add them, this was a big reason why I am a proponet for trait aliases, as they allow applications to build ontop of the core traits and create easier trait bounds while creating easier error messages. They also make things like this much less scarier. Most types you pass in will most 95% of the bounds but we just need to be explicit.

LucioFranco on 11 Dec 2019

I only mentioned Send because:

I don't know a better way while still allowing !Send servers.

Maybe client can also work if I duplicate these methods using a !Send-friendly executor?

@seanmonstar bot: remove Unpin please? 🙂

dekellum on 11 Dec 2019

remove Unpin please?

We should! Filed https://github.com/hyperium/hyper/issues/2070

I ended up forking hyper to unseal Connect because this is really too painful.

Perhaps the Connect "trait alias" should be made public again, so people can more easily set the bounds in their own libraries?

seanmonstar on 11 Dec 2019

👍2

As for using a Service, here's what some sample code using @hwchen's comment could look like:

async fn fetch_reddit_new<S, ReqBody, ResBody>(
    client: &mut S,
    subreddit: &str,
    posts: usize,
) -> Result<Vec<PostData>, YourError>
where
    S: Service<
        http::Request<ReqBody>,
        Response = http::Response<ResBody>,
    >,
    ReqBody: Default,
    ResBody: HttpBody,
    YourError: From<S::Error>,
    YourError: From<ResBody::Error>,
{
    let url = /* ... */;
    let req = Request::get(url).body(ReqBody::default())?;

    let res = client.call(req).await?;

    let bytes = hyper::body::to_bytes(res.into_body()).await?;

    // ...
}

We could reduce some of that by exposing the HttpService "trait alias" that exists in hyper...

seanmonstar on 11 Dec 2019

I just wanted to say +1 to making the Connect trait alias publicly visible again. Making the trait visible (even while still not allowing others to implement it) seems better in all ways than the current form. First it makes it possible to write generic bounds against it, second it can show up in docs which will also highlight the blanket impl for tower::Service along with an opportunity to write a comment that tower::Service is what should be implemented. Right now the only way to know you need to implement tower::Service is to read the source code.

An issue when moving from a hyper::Client to a generic tower::Service is that the ownership semantics between hyper and tower are not the same. Hyper has internal synchronization so that you can invoke methods on shared references. A tower service requires a mutable reference to invoke call. This means that I now need to implement my own synchronization around the tower service, even though I really just want to support hyper::Clients that already provide their own. My code would end up being much more complicated and would be doing double synchronization for the trouble.

ggriffiniii on 12 Dec 2019

👍2

OK, so there is https://github.com/hyperium/hyper/pull/2073 up regarding the Connect trait.

When exposing this, I remembered why it was hidden: I hope to be able to remove some of the required bounds on some of the associated types, but if the trait is public, you could use it to assume the same bounds (and thus when hyper removed the need for them, your code could break). So this PR exposes the Connect trait that is implemented for all Services with the right bounds, but you otherwise can't access any associated types and assume their bounds.

So, again using a previous example, the code can now be:

pub struct Requester<C>
where
    C: Connect + Clone + Send + Sync + 'static,
{
    client: hyper::Client<S, Body>,
}

seanmonstar on 12 Dec 2019

Thanks, with master (#2070) and #2073 my bounds reduce to the following, and I can avoid some manual/unsafe Unpin impl or pin projection.

pub fn request_dialog<CN, B>(
    client: &hyper::Client<CN, B>,
    rr: RequestRecord<B>,
    tune: &Tunables)
    -> impl Future<Output=Result<Dialog, FutioError>> + Send + 'static
    where CN: hyper::client::connect::Connect + Clone + Send + Sync + 'static,
          B: hyper::body::HttpBody + Send + 'static,
          B::Data: Send,
          B::Error: Into<Flaw>

Where Flaw is my own type alias for Box<dyn std::error::Error + Send + Sync + 'static>.

dekellum on 12 Dec 2019

Is really the only difference here not using the actual Service trait with the Urigeneric? Seems almost the same as before?

LucioFranco on 12 Dec 2019

As opposed to my prior signature (an object of humor!) its 4 instead of 7 (lines of) bounds and all Unpin removed. I haven't tried replacing Client with Service yet.

dekellum on 12 Dec 2019

As for using a Service, here's what some sample code using @hwchen's comment could look like:

async fn fetch_reddit_new<S, ReqBody, ResBody>(
    client: &mut S,
    subreddit: &str,
    posts: usize,
) -> Result<Vec<PostData>, YourError>
where
    S: Service<
        http::Request<ReqBody>,
        Response = http::Response<ResBody>,
    >,
    ReqBody: Default,
    ResBody: HttpBody,
    YourError: From<S::Error>,
    YourError: From<ResBody::Error>,
{
    let url = /* ... */;
    let req = Request::get(url).body(ReqBody::default())?;

    let res = client.call(req).await?;

    let bytes = hyper::body::to_bytes(res.into_body()).await?;

    // ...
}

We could reduce some of that by exposing the HttpService "trait alias" that exists in hyper...

I've migrated my code to the new Service trait.
The problem with the call methos, is that it requires a mutable reference, therefore you can't make parallel calls on the same Client, therefore I have to clone the same Client for every parallel call, only to be able to accept Http or Https clients seamlessy.
I don't think it's what we want to reach with Hyper.
When the new version with the again public Connect trait will be published?

nappa85 on 13 Dec 2019

The problem with the call methos, is that it requires a mutable reference, therefore you can't make parallel calls on the same Client, therefore I have to clone the same Client for every parallel call, only to be able to accept Http or Https clients seamlessy.

This is normal, and how tower was designed to work with its poll_ready. I recommend reading the docs around tower's contract. Cloning in this case is cheap and if you want to have concurrent requests just include a impl Service + Clone.

LucioFranco on 13 Dec 2019

Though this brings up an interesting thought, since hyper::Client allows making requests with a shared reference, maybe hyper should impl Service for &'_ Client as well...

seanmonstar on 13 Dec 2019

👍3

FYI, v0.13.1 is just released that exposes hyper::client::connect::Connect.

seanmonstar on 16 Dec 2019

🎉3

I am trying to port some code to [email protected] which does not expose MakeConnection anymore. Why is that, and is there a workaround?

dignifiedquire on 3 Mar 2020

@dignifiedquire there is a trait alias bound that you can implement that version of tower::Service https://docs.rs/hyper/0.13.2/hyper/client/connect/trait.Connect.html

basically where impl Service<Uri, Response = impl AsyncRead + AsyncWrite>

LucioFranco on 3 Mar 2020

👍1

I'm trying to write a function that returns a Server<AddrIncoming, MakeServiceFn<F>> but MakeServiceFn is private, so it cannot be named. Could MakeServiceFn be made public?

Edit: Maybe the solution is to instead return impl Future<Output = ...>?

TheDan64 on 19 Jun 2020

Honestly after trying to mess around with the API for a bit, I can safely say that the lack of public types makes things a nightmare to deal with. Like, with all due respect, trying to fenangle the types for MakeServiceFn and co. was the most miserable experience I've ever had trying to work with a Rust library.

IMHO, if the types are too complicated to work with internally, then they're too complicated to work with externally too. Aliases that are helpful internally should be packaged and offered externally.

I do think that using impl trait more often could seriously help with the types for things, although I personally tend to be of the mindset that using newtypes is generally better for public APIs as it lets you name the types more easily.

Mostly just my 2¢. Haven't read the full thread and haven't done a whole lot of work in Rust recently, so, may not be the most representative opinion of other users of the library.