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