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