Hello!

On Thu, Jan 17, 2019 at 11:28:54AM -0800, Yuchen Wu via nginx-devel wrote:

> # HG changeset patch
> # User Yuchen Wu <yuchen@cloudflare.com>
> # Date 1547749157 28800
> # Thu Jan 17 10:19:17 2019 -0800
> # Node ID 8a6290c41b33c664d52d7a472400381e71ecf171
> # Parent 8acaa1161783347106dcaea574837e441e13b540
> Be consistent with keepalive during graceful shutdown
>
> Before, when nginx sends the Connection: Keep-Alive header and then
> nginx decides to shutdown, it will close the connection. Downstream
> may already reuse the connection for another request in the meanwhile.
>
> This change allows one extra use of such a connection to make sure it
> is closed cleanly.
>
> diff -r 8acaa1161783 -r 8a6290c41b33 src/http/ngx_http_header_filter_module.c
> --- a/src/http/ngx_http_header_filter_module.c Thu Dec 27 19:37:34 2018 +0300
> +++ b/src/http/ngx_http_header_filter_module.c Thu Jan 17 10:19:17 2019 -0800
> @@ -187,6 +187,10 @@
> r->header_only = 1;
> }
>
> + if (ngx_terminate || ngx_exiting) {
> + r->keepalive = 0;
> + }
> +
> if (r->headers_out.last_modified_time != -1) {
> if (r->headers_out.status != NGX_HTTP_OK
> && r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT
> diff -r 8acaa1161783 -r 8a6290c41b33 src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c Thu Dec 27 19:37:34 2018 +0300
> +++ b/src/http/ngx_http_request.c Thu Jan 17 10:19:17 2019 -0800
> @@ -2611,9 +2611,7 @@
> r->lingering_close = 1;
> }
>
> - if (!ngx_terminate
> - && !ngx_exiting
> - && r->keepalive
> + if (r->keepalive
> && clcf->keepalive_timeout > 0)
> {
> ngx_http_set_keepalive(r);

Thank you for the patch.
Some notes, in no particular order:

- As per HTTP RFC, clients are expected to be prepared to
connection close even if there is no "Connection: close" header,
https://tools.ietf.org/html/rfc2616#section-8.1.4:

A client, server, or proxy MAY close the transport connection at any
time. For example, a client might have started to send a new request
at the same time that the server has decided to close the "idle"
connection. From the server's point of view, the connection is being
closed while it was idle, but from the client's point of view, a
request is in progress.

This means that clients, servers, and proxies MUST be able to recover
from asynchronous close events. Client software SHOULD reopen the
transport connection and retransmit the aborted sequence of requests
without user interaction so long as the request sequence is
idempotent (see section 9.1.2).

Given this, nginx more or less doesn't care when it closes idle
(keepalive) connections. In particular, it does so when
keepalive_timeout expires (which can be quite low), on graceful
shutdown of worker process, and also when there isn't enough
worker_connections.

- Waiting for another request on graceful shutdown means that old
worker processes will be running for keepalive_timeout time even
if there are no requests in progress. While this might not be
important for configurations where are requests in progress for
significantly longer time, this is an important change for
configurations where normally no in-progress requests and so
configuration reloads are more or less immediate now. This should
be carefully considered before changing the behaviour (if at all).

- The patch suggested introduces inconsistency between handling of
connections which were moved to keepalive before shutdown was
requested, and connections which are moving to keepalive after
this point.

- The patch suggested introduces connections which are in idle
state, yet not closed after a shutdown request. There should be
no such connections, and the fact that such connections are not
closed immediately is a result of an optimization, see
http://hg.nginx.org/nginx/rev/5e6142609e48.

If you want to improve keepalive connection handling on shutdown /
configuration reload, you may want to focus on less intrusive
changes, in particular:

- Reset r->keepalive on shutdown if it is still possible (as
ngx_http_header_filter_module.c part of your patch does).

- Try to process pipelined requests if there are any if nginx
worker is shutting down but wasn't yet able to announce
"Connection: close" to the client.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel