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

[Sergey Kandaurov]

> On 27 Jan 2023, at 08:01, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> Hello!
> 
> [..]
> Overall, after looking into logs and tcpdump you've provided I 
> tend to think that the only working fix would be to introduce 
> c->pipeline flag, and force lingering close if there were any 
> pipelined requests on the connection.
> 
> Below is the patch which implements this approach.  Review and 
> testing appreciated.  It can be used either separately or with the 
> previously provided patch to use posted next events.
> 
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1674790916 -10800
> #      Fri Jan 27 06:41:56 2023 +0300
> # Node ID 784d0fa0b5a0796561642a5a64dc4e9e07592852
> # Parent  4eb1383f6432b034630e6de18739b817f6565c8c
> Lingering close for connections with pipelined requests.
> 
> This is expected to help with clients using pipelining with some constant
> depth, such as apt[1][2].
> 
> When downloading many resources, apt uses pipelining with some constant
> depth, a number of requests in flight[1][2].  This essentially means that

Are links repeated on purpose?

> after receiving a response it sends an additional request to the server,
> and this can result in requests arriving to the server at any time.  Further,
> additional requests are sent one-by-one, and can be easily seen as such
> (neither as pipelined, nor followed by pipelined requests).
> 
> The only safe approach to close such connections (for example, when
> keepalive_requests is reached) is with lingering.  To do so, now nginx
> monitors if pipelining was used on the connection, and if it was, closes
> the connection with lingering.
> 
> [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=973861#10
> [2] https://mailman.nginx.org/pipermail/nginx-devel/2023-January/ZA2SP5SJU55LHEBCJMFDB2AZVELRLTHI.html
> 
> diff --git a/src/core/ngx_connection.h b/src/core/ngx_connection.h
> --- a/src/core/ngx_connection.h
> +++ b/src/core/ngx_connection.h
> @@ -172,6 +172,7 @@ struct ngx_connection_s {
>     unsigned            timedout:1;
>     unsigned            error:1;
>     unsigned            destroyed:1;
> +    unsigned            pipeline:1;
> 
>     unsigned            idle:1;
>     unsigned            reusable:1;
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -2753,7 +2753,8 @@ ngx_http_finalize_connection(ngx_http_re
>         || (clcf->lingering_close == NGX_HTTP_LINGERING_ON
>             && (r->lingering_close
>                 || r->header_in->pos < r->header_in->last
> -                || r->connection->read->ready)))
> +                || r->connection->read->ready
> +                || r->connection->pipeline)))
>     {
>         ngx_http_set_lingering_close(r->connection);
>         return;
> @@ -3123,6 +3124,7 @@ ngx_http_set_keepalive(ngx_http_request_
> 
>         c->sent = 0;
>         c->destroyed = 0;
> +        c->pipeline = 1;
> 
>         if (rev->timer_set) {
>             ngx_del_timer(rev);
> 

Looks good to me.

Further, as request pipelining implementation status is quite limited
(often disabled by default or unimplemented, notably in browsers),
I believe the change is fine to not cause noticeable legit resource usage.
Still, old behaviour can be reverted with "lingering_close off;".

-- 
Sergey Kandaurov