hyper icon indicating copy to clipboard operation
hyper copied to clipboard

Create hyper::Service trait

Open seanmonstar opened this issue 2 years ago • 5 comments

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

seanmonstar avatar May 20 '22 18:05 seanmonstar

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 :smile:).

tomkarw avatar Jul 23 '22 21:07 tomkarw

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?

tomkarw avatar Jul 27 '22 16:07 tomkarw

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.

seanmonstar avatar Jul 27 '22 17:07 seanmonstar

Extending the scope – music to my ears.

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

tomkarw avatar Jul 27 '22 19:07 tomkarw

I think I got it. Had to work around the oneshot::Oneshot a bit, but it compiles :smile:.
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.

tomkarw avatar Jul 27 '22 22:07 tomkarw