[PATCH] HTTP: trigger lingering close when keepalive connection will be closed

[Maxim Dounin]

Hello!

On Wed, Jan 18, 2023 at 11:28:52PM +0800, Miao Wang wrote:

> # HG changeset patch
> # User Miao Wang <shankerwangmiao at gmail.com>
> # Date 1674055068 -28800
> #      Wed Jan 18 23:17:48 2023 +0800
> # Node ID 73aa64bd29f3dec9e43e97560d6b5a07cdf40063
> # Parent  07b0bee87f32be91a33210bc06973e07c4c1dac9
> HTTP: trigger lingering close when keepalive connection will be closed
> 
> When finalizing a request, if the request is not keepalive but
> its connection has served more than one request, then the connection
> has been a keepalive connection previously and this connection will
> be closed after this response. In this condition, it is likely that
> there are pipelined requests following this request, which we should
> ignore. As a result, lingering close is necessary in this case.
> 
> Without this patch, nginx (with its default configuration) will send
> out TCP RST when there are more pipelined requests. The symptom is
> obvious when nginx is serving a debian repository and apt is
> downloading massive of packages. See [1]. It becomes more obvious
> when `keepalive_requests` is lower or nginx is under a relative
> higher load, and it disappears when specifying
> `lingering_close always`.
> 
> [1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=973861#10
> 
> diff -r 07b0bee87f32 -r 73aa64bd29f3 src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c	Wed Dec 21 14:53:27 2022 +0300
> +++ b/src/http/ngx_http_request.c	Wed Jan 18 23:17:48 2023 +0800
> @@ -2749,6 +2749,10 @@
>          return;
>      }
>  
> +    if (!r->keepalive && r->connection->requests > 1) {
> +        r->lingering_close = 1;
> +    }
> +
>      if (clcf->lingering_close == NGX_HTTP_LINGERING_ALWAYS
>          || (clcf->lingering_close == NGX_HTTP_LINGERING_ON
>              && (r->lingering_close

Thanks for the patch and the link to the Debian bug report.

Lingering close implies noticeable additional resource usage: even 
if nothing happens on the connection, it will be kept open for 
lingering_timeout, which is 5 seconds by default.  Given that 
pipelining is not used by most of the clients, forcing lingering 
close for all clients which are using keepalive does not look like 
a good solution to me.

In general, nginx tries hard to determine if any additional data 
are expected on the connection, and uses lingering close if there 
is a good chance there will be some, but avoids lingering close by 
default if additional data are unlikely.  If this logic does not 
work for some reason, lingering close can be explicitly requested 
with "lingering_close always;".

In particular, note the "r->header_in->pos < r->header_in->last" 
and "r->connection->read->ready" checks - these are expected to 
catch connections with additional pipelined requests (see revision
3981:77604e9a1ed8).  And from the log provided in the report it 
looks like it works most of the time - there are more than 6k HTTP 
requests, and 60+ connections.  But sometimes it fails - there are 
two RST errors logged (and one "Undetermined Error", which looks 
like a bug in apt, but might be related).

It looks like when apt is downloading many resources, it does not 
send all the requests at once (or in batches), but instead tries 
to maintain a constant "depth", a number of pipelined requests in 
flight.  This essentially means that after reading of a response 
it sends an additional request.

I see at least two possible cases which can result in nginx not 
using lingering close with such a load:

1.  If a response where keepalive_requests is reached happens to 
be the last request in the r->header_in buffer (so the 
"r->header_in->pos < r->header_in->last" won't be effective), and 
there is a chance that nginx wasn't yet got an event from kernel 
about additional data (and therefore "r->connection->read->ready" 
will not be set).  As such, nginx won't use lingering close, and 
might close connection with unread data in the socket buffer, 
resulting in RST.

2.  Similarly, if nginx happens to be faster than apt, and socket 
buffers are large enough, it might sent all the responses, 
including the last one with "Connection: close", and close the 
connection (since there are no pending pipelined requests at the 
moment) even before an additional request is sent by apt.  When 
later apt will send an additional request after reading some of 
the responses, it will send the request to already closed 
connection, again resulting in RST.

It would be interesting to see more details, such as tcpdump 
and/or nginx debug logs, to find out what actually goes on here.

Overall, given how apt uses pipelining, I tend to think that at 
least (2) is unavoidable and can happen with certain sizes of the 
responses.

A good enough solution might be check for r->pipeline, which is 
set by nginx as long as it reads a pipelined request.  It might 
not be enough though, since r->pipeline is only set for requests 
seen by nginx as pipelined, which might not be true for the last 
request.

A more complete solution might be to introduce something like 
c->pipeline flag and use lingering close if any pipelined requests 
were seen on the connection.

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