Skip to content

feat(server): allow creating Server with shared Handle #1348

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

Closed
wants to merge 1 commit into from

Conversation

kamyuentse
Copy link
Contributor

  1. impl Future for Server [WIP]
  2. add method bind_handle to Http
  3. add an example to use shared Handle in multiple server

More details about the impl Future for Server should be discussed, the mechanism shutdown_signal like what run_until do may require external fields in struct Server to impl in Server's Future.

simple performance bench:

  • example/server:
wrk -t4 -c100 -d30s http://localhost:1337
Running 30s test @ http://localhost:1337
  4 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    13.31ms   10.16ms 179.49ms   93.49%
    Req/Sec     2.07k   600.64     3.70k    80.50%
  247321 requests in 30.08s, 21.23MB read
Requests/sec:   8222.20
Transfer/sec:    722.65KB
  • example/multi_server, run two wrk test in the same time:
wrk -t4 -c100 -d30s http://localhost:1337
Running 30s test @ http://localhost:1337
  4 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    22.01ms    8.13ms 136.12ms   86.13%
    Req/Sec     1.16k   293.74     2.48k    85.67%
  139004 requests in 30.04s, 12.20MB read
Requests/sec:   4627.80
Transfer/sec:    415.78KB
=====================================================
wrk -t4 -c100 -d30s http://localhost:1338
Running 30s test @ http://localhost:1338
  4 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    21.72ms    8.08ms 136.10ms   84.03%
    Req/Sec     1.18k   341.48     2.67k    84.92%
  140851 requests in 30.04s, 12.36MB read
Requests/sec:   4688.83
Transfer/sec:    421.26KB

@seanmonstar
Copy link
Member

Awesome! I haven't had a moment to look this over, I hope to find time tomorrow.

@seanmonstar
Copy link
Member

Curious, when you paste the results from the multi-server example, is that running two instances of wrk at the same time, or 1 for the first port, and then 1 for the second port? I'm worried if only 1 at a time, that means the performance somehow dropped significantly...

@seanmonstar
Copy link
Member

the mechanism shutdown_signal like what run_until do may require external fields in struct Server to impl in Server's Future.

Yea, so either we'd need to:

  • Store a boxed future on the struct that can be selected on. It'd probably be better to not need a box.
  • Possibly Server itself shouldn't implement Future, but instead be a builder that can be combined with a shutdown signal to create yet another type, ServerFuture (maybe a better name), that is generic over the shutdown signal.

Or perhaps Server still can implement future, which would internally just be a ServerFuture<Empty<(),()>>, and setting the shutdown signal returns a ServerFuture<F>...

@kamyuentse
Copy link
Contributor Author

is that running two instances of wrk at the same time, or 1 for the first port, and then 1 for the second port? I'm worried if only 1 at a time, that means the performance somehow dropped significantly...

previous multi_server test ran at the same time, 1 test per port.
using

#!/bin/bash

wrk -t4 -c100 -d30s http://localhost:1337 > result1.txt &
wrk -t4 -c100 -d30s http://localhost:1338 > result2.txt &

If just run 1 test on 1 port, the result could be

wrk -t4 -c100 -d30s http://localhost:1337
Running 30s test @ http://localhost:1337
  4 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    12.38ms    8.25ms 149.64ms   94.21%
    Req/Sec     2.21k   516.48     2.57k    89.17%
  264028 requests in 30.02s, 23.17MB read
Requests/sec:   8795.68
Transfer/sec:    790.24KB

similar with the server example result. Because in previous test, two Server share the Handle, so the result shown that the ability to deal with the request is a half of the server example.

  • Store a boxed future on the struct that can be selected on. It'd probably be better to not need a box.
  • Possibly Server itself shouldn't implement Future, but instead be a builder that can be combined with a shutdown signal to create yet another type, ServerFuture (maybe a better name), that is generic over the shutdown signal.

I will try to implement this proposal later.

@srijs
Copy link
Contributor

srijs commented Oct 18, 2017

I'm keen to understand why we need a shutdown signal in the first place, thus requiring the Server to hold on to the signal future. Could this not just be method like Server::shutdown(&mut self) instead? This seems like it would be more generally useful.

For convenience, you could always construct some sort of Shutdown future around Server which when polled polls the shutdown signal first, and if successful calls Server::shutdown.

What am I missing?

@seanmonstar
Copy link
Member

@srijs that's a great suggestion, thanks! Indeed, the shutdown signal and timeout don't necessarily need to live in the base future. With a shutdown method, anyone can implement graceful shutdown with a timeout themselves...

@seanmonstar
Copy link
Member

So, a question that has come up in #1342 is if Server should instead be a Stream of Connections, where each Connection is a Future managing HTTP on that socket. If so, that'd conflict with Server being a Future itself...

@kamyuentse
Copy link
Contributor Author

kamyuentse commented Oct 29, 2017

@seanmonstar Should we decouple the reactor from Server, then merge Http and Server, and introduce a type named ServerBuilder?

@seanmonstar
Copy link
Member

I've been working with a start from this PR, and here's what I've got so far:

  • Http::serve_addr -> Serve which is a Stream<Item=Connection>
  • Server::spawn_all(executor: E) -> Future which will spawn all incoming Connections onto the executor.

There's still the question of handling a shutdown signal on the SpawnAll. There could perhaps be SpawnAll::run_until<F>(f) -> RunUntil<F> or something...

@seanmonstar
Copy link
Member

I've been stumped the past couple days because tokio will very soon have the concept of a default handle, as well tokio_core::Handle no longer being the preferred executor, and so haven't particularly wanted to settle on a new API that would change in a couple weeks.

I could just push what exists now, without the RunUntil or whatever...

(PS, I've just been amending your commit, so it still has your name on it. Hope that's OK...)

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.

3 participants