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.

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

Create hyper::Service trait #2853

Closed
seanmonstar opened this issue May 20, 2022 · 5 comments · Fixed by #2920
Closed

Create hyper::Service trait #2853

seanmonstar opened this issue May 20, 2022 · 5 comments · Fixed by #2920
Labels
A-server Area: server. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Milestone

Comments

@seanmonstar
Copy link
Member

Make a simplified Service trait that is only the call method. Update the server Connection to use it instead of tower-service.

@seanmonstar seanmonstar added A-server Area: server. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. labels May 20, 2022
@davidpdrsn davidpdrsn added this to the 1.0 milestone May 20, 2022
@tomkarw
Copy link
Contributor

tomkarw commented Jul 23, 2022

Hey guys, decided to give this a go next.

Some questions:

  • should Service be exported from root level, i.e hyper::Service or hyper::service::Service
  • I extended the scope a bit and tried to use the new Service trait wherever possible. I had to stop short of removing tracing-service dependency completely, as I couldn't refactor oneshot without it (it actually uses poll_ready heavily)

If you don't like the extended scope, we can merge any subset of commits. I tried to separate it into increments of changes.

There is also a bunch of clippy warnings on the repo, can I add a commit with some refactors here, or should I open a new PR (new PR probably 😄).

@tomkarw
Copy link
Contributor

tomkarw commented Jul 27, 2022

Hey @seanmonstar, I see you being active in other issues that I commented.
Did you miss this one, or do you need bit more time?

@seanmonstar
Copy link
Member Author

Sorry, it was still in my inbox waiting for my review, been trying to triage between my deadlines XD

should Service be exported from root level, i.e hyper::Service or hyper::service::Service

Unknown. Perhaps not root level to start. I dunno... I mean, the reason to have it inside hyper directly is part of the server API, I don't think the client stuff would use it. But probably find to stay in a separate module.

I had to stop short of removing tracing-service dependency

I didn't know we even had this dependency? While I do think the community at-large should use tower::Service, I just don't think we can justify depending on it publicly in hyper (it's complex and not stable). So, inside hyper, it's gots to go.

@tomkarw
Copy link
Contributor

tomkarw commented Jul 27, 2022

Extending the scope – music to my ears.

I'll get back when tracing-service is gone.

@tomkarw
Copy link
Contributor

tomkarw commented Jul 27, 2022

I think I got it. Had to work around the oneshot::Oneshot a bit, but it compiles 😄.
Not sure if we're not loosing something vital by removing poll_ready in context of backpressure.

One test might fail when you trigger the pipeline, dispatch_impl::drop_response_future_closes_in_progress_connection. I'll need help finding the root cause, no idea how it is related to the changes.

seanmonstar pushed a commit that referenced this issue Sep 8, 2022
This removes the dependency on `tower-service`, and simplifies the `Service` trait to be used by hyper's server connections.

Closes #2853 

BREAKING CHANGE: Change any manual `impl tower::Service` to implement `hyper::service::Service` instead. The `poll_ready` method has been removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-server Area: server. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants