Welcome! Log In Create A New Profile

Advanced

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

Maxim Dounin
February 02, 2023 08:52PM
Hello!

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

> > On 27 Jan 2023, at 08:01, Maxim Dounin <mdounin@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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

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

shankerwangmiao 679 January 18, 2023 10:30AM

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

Maxim Dounin 199 January 22, 2023 11:06PM

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

shankerwangmiao 167 January 23, 2023 06:02AM

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

Maxim Dounin 122 January 24, 2023 09:18PM

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

shankerwangmiao 202 January 24, 2023 11:50PM

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

Maxim Dounin 127 January 26, 2023 11:02PM

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

shankerwangmiao 141 January 27, 2023 07:12AM

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

Maxim Dounin 127 January 27, 2023 04:58PM

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

Sergey Kandaurov 160 February 02, 2023 09:54AM

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

Maxim Dounin 269 February 02, 2023 08:52PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 262
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready