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

[Draft] tower_service::Service <-> warp::Filter interop #322

Closed
wants to merge 3 commits into from

Conversation

jxs
Copy link
Collaborator

@jxs jxs commented Nov 26, 2019

I continued the work started on #70 to try and make tower_service::Service's run on warp and the ability to run warp::Filter's as a service on Hyper.

Notes

  • to be able to call tower_service::Service::call(&mut self, req: Request) I had to remove the Arc usage and Clone to the trait bounds, I benchmarked and results seemed similar benchmarking the hello world example with --release:

(with Arc)

$ wrk -t4 -c1000 -d30s --latency http://127.0.0.1:3030
Running 30s test @ http://127.0.0.1:3030
  4 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.34ms    3.79ms  82.56ms   87.93%
    Req/Sec    12.71k     3.47k   22.53k    65.86%
  Latency Distribution
     50%    3.19ms
     75%    5.16ms
     90%    8.69ms
     99%   18.67ms
  1519131 requests in 30.06s, 188.34MB read
  Socket errors: connect 751, read 114, write 2, timeout 0
Requests/sec:  50530.67
Transfer/sec:      6.26MB

(with Clone)

$ wrk -t4 -c1000 -d30s --latency http://127.0.0.1:3030
Running 30s test @ http://127.0.0.1:3030
  4 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.31ms    2.95ms  41.62ms   81.27%
    Req/Sec    12.68k     3.62k   22.61k    60.70%
  Latency Distribution
     50%    3.57ms
     75%    5.20ms
     90%    8.03ms
     99%   14.95ms
  1517745 requests in 30.08s, 188.17MB read
  Socket errors: connect 751, read 102, write 42, timeout 0
Requests/sec:  50463.45
Transfer/sec:      6.26MB
  • I looked into Consider removal of warp::ext::set #222 and since the WarpService trait already existed and is also passing the remote_addr extra argument, so I impled WarpService for T: tower_service::Service , when a Service is called via warp remote_addr(or other future arguments's, see Add warp::tls::peer_certificates() filter. #318) is discarded

  • impl WarpService for T: tower_service::Service requires tower_service::Service::Error : warp::Reject that seemed to be the simplest way to then integrate Service errors with warp's logging and error handling, should another error be used/should Reject be impled for T?

  • I think there still needs to be developed a way to compose warp::Filter's with tower_service::Service's, but maybe that could be done after closing the interop API?

- add Filter::into_service function that converts a Warp Filter into a
 tower_service::Service
- add example running a warp Filter on Hyper
add warp::serve_service which runs tower_service::Service<Request<Body>>
use tower_service::Service;

#[derive(Clone)]
struct TowerService;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use service_fn as an example here

@seanmonstar
Copy link
Owner

Thanks for the draft, I used what you had here in #367.

@seanmonstar seanmonstar closed this Jan 8, 2020
@jxs
Copy link
Collaborator Author

jxs commented Jan 8, 2020

awesome! do you plan to impl Filter for Service on a following PR?

@seanmonstar
Copy link
Owner

I'm not so sure there needs to be a conversion the other way around, one can svc.call inside a filter.and_then if need be.

@jxs jxs deleted the tower-service branch January 8, 2020 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants