[PATCH] HTTP: keepalive_graceful_close support

[Maxim Dounin]

Hello!

On Fri, Dec 17, 2021 at 01:59:40PM +0800, Youlin Feng wrote:

> # HG changeset patch
> # User Youlin Feng <fengyoulin at live.com>
> # Date 1639718067 -28800
> #      Fri Dec 17 13:14:27 2021 +0800
> # Node ID 54db7272bb0c9040eaf657f92bb02c9147d927e6
> # Parent  a7a77549265ef46f1f0fdb3897f4beabf9e09c40
> HTTP: keepalive_graceful_close support.
> 
> Previously, keepalived connections will be suddenly closed during worker
> process exiting, without notification to the client. The client may be
> sending a request when the connection is closed, resulting in an error.
> 
> With "keepalive_graceful_close on;", the client will receive an
> "Connection: close" response header in the last request, then the
> connection can be closed gracefully.

Thanks for the patch.

First of all, you may want to clarify which problem you are trying 
to solve.  Note that clients are expected to be prepared for a 
connection close by the server, and retry requests as appropriate.  
If the client cannot handle this, probably there is a room for 
improvement in the client.  Further, there is an unavoidable race, 
and a client which cannot handle connection close is likely to see 
errors regardless of server behaviour.  See ticket #1022 
(https://trac.nginx.org/nginx/ticket/1022) for detailed 
explanation.

What probably nginx should do here is to disable keepalive and 
avoid sending "Connection: keep-alive" if it already knows that 
keepalive cannot be used when sending response headers.  This 
should reduce unneeded request failures and retries at no 
additional cost.

Patch:

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1640613750 -10800
#      Mon Dec 27 17:02:30 2021 +0300
# Node ID 0430ee0554a19281abcc074981702ac201f2922e
# Parent  4fa260f60bac03037017a33672da1ac75ac31408
Avoid sending "Connection: keep-alive" when shutting down.

When a worker process is shutting down, keepalive is not used: this is checked
before the ngx_http_set_keepalive() call in ngx_http_finalize_connection().
Yet the "Connection: keep-alive" header was still sent, even if we know that
the worker process is shutting down, potentially resulting in additional
requests being sent to the connection which is going to be closed anyway.
While clients are expected to be able to handle asynchronous close events
(see ticket #1022), it is certainly possible to send the "Connection: close"
header instead, informing the client that the connection is going to be closed
and potentially saving some unneeded work.

With this change, we additionally check for worker process shutdown just
before sending response headers, and disable keepalive accordingly.

diff --git a/src/http/ngx_http_header_filter_module.c b/src/http/ngx_http_header_filter_module.c
--- a/src/http/ngx_http_header_filter_module.c
+++ b/src/http/ngx_http_header_filter_module.c
@@ -197,6 +197,10 @@ ngx_http_header_filter(ngx_http_request_
         }
     }
 
+    if (r->keepalive && (ngx_terminate || ngx_exiting)) {
+        r->keepalive = 0;
+    }
+
     len = sizeof("HTTP/1.x ") - 1 + sizeof(CRLF) - 1
           /* the end of the header */
           + sizeof(CRLF) - 1;

-- 
Maxim Dounin
http://mdounin.ru/