[PATCH] Be consistent with keepalive during graceful shutdown

[Maxim Dounin]

Hello!

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

> # HG changeset patch
> # User Yuchen Wu <yuchen at 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/