hyper: Don't use private traits in public APIs

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.

We are working on plans for hyper 1.0, which should greatly improve on all of this.

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

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…

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>,
}

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.

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.

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<C> to a generic tower::Service<http::Request, Response=http::Response> 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.

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?

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?

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>>,

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.

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 = ...>?

@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>

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.

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!

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.