tide icon indicating copy to clipboard operation
tide copied to clipboard

Use `async-listen` crate for errorless listen

Open tailhook opened this issue 5 years ago • 6 comments

This is the part of the effort described here

Problem

Short summary for the problem that async-listen and this PR solves, is: before the PR I get the error:

$ cargo run --example hello          
   Compiling tide v0.5.1 (/work)
    Finished dev [unoptimized + debuginfo] target(s) in 5.47s
     Running `target/debug/examples/hello`
Server is listening on: http://127.0.0.1:8080
Error: Custom { kind: Other, error: Error(Accept, Os { code: 24, kind: Other, message: "Too many open files" }) }

If I run wrk against the app (i.e 1100 simultaneous connections):

wrk -c1100  http://localhost:8080/

(this is with ulimit -n of 1024 which is default on many linuxes)

And after the pull request there is no error.

Benchmarks

Before the PR (tested on the same --example hello, release build):

$ wrk -c 1000 http://locahost:8080/
Running 10s test @ http://localhost:8080/
  2 threads and 1000 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     8.69ms    3.02ms  26.77ms   71.23%
    Req/Sec    49.68k     2.74k   56.27k    71.72%
  986188 requests in 10.01s, 139.19MB read
Requests/sec:  98481.58
Transfer/sec:     13.90MB

And after the PR:

$ wrk -c 1000 http://locahost:8080/
Running 10s test @ http://localhost:8080/                                      
  2 threads and 1000 connections                                               
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     9.08ms    2.66ms  28.08ms   74.56% 
    Req/Sec    49.91k     3.05k   70.36k    71.36%
  995705 requests in 10.10s, 140.54MB read                                     
Requests/sec:  98597.23                                                        
Transfer/sec:     13.92MB                 

Which means performance is basically whiting the measurement error.

Questions

  1. What default limit should be? I thought it could depend on resource limit, but then it's another dependency, and still it's not clear how to transform ulimit to max-connections, for some apps it could be max_connections = ulimit - 10 for others max_connections = ulimit / 2. So I've set a 1000 as default.
  2. Will this PR be accepted?

tailhook avatar Jan 11 '20 01:01 tailhook

By the way, I've made a pull request to the async-std book with, perhaps more polished problem statement and description of the issues.

tailhook avatar Jan 12 '20 01:01 tailhook

Here is how errors look like with the latest commit (if env_logger is enabled):

Server is listening on: http://127.0.0.1:8080                                                                                                                 
[2020-01-13T06:37:01Z ERROR tide::server] Too many open files (os error 24). Listener paused for 0.5s. Increase per-process open file limit https://bit.ly/async-err#EMFILE 

tailhook avatar Jan 13 '20 06:01 tailhook

Note from triage: this PR is still desirable to have as it solves an actual problem.

yoshuawuyts avatar Apr 24 '20 12:04 yoshuawuyts

@tailhook this is really nice, but we are missing Clone on the resulting stream. Thish should be available on the latest [email protected] for both TcpStream and UnixStream.

dignifiedquire avatar May 22 '20 23:05 dignifiedquire

@tailhook this is really nice, but we are missing Clone on the resulting stream. Thish should be available on the latest [email protected] for both TcpStream and UnixStream.

I'll take a look later this week.

tailhook avatar May 25 '20 12:05 tailhook

Related to https://github.com/http-rs/tide/issues/586

Fishrock123 avatar Jun 12 '20 18:06 Fishrock123