service: Impl Service for async functions

[davidpdrsn]

This PR just a proposal, hence being a draft, but I think its something
we should at least consider for tower-service 1.0.

The idea is to remove the blanket Service impls for &mut S and
Box<S> and replace them with an impl for F where FnMut(Request) -> Future<Output = Result<Response, Error>>, and then remove service_fn.

I personally think and impl for async fns is more valuable mainly to
hammer home the "services are just async fns" line.

I haven't yet encountered any actual need for the previous impls.
Ready was the only thing in tower that used it and it could be easily
changed to not require them.

If we decide to do this I think we should consider doing the same for
Layer, i.e. remove impl Layer for &'a L and make Fn(S) -> S2 impl
Layer.

This came out of some talk in #650.

TODO

Should we decide to move forward this, these are some things to fix in this PR:

• Fix tests
• Mention the impl in the docs
• Docs

[hawkw]

Hmm, this is an interesting proposal! I don't have a great sense for how much user code requires the previous impl Service for &mut S, but I agree that it would be much easier to teach the concept that "Services are just async fns" if async fn actually implemented Service...and, not having to write service_fn everywhere probably improves new user experience a bit.

However, I will note that this PR by itself does completely remove the ability to pass a mutable borrow of a Service to a function that takes an S: Service, which I believe is the primary reason the impl for mut refs existed. I think that, if we end up agreeing that implementing Service for FnMuts is a better use of the blanket impl than for mutable references, we could at least consider adding a by-reference adapter in ServiceExt, like

impl<S: Service<R>, R> ServiceExt<R> for S {
   // ...

   fn by_ref(&mut self) -> ByRef<'_, S> {
       ByRef(self)
   }
 }

impl<S: Service<R>, R> Service<R> for ByRef<'_, S> {
    // ...
    fn poll_ready(&mut self, cx: ...) -> Poll<Result<(), S::Error>> {
       self.0.poll_ready(cx)
   }

   fn call(&mut self, req: R) -> S::Future { 
       self.0.call(req)
   }
}

this way, if I have a function or something that takes a S: Service<R> , I can pass a Service I only have a mutable reference to by calling svc.by_ref().

Does that make sense?

[davidpdrsn]

@hawkw Yes I think that makes sense!

Edit: I've pushed a ByRef impl. Still missing docs.

[cppforliving]

There seems to be one subtle difference between ServiceFn<T> and blanket impl for async fns. ServiceFn<T> has unconstrained Debug impl while async fns and closures returning futures do not implement Debug, so a tower stack with async fn at the bottom may no longer satisfy Debug trait bounds. Not sure how crucial is this for the users, but IMHO worth mentioning as a potential drawback

[hawkw]

There seems to be one subtle difference between ServiceFn<T> and blanket impl for async fns. ServiceFn<T> has unconstrained Debug impl while async fns and closures returning futures do not implement Debug, so a tower stack with async fn at the bottom may no longer satisfy Debug trait bounds. Not sure how crucial is this for the users, but IMHO worth mentioning as a potential drawback

Hmm, that's a good point, we should definitely consider that before deciding whether or not to make this change. We could still provide service_fn as well in order to allow users to avoid unconstrained Debug impls...but if we do that, we now have two ways to do the same thing, and maybe it is no longer worth spending the one blanket impl we get on Fns in that case.

[LucioFranco]

Hmm, that's a good point, we should definitely consider that before deciding whether or not to make this change. We could still provide service_fn as well in order to allow users to avoid unconstrained Debug impls...but if we do that, we now have two ways to do the same thing, and maybe it is no longer worth spending the one blanket impl we get on Fns in that case.

Yeah but I bet discovering this would be quite painful because those errors are likely quite rough. Though we could add a check debug method like we have for clone.