tower

tower copied to clipboard

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 Yes I think that makes sense!

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

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

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.

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.