Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

service: improve unsized types' support #650

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

service: improve unsized types' support #650

wants to merge 1 commit into from

Conversation

cppforliving
Copy link
Contributor

While Service impl for Box<S> covers unsized S case, the impl for &mut S is not, this PR is an attempt to fix that.

Also there are a few places in Service docs where the poll_ready call seems to be missing.

@davidpdrsn
Copy link
Member

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 service_fn.

@cppforliving
Copy link
Contributor Author

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 tower::util::Ready as its current impl relies on the &mut Service blanket impl. Maybe a bit unrelated example, but std::future::Future has a blanket impls for &mut Future and Box<Future> which to me seems quite useful, so maybe it would be worthy to keep similar blanket impls for tower::Service as well, just thinking...

@cppforliving
Copy link
Contributor Author

Any thoughts on this?

davidpdrsn added a commit that referenced this pull request Mar 21, 2022
@davidpdrsn
service: Impl Service for async functions
5327f0d 
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.
davidpdrsn added a commit that referenced this pull request Mar 21, 2022
@davidpdrsn
service: Impl Service for async functions
cd8e729 
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.
@davidpdrsn davidpdrsn mentioned this pull request Mar 21, 2022
Draft
3 tasks
@davidpdrsn
Copy link
Member

I filed a proposal for removing the old blanket impls #657

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cppforliving @davidpdrsn