[PATCH]HTTP/2 connection not properly closing during graceful shutdown

[Roman Arutyunyan]

Hi,

On Tue, Apr 23, 2024 at 06:02:08PM +0800, Kasei Wang wrote:
> Hello,
> 
> I found that there is a slight probability of HTTP/2 connections not
> properly closing during graceful shutdown, leading to worker processes
> in shutting down state remaining stuck for an extended period. After
> investigation, the issue appears to stem from the following:
> 
> 1. worker processes in shutting down state use
> ngx_close_idle_connections to close all idle connections, including
> HTTP/2 connections.
> 2. For HTTP/2 connections, c->idle is set to true in ngx_http_v2_init.
> According to the explanation in
> https://hg.nginx.org/nginx/rev/5e95b9fb33b7, GOAWAY should be sent to
> all HTTP/2 connections.
> 3. There might be a time gap between ngx_event_accept and
> ngx_http_v2_init. For TLS connections, ngx_http_v2_init will be
> executed after ALPN received, and for plaintext http2 connections,
> ngx_http_v2_init will be executed after parsing the http2 preface. If
> ngx_close_idle_connections is executed between ngx_event_accept and
> ngx_http_v2_init, there's a possibility that c->idle of some
> connections is set to true AFTER ngx_close_idle_connections, causing
> those connections to not enter the GOAWAY process and leading to the
> aforementioned problem.

You're right, the problem does exist.

> To verify this, I've written a simple HTTP/2 client. This program will
> wait 15 seconds after TCP connection establishment before starting to
> send data. The purpose of sleep is to to raise the probability of
> encountering the issue. You can reproduce the problem by executing
> "nginx -s reload" during this 15-second wait. If you're interested,
> you can try my test program
> (<https://github.com/kaseiwang/http2-grace-shutdown-test>) to
> reproduce the issue.
> 
> I believe the simplest solution is to set c->close based on
> ngx_exiting in ngx_http_v2_init. In this way, the connection will send
> GOAWAY in ngx_http_v2_read_handler.

While setting c->close does the trick, I personally don't like abusing its
semantics.  The flag is used by nginx core to signal the application layer
that the connection is being closed.  Using it within the application layer
is not good, even though this does happen sometimes.

> Please confirm if this issue exists, review my analysis and the patch
> if possible. Thank you very much.
> 
> # HG changeset patch
> # User Kasei Wang <kasei at kasei.im>
> # Date 1713863474 -28800
> # Tue Apr 23 17:11:14 2024 +0800
> # Branch help
> # Node ID e5ba8996973cdd1edd7591a28ee1c145af86c556
> # Parent 8618e4d900cc71082fbe7dc72af087937d64faf5
> HTTP/2: close http2 connections created during graceful shutdown.
> 
> diff -r 8618e4d900cc -r e5ba8996973c src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c Tue Apr 16 18:27:50 2024 +0400
> +++ b/src/http/v2/ngx_http_v2.c Tue Apr 23 17:11:14 2024 +0800
> @@ -304,6 +304,10 @@
> c->idle = 1;
> ngx_reusable_connection(c, 0);
> 
> + if (ngx_exiting) {
> + c->close = 1;
> + }
> +
> if (c->buffer) {
> p = c->buffer->pos;
> end = c->buffer->last;

This solution allows reading and handling the data from c->buffer prior to
closing the connection, which we probably don't need.  I think it'd be better
to just call ngx_http_v2_finalize_connection() at the right moment in
ngx_http_v2_init().

--
Roman Arutyunyan