Consider switching Service::Request to a generic

[carllerche]

Problem

Currently, Service represents the request as an associated type. This was originally done to mirror Sink in the futures crate (relevant issue). However, requests are an "input" to Service which can make representing some things a bit tricky.

For example, HTTP services accept requests in the form of http::Request<T: BufStream>. In order to implement a service that handles these requests, the service type must have a phantom generic:

struct MyService<RequestBody> {
    // ...
    _phantom: PhantomData<RequestBody>,
}

impl<RequestBody> Service for MyService<RequestBody> {
    type Request = http::Request<RequestBody>;
}

This, in turn, can cause type inference issues as the request body must be picked when MyService is initialized.

Proposal

The Service trait would become:

pub trait Service<Request> {
    type Response;
    type Error;
    type Future: Future<Item = Self::Response, Error = Self::Error>;

    fn poll_ready(&mut self) -> Poll<(), Self::Error>;

    fn call(&mut self, req: Request) -> Self::Future;
}

This moves the request type to a generic.

Impact

The full impact of this change is unknown. My best guess is it will be mostly ergonomic. For example:

Before:

fn foo<S: Service>(service: S) { }

After:

fn foo<R, S: Service<R>>(service: S) { }

And it linearly gets worse on the number of generic services:

Before:

fn foo<S1, S2>(s1: S1, s2: S2)
where
    S1: Service,
    S2: Service<Response = S1::Response>,
{ }

After:

fn foo<S1, S2, R1, R2>(s1: S1, s2: S2)
where
    S1: Service<R1>,
    S2: Service<R2, Response = S1::Response>,
{ }

today there already are plenty of cases in which the one has to add additional generic parameters. In order to bound an HTTP service today, the following is required:

fn foo<S, B>(service: S)
where
    S: Service<Request = http::Request<B>>,
    B: BufStream,
{ }

In practice, this turns out to be quite painful and it gets unmanageable quickly. To combat this, "alias" traits are created. It is unclear how this change would impact the ability to use this trick.

Evaluation

In order to evaluate the impact of the proposed change, the change has been applied to Tower, and a number of projects that use Tower.

• tower
• tower-h2
• tower-grpc
• tower-web
• linkerd2-proxy

[carllerche]

@withoutboats Given your work w/ earlier work w/ tokio_service and your lang expertise, I would appreciate your thoughts on this proposal.

[hawkw]

FWIW, I'm taking a crack at making this change and updating the rest of the tower stack to work with it, in order to prove out the surface area of the change.

[withoutboats]

At least hypothetically this makes sense: Service is a lot like Fn, which is generic over its arguments and has an associated type for its output. So if its ergonomic, that seems great to me.

[hawkw]

For anyone interested: this branch rewrites all of tower to use a Service type that's generic over the Request type. I'm going to continue experimenting with rewriting dependent crates like tower-web to compile with this branch as well.

Some preliminary observations:

• Making this change seems to make a lot of boilerplate PhantomData fields necessary on pretty much all middleware types, which, although not that big a problem, is a bit of a pain.
• It's no longer possible to implement EitherService as just an enum.
• The changes necessary to make the tests compile are a decent preview of how this might impact code that depends on tower...

Edit: as @withoutboats suggested, I've gone back and removed almost all the uses of PhantomData in my branch, so most of the above comments are no longer the case.

[withoutboats]

@hawkw a lot of these phantomdata seem to be because you're applying the Service or Discover bounds more aggressively than necessary. For example, I believe EitherService could continue to be an enum if you just didn't bound the constructors in any way.

[hawkw]

@withoutboats In most of these cases, the type bounds were already present in the original code; I tried not to remove any type bounds that were already present.

That said, going back, you're right that there were a number of middleware types that didn't actually need to be parameterized over the request type of the wrapped service. I thought these were only added when the compiler actually told me they were necessary, but that doesn't seem to be the case --- I'm in the process of cleaning the branch up a bit. Thanks!

[withoutboats]

@hawkw I know you didn't add them, but I believe if you removed them where they aren't strictly necessary you would probably be able to eliminate just about every PhantomData you had to add. :)

[hawkw]

@withoutboats Yup, I removed most of the unnecessary bounds (and thus, the PhantomDatas) as you suggested in ab8a528; it looks much nicer now. Thanks.

Originally, I didn't want to remove the bounds just to keep the diff smaller, but I think it ended up being worth it.

[seanmonstar]

While it's been common for Rust to not add bounds for constructors and things, and only for the trait impls when it needs them, I've actually found that there's benefit in them being in the constructors for this reason: it helps tame the errors produced by the compiler.

It's easier to notice the error saying WithPeakEwma::new(bad_type) doesn't implement the necessary traits than the huge error saying the eventual future doesn't implement Future. 🤷

[hawkw]

For those watching from home: this branch rewrites tower-h2 to depend on the generic-request version of tower-service, and this branch makes the same change in tower-http.

Writing these branches was mostly pretty painless, aside from one issue that threw me off for a bit --- generic type parameters in a blanket impl of a trait for T: Service where the implemented trait accepts no type parameters being considered unbounded by the compiler. In this case, the trait was just a Sealed marker trait to protect impls from outside of the crate, and I was able to add a version that accepts a type parameter of its own to resolve this issue, but there may be cases where it's more of an issue elsewhere.

[carllerche]

@seanmonstar

While it's been common for Rust to not add bounds for constructors and things, and only for the trait impls when it needs them, I've actually found that there's benefit in them being in the constructors for this reason: it helps tame the errors produced by the compiler.

I actually agree w/ you. What I have done (when I took the time) is put constructors in dedicated impl blocks that include the bounds.

impl<T: Foo> {
    pub fn new(foo: T) -> Self { ... }
}

impl<T> {
   pub fn get_ref(&self) -> &T { ... }
}

It works pretty well.

[hawkw]

And here is a branch rewriting tower-grpc to work with this change as well. There are admittedly some spots that are a bit ugly, particularly in codegen, but everything works.

While tower-grpc is complex enough that making the change was fairly laborious, I didn't run into any particular difficulties caused by changing the request type to a generic parameter.

[hawkw]

As an example of the impact that this change might have on a larger codebase, here is the Linkerd 2 proxy rewritten to use my tower, tower-h2, and tower-grpcbranches: https://github.com/linkerd/linkerd2-proxy/compare/eliza/generic-service?expand=1

I'll try to write up some of my thoughts on the experience and post them here as well.

[hawkw]

Here are some of my experiences thus far:

• Types implementing Service that are themselves generic over a Service can only constrain the generic type in the implementation of Service for that type, because the request type parameter to the Service implementation that type is generic over is otherwise unbounded. For example, currently, you can do this:

  pub struct MyMiddleware<S> {
      inner: S,
  }
  
  impl<S: Service> MyMiddleware<S> {
      pub fn new(inner: S) -> Self {
          Self { inner }
      }
  }
  
  impl<S: Service> Service for MyMiddleware<S> {
      // ...
  }

  However, when the request type becomes a generic parameter, constraining the constructor with S: Service is no longer possible. Similar code like this:

  pub struct MyMiddleware<S> {
      inner: S,
  }
  
  impl<S: Service<R>, R> MyMiddleware<S> {
      pub fn new(inner: S) -> Self {
          Self { inner }
      }
  }
  
  impl<S: Service<R>, R> Service<R> for MyMiddleware<S> {
      // ... 
  }

  is rejected by the compiler due to error 0207:

  error[E0207]: the type parameter `R` is not constrained by the impl trait, self type, or predicates
    --> src/main.rs:10:21
     |
  10 | impl<S: Service<R>, R> MyMiddleware<S> {
     |                     ^ unconstrained type parameter
  

  Although the constraint of S: Service on the constructor is not strictly necessary, as @seanmonstar points out above, it can lead to error messages that are easier for users of a library to understand.

  For structs or enums which are generic over Service parameters, this can be resolved by adding a PhantomData field, although this is additional boilerplate. However, for blanket impls of traits for T: Service, it's not possible to add PhantomData. For example, it's currently possible to write code like this:

  trait MyTrait {
      // ...
  }
  
  impl<T> MyTrait for T where T: Service {
      // ...
  }

  When Service is generic over the request type, however, this is no longer the case. Similar code like:

  trait MyTrait {
      // ...
  }
  
  impl<T, R> MyTrait for T where T: Service<R> {
      // ...
  }

  is now rejected as the R type parameter to the impl of MyTrait is unconstrained. This was an issue in some cases, such as tower-grpc.

• "Bridging the gap" between traits with generic parameters and associated types is an interesting problem. It's possible to implement a generic trait for a trait with associated types, supplying associated types for the type parameters. For example, this code compiles with the current version of tower-service:

  trait MyServiceLikeTrait<R, F> {
      fn something_like_call(&mut self, R) -> F;
  }
  
  impl<T: Service> MyServiceLikeTrait<T::Request, T::Future> for T {
      fn something_like_call(&mut self, r: T::Request) -> T::Future {
          self.call(r)
      }
  }

  On the other hand, it's not possible to implement a trait with associated types for another trait which is generic and assign the type parameters to the associated types. For example, the code:

  trait MyServiceLikeTrait {
      type Request;
      type Future;
      fn something_like_call(&mut self, Self::Request) -> Self::Future;
  }
  
  impl<T, R> MyServiceLikeTrait for T where T: Service<R> {
      type Request = R;
      type Future = T::Future;
      fn something_like_call(&mut self, r: Self::Request) -> Self::Future{
          self.call(r)
      }
  }

  does not compile, because --- again --- the R parameter on the impl is unconstrained.

• In the issue description, @carllerche points out that

  today there already are plenty of cases in which the one has to add additional generic parameters. In order to bound an HTTP service today, the following is required:

  fn foo<S, B>(service: S)
  where
      S: Service<Request = http::Request<B>>,
      B: BufStream,
  { }

  In practice, this turns out to be quite painful and it gets unmanageable quickly. To combat this, "alias" traits are created. It is unclear how this change would impact the ability to use this trick.

  Creating these "alias" traits is still possible. However, as I discussed above, if the Service's request type is parameterized in some way, the parameters to the "alias" trait that are part of the request type must be provided as generic parameters rather than associated types.

• Often, one may want to write middleware which use the associated types of a wrapped service as the types of fields. For example, a middleware type might return a ResponseFuture<T> struct which includes a field of type T::Future, where T: Service. This is still possible, but in order to add a Service bound on T so that the associated type T::Future may be used, it is now necessary for the middleware type to be generic over the request type as well . The response future might now have a type like ResponseFuture<T, R> where T: Service<R>. This means that when services are composed of large stacks of nested middleware, their type signatures may sometimes repeat the request type multiple times.

  For example, consider this type in the Linkerd 2 proxy. Prior to making the request type generic, it was already pretty unwieldy:

  type Service = InFlightLimit<Timeout<Buffer<Balance<
      load::WithPeakEwma<Discovery<B>, PendingUntilFirstData>,
      choose::PowerOfTwoChoices,
  >>>>;

  Now, some of the middleware must be parameterized over the request type, and it becomes somewhat noisier:

    type Service = InFlightLimit<Timeout<Buffer<Balance<
      load::WithPeakEwma<Discovery<B>, PendingUntilFirstData>,
      choose::PowerOfTwoChoices, http::Request<B>;
  >, http::Request<B>>>>;

  In the Linkerd 2 proxy codebase, we often use type aliases to break apart complex types like this one. This trick still works, and each alias isn't made significantly more complex by the added repetition. However, if the compiler expands these aliases in a type error message, the resulting long type is often significantly longer --- this was an issue that I encountered while making this change in the proxy.

• Similarly, types which use the <T as Trait> syntax for disambiguating associated types can become much more verbose as well, since the Service trait must be parameterized

• This is admittedly subjective, but personally, I think type signatures where both the Request and Response types are associated types are more understandable for the reader, especially for newcomers to the library. For example, consider this where clause in tower-grpc. On the current master, it looks like this:

    where S: UnaryService<Request = T::Decode,
                         Response = T::Encode>,
          B: Body<Data = Data>,

  After changing the Request associated type on Service to a type parameter, it's also necessary to change the associated type on tower-grpc's UnaryService trait, as I discussed above. Now, the where clause looks like this:

    where S: UnaryService<T::Decode,
                         Response = T::Encode>,
          B: Body<Data = Data>,

  In my opinion, this doesn't indicate as clearly that the UnaryService must accept requests of type T::Decode.

I don't feel like any of these issues are necessarily significant enough for me to come down strongly against making this change. However, I do feel like making this change has some impacts on ergonomics that ought to be taken into account when deciding whether or not to move forwards with it.

[seanmonstar]

Just from watching this unfold, while it does seem like from a purity point-of-view, the request should be a generic, in practice it seems like it is better to be an associated type.

[hawkw]

@seanmonstar That's roughly how I feel about it as well. At first glance, it certainly seems like the Right Thing, but it feels like it makes a lot of fairly common patterns with Services less ergonomic.

[carllerche]

@seanmonstar could you dig in more detail. I’d like to hear more specifically what you consider the factors that tilt it towards keeping it as an associated type.

We haven’t hit significant problems with the associated type yet, but I expect that the associated type will cause problems with reusable stack definitions once we get there. I will try to sketch out what I mean ASAP.

[hawkw]

And here's tower-web rewritten to use generic request types as well.

Making the change wasn't too difficult, but I think the resulting APIs may be a bit less easy to use (and/or uglier).

[withoutboats]

Just from watching this unfold, while it does seem like from a purity point-of-view, the request should be a generic, in practice it seems like it is better to be an associated type.

I want to push back on this in one way: the practical experience here is with code that was all originally written with an associated type. Patterns that emerge from that definition of Service (like applying the Service bound liberally, because it has no parameters) are definitely easier, whereas patterns that would emerge from the new definition (services that are generic over multiple kinds of responses) would be less likely to emerge, because they're less convenient to write.

That is, rewriting existing code will tend to favor the old API, but writing new code might not have the same experience.

[carllerche]

@withoutboats it would be helpful if you have any thoughts as to new patterns :)

[hawkw]

@withoutboats

That is, rewriting existing code will tend to favor the old API, but writing new code might not have the same experience.

That's true, and I'll freely admit that when I've been making this change in existing codebases, I've tried to intentionally make the smallest necessary change to get it to compile, rather than rewriting it from scratch.

That said, I'm somewhat wary of weighing things that work now against potential future patterns --- I'm much more comfortable with comparing the drawbacks of the change with the patterns that we already know it would enable...

[withoutboats]

@carllerche I don't have particular examples but vaguely, let's say you want to take any request that implements a certain trait:

impl<R: MyTrait> Service<R> for MyService { }

To do this with the assoc type API, you need to litter PhantomData:

struct MyService<R> {
    _request: PhantomData<R>,
    ...
}

impl<R: MyTrait> Service for MyService<R> { ... }

This applies equally well if you want to write a service over a defined set of concrete types, e.g. impl Service<Request1> for MyService and impl Service<Request2> for MyService. With the existing API you would again need PhantomData to inject that parameter into MyService.

There's an inverse relationship between the painpoints: if you want your service to be generic in the old API, you need to add PhantomData, whereas if you want to be generic over Services in the new API, you need to add PhantomData.

[withoutboats]

It's possible also there's a bridge that makes this easier by implementing this trait for your services that are not generic

trait SpecificService: Service<Self::Request> {
    type Request;
}

I think there's no way to provide a blanket impl of this, but if you are okay with expecting non-generic services to write this impl, I think you can then just write S: SpecificService to take any non-generic service.

[carllerche]

@hawkw regarding the new fn bound, can this be solved by adding a where clause on the new fn instead of the impl?

[carllerche]

@hawkw re “bridging the gap”

"Bridging the gap" between traits with generic parameters and associated types is an interesting problem.

I think we just wouldn’t try to do that. Aliases would have to have a generic as well. How do things look is you go that route.

[carllerche]

@hawkw re ‘’ the linked example shouldn’t actually need that syntax?

In general, I agree it is even more painful to do, but I believe that T as Trait should be minimized in general. Can it be completely avoided in the various codebases you updated?

[hawkw]

@carllerche

regarding the new fn bound, can this be solved by adding a where clause on the new fn instead of the impl?

Yes, that does work. It's a little less ergonomic in cases where there's several inherent functions on the middleware struct and they all need the where bound, but that's not a huge problem...

re “bridging the gap”

"Bridging the gap" between traits with generic parameters and associated types is an interesting problem.

I think we just wouldn’t try to do that. Aliases would have to have a generic as well. How do things look is you go that route.

I think we're on the same page here -- sorry if my earlier post wasn't clear about that. What I was saying was that while it's possible to implement a trait with associated types for a generic type, it's not really possible to implement a generic trait for another trait with associated types. So, I didn't do this in any of my branches --- aliases must always be generic as well, as you said.

In general, I agree it is even more painful to do, but I believe that T as Trait should be minimized in general. Can it be completely avoided in the various codebases you updated?

I think almost all of the cases of <T as Trait> syntax in the codebases I updated were already there --- this change doesn't necessitate the use of more <T as Trait>s because it reduces the number of associated types, and <T as Trait> is only necessary for disambiguating associated types. However, it does make that syntax somewhat more painful to use.

I'm not sure how much of the T as Trait syntax in the libraries I've updated can be removed. Some of it may be unnecessary but I suspect a lot of it can't be removed without significant restructuring, though I would have to actually try to rewrite that code to be sure.

[carllerche]

@hawkw Would you be able to take your branches and submit them as PRs to your forked repo, that would make discussing the specific changes easier.

[hawkw]

@carllerche I've opened the following PRs:

• [RFC] Change Service associated Request type to a type parameter hawkw/tower#1
• [RFC] Make Service generic hawkw/tower-h2#1
• [RFC] Change Service associated Request type to a type parameter hawkw/tower-grpc#1
• [DRAFT] distroless docker builds hawkw/linkerd2-proxy#1

[carllerche]

I believe that tower-rs/tower-grpc#64 is a pretty strong argument for making this change.

[carllerche]

@hawkw is it possible to remove LiftReqBody from the public API here after the change?

[hawkw]

I've opened hawkw/tower-web#1 for the change to tower-web, sorry I forgot that yesterday.

[carllerche]

@seanmonstar any updated thoughts? I'm thinking we should do this and move forward with the change sooner than later.

[seanmonstar]

Yea, seems good. +1