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

[Maxim Dounin]

Hello!

On Thu, Feb 02, 2023 at 06:53:20PM +0400, Sergey Kandaurov wrote:

> > 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?

No, thanks for catching.  Removed the second pair.

> > 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;".

Well, previous behaviour is a bit better than "lingering_close 
off;", but indeed, the idea behind the patch is that pipelining is 
mostly unused, and hence the change shouldn't affect most of the 
workloads at all.  On the other hand, if pipelining is used, there is 
good chance that lingering close is actually needed.  Further, 
since 1.19.7 nginx is able to force-close connections in lingering 
close state when free worker connections are exhausted, so some 
additional lingering shouldn't be problematic.

Thanks for the review, pushed to http://mdounin.ru/hg/nginx.

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