Hyper: Switching to tower::Service

[seanmonstar]

We've been working a while on tower, feel like tower-service 0.2 has stabilized, so that it's becoming reasonable for hyper to depend on it without worrying about rapid breaking changes.

hyper 0.12 current has its own Service trait which was inspired by earlier versions of tower, mostly so that hyper didn't need to commit to breaking changes from tower while we iterated. Now seems like a good time to "switch". But how?

Well, we can't just swap the traits directly, as that is a breaking change. Instead, the direct swap will need to happen in hyper 0.13. In the meantime, we can provide an easy utility directly in hyper to convert tower::Services to hyper::Services.

A hyper::service::tower utility

• Add a hyper::service::tower() function that takes a tower::Service (with the appropriate associated types) and wraps it into a hyper::Service. This allows easily converting in any place that needs a hyper::Service, such as hyper::server::conn::Http::serve_connection.
• Add a hyper::service::tower::make() function that essentially takes a tower::MakeService and creates a hyper::service::MakeService. We won't actually have the bounds use tower's MakeService, but that's just to not depend on the less stable tower-service-util.

Example

This ends allowing code like this:

let mk_svc = some_tower_stack();

let srv = hyper::Server::bind(addr)
    .serve(hyper::service::tower::make(mk_svc));

Implement tower::Service for client stuff

• It's pretty straight forward how we would imlement tower::Service for hyper::client::conn::SendRequest.
• It's less clear what a hyper::Client should be. Is it a Service? Sorta, you can send requests and get responses. But internally it manages a connection pool (well, a Service pool?). But it's not really a MakeService either, as it doesn't have a way to hand out those pooled Services, it just uses them internally...

Questions

• Does implementing Service for Client or hyper::client::conn::SendRequest make it harder for those to eventually convert to async fn? Yea kinda, as we could no longer put those opaque future types in the associated types of Service, at least not until trait existential types is stable.
  
  
  
  • One suggested option is to not implement Service directly, but to add some into_service(self) -> impl Service methods. That way does allow the opaque types in the associated types.
  
  
  • The downside with that suggestion is it prevents naming the impl Service externally, which is still frequently done...
  
  

[seanmonstar]

cc @carllerche @LucioFranco @davidbarsky

[LucioFranco]

First, I am really excited for this!

So at a high level, is it worth it right now to introduce kinda a halfway there approach instead of going all in and releasing 0.13.0? Are these compatibility types something that could in the meantime just live in tower-hyper?

For me the biggest things I'd like to see in the transition to tower is:

• Transition to BufStream instead of Payload so that we can take advantage of the tower ecosystem without having to do any "lifting". This would enable us to use hyper with tower-grpc and tower-consul since they both use HttpService.
• Provide full compatibility with the growing tower ecosystem layers.

That said, with the BufStream transition we could in theory provide all the tower compatability _without_ having to include tower within the main hyper crate as done in tower-hyper.

For the clients, I'm not totally sure what the right path is for them as well. I have been going back and forth between providing a Service interface for hyper::Client or providing the lower-level MakeService and Service for the hyper::client::conn::* utils. Ideally I guess I would love to see the pool decoupled from hyper itself and implemented as its own tower-http utility, which should allow for us to provide different pool strategies, so maybe this is something worth exploring?

All in all, Im really excited to move this forward but I still think there are some unanswered questions still in tower itself around what is a _client_, does it provide a target -> Future<Service> interface or would it just be a Service + Clone out of the box kind of like how hyper::Client is right now.

[LucioFranco]

And to add something else, at work we have been using the Service + Clone method to wrap rusoto and hyper. It seems to be working quite well but have not explored it enough. But would love to hear more on that!

[seanmonstar]

Transition to BufStream instead of Payload

Oh that's right, thanks for reminding me. I think that change is a little trickier:

• BufStream is basically the poll_data part of Payload, but has no trailers.
• tower-http offers a Body trait that is based off Payload. It can poll data bufs, and trailers, which hyper needs. However, it's a separate trait from BufStream, doesn't have it as a super trait, and there is a blanket implementation of Body for all T: BufStream.

The issue this creates is that in order for something to be both a BufStream and Body, it could only manually implement BufStream, and assume the blanket Body implementation, which skips trailers.

[quininer]

The issue this creates is that in order for something to be both a BufStream and Body, it could only manually implement BufStream, and assume the blanket Body implementation, which skips trailers.

I think tower-http-service takes this into account. We can implement the Body and then get BufStream by BodyExt.

[davidbarsky]

These thoughts aren't complete, nor are they particularly well-edited, but here goes:

1. I think a breaking change to support Tower everywhere in Hyper is _probably_ okay, given the last breaking release to Hyper was June 1st, 2018, almost ten months ago.
2. I kinda view hyper::Client as a particularly chunky Service that manages connection pooling, re-connection, and whatnot. From the perspective of Tower, I think it can be viewed as a newtype produced by a bunch of Layers + a ServiceBuilder.
3. Speaking of naming services, I'm somewhat split:
   
   
   
   1. It might make sense to encourage people to create new clients on the fly, especially if the underlying connection pool can be shared and reused, side-stepping the naming issue, and a bunch of unpleasant Arc<Mutex<T>> to get around borrow checking errors.
   
   
   2. Hyper could vend some common client configurations under a newtype'd struct with the uncomfortably large type signatures tucked away behind a field + concrete types, with various policies (timeouts, retries, the like) would be accessible through some sort of builder.
   
   

I can't comment on the BufStream/Body stuff as I don't have enough context or expertise on the matter.

[LucioFranco]

@seanmonstar could we add an impl<T: Payload> Body for T>?

[seanmonstar]

I haven't tried, but I don't believe we can implement a blanket implementation of a foreign trait in hyper...

[carllerche]

Roughly, I think the proposed step of introducing Tower support without introducing any breaking changes is good. This would add initial support while we figure out the best "breaking change" API.

Looking at hyper::Client, it does look like it should implement Service itself. You are correct that it does a lot internally, but in 0.12, the simplest strategy will be to just implement Service for it. Once a breaking change happens, I think the best option will be to have struct Client<T: HttpService = DefaultStack> { ... } or something.

I think there may be an alternate way to add support for tower_http_service::Body... Starting with Client:

You don't have a bound on B in the struct definition, so you can impl tower::Service for Client<B> where B: tower_http_service::Body. The same strategy may work with the server half. Basically, you ignore the Payload trait when implementing the tower traits.

I do think that we should explore what an 0.13 API may look like more in tower-hyper before committing on breaking changes.

[legokichi]

Can I use server-sent event (event source https://github.com/hyperium/hyper/issues/719) after this change?

[bstrie]

I see this is listed under the 0.13 milestone, what work remains to be done for this issue?

[seanmonstar]

I think it's mostly done!