service: improve unsized types' support #650
Conversation
|
I'm curious what your use case for this is 🤔 I've personally considered removing this blanket impl and instead have a blanket impl for async functions so we wouldn't need |
|
Not that I particularly need this, I just spotted #636 so I looked at tower-service API and noticed this minor asymmetry. I really like the idea of adding blanket impl for async functions, however it seems that removing existing blanket impls would require reworking |
|
Any thoughts on this? |
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.
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.
|
I filed a proposal for removing the old blanket impls #657 |
There was a problem hiding this comment.
I've read through this and #657. From my understanding this PR makes the blanket impl for &mut S where S: Service more generic. I think this is a good idea. Blanket trait impls should be as generic as they can be, part of the robustness principle.
I don't see why #657 and adding a blanket Service trait impl for Fn() types would mean we wouldn't want to offer these reference type impls too.
The doc example changes make sense to me too. We should definitely not be showing examples that breach the "wait for ready before calling" API contract of Service.
4 participants
While
Serviceimpl forBox<S>covers unsizedScase, the impl for&mut Sis not, this PR is an attempt to fix that.Also there are a few places in
Servicedocs where thepoll_readycall seems to be missing.