Support persistent connections (Connection: keep-alive)

[clue]

This changeset finally adds support for persistent connections (Connection: keep-alive) 🎉

This is a pure feature addition that should show a noticeable performance improvement when using persistent connections (which is the default pretty much everywhere). Together with #404, this improves benchmarking performance from ~11000 requests per second to ~22000 requests per second on a single CPU core.

$ docker run -it --rm --net=host jordi/ab -n100000 -c80 -k http://localhost:8080/
…
Requests per second:    22327.67 [#/sec] (mean)
…

This has limited support for pipelining (which is rarely used in practice), I'll look into this in a follow-up PR because this requires a considerable refactoring first. If you do not wish to use persistent connections, you can always use a middleware like this:

$server = new Server(
    $loop,
    function (ServerRequestInterface $request, callable $next) {
        // disable keep-alive (not usually recommended)
        return $next($request)->withHeader('Connection', 'close');
    },
    function (ServerRequestInterface $request) {
        return new Response(
            200,
            []
            'Hello world!'
        );
    }
);

Resolves #39

[mmoreram]

What a great achievement. Congratulations and thanks for your work.

[acasademont]

Thank you @clue ! How about keep-alive timeouts? If the client just disappears wouldn't the server socket be kept alive forever, potentially causing socket starvation? This nginx blog post summarizes the pros and cons of using keep alive quite well.

https://www.nginx.com/blog/http-keepalives-and-web-performance/

and

http://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_requests

Should this time-out be configurable somehow? Maybe we should discuss this in a separate issue?

[WyriHaximus]

@acasademont Not sure if @clue already has a branch with this. But essentially exactly that is why we added the loop as a constructor argument for the server, so we can implement a timeout once this landed. It's also more about closing idle connections after X seconds.

[acasademont]

Yep! It's basically a way to "garbage-collect" idle browser connections. I'd like to run some tests to see how it behaves, especially where reactphp sits behind CloudFront or GCLB, which support Keep Alive connections to backends.

[acasademont]

Seems like GCLB will close the connection after 10 minutes of idling. I will deploy now this new release and see how it goes :D

https://cloud.google.com/load-balancing/docs/https#timeouts_and_retries

[acasademont]

There seems to be a problem with php-pm not being able to fully work with keepalive connections. Opened php-pm/php-pm#538 for reference, I will keep digging

[clue]

Thank you @clue ! How about keep-alive timeouts? If the client just disappears wouldn't the server socket be kept alive forever, potentially causing socket starvation? This nginx blog post summarizes the pros and cons of using keep alive quite well.

https://www.nginx.com/blog/http-keepalives-and-web-performance/

and

http://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_requests

Should this time-out be configurable somehow? Maybe we should discuss this in a separate issue?

@acasademont Thanks for the feedback! As a starting point, this PR indeed intentionally doesn't implement a keep-alive timeout. This is done to mimic initial connections which don't implement a timeout either. The documentation also suggests running this behind a reverse proxy server, so this server would usually take care of this timeout handling anyway.

That said, I agree that the next step would be to implement proper timeouts as briefly discussed in #194 and as started by @WyriHaximus in #425. Let's continue this discussion there? 👍