[PATCH] Proxy: set u->keepalive also if the whole body has already been read.

[Jan Prachař]

On Po, 2020-09-21 at 22:07 +0300, Maxim Dounin wrote:
> Hello!
> 
> On Mon, Sep 21, 2020 at 08:09:29PM +0200, Jan Prachař wrote:
> 
> > # HG changeset patch
> > # User Jan Prachař <jan.prachar at gmail.com>
> > # Date 1600710589 -7200
> > #      Mon Sep 21 19:49:49 2020 +0200
> > # Node ID f211684e1acee34eabfdd9dd39283bcff8dc7087
> > # Parent  052ecc68d35038b1b4adde12efe6249a92055f09
> > Proxy: set u->keepalive also if the whole body has already been
> > read.
> > 
> > HTTP redirection responses often contain a small few hundred bytes
> > body. This
> > allows to left keep alive connection open, if the body fits into
> > the
> > upstream
> > buffer.
> 
> HTTP redirection responses are expected to be read from the 
> connection and returned to the client.  The code to do it will 
> properly read the response body, check it, and will set 
> u->keepalive as appropriate.
> 
> Most likely, you are trying to cover a specific case when 
> redirections responses are intercepted using 
> proxy_intercept_errors and error_page, as recently reported in 
> ticket #2033 (https://trac.nginx.org/nginx/ticket/2033).

Hello, 

Yes. This is one of the use-case. I am also trying to cover X-Accel-
Redirect header in a response from upstream.


> > diff -r 052ecc68d350 -r f211684e1ace
> > src/http/modules/ngx_http_proxy_module.c
> > --- a/src/http/modules/ngx_http_proxy_module.c	Wed Sep 16
> > 18:26:25
> > 2020 +0300
> > +++ b/src/http/modules/ngx_http_proxy_module.c	Mon Sep 21
> > 19:49:49
> > 2020 +0200
> > @@ -1905,7 +1905,8 @@
> >              }
> >  
> >              /*
> > -             * set u->keepalive if response has no body; this
> > allows
> > to keep
> > +             * set u->keepalive if response has no body or if the
> > whole body
> > +             * has been already read to u->buffer; this allows to
> > keep
> >               * connections alive in case of r->header_only or X-
> > Accel-
> > Redirect
> >               */
> >  
> > @@ -1915,7 +1916,7 @@
> >                  || u->headers_in.status_n == NGX_HTTP_NOT_MODIFIED
> >                  || ctx->head
> >                  || (!u->headers_in.chunked
> > -                    && u->headers_in.content_length_n == 0))
> > +                    && u->headers_in.content_length_n <= u-
> > > buffer.last-u->buffer.pos))
> >              {
> >                  u->keepalive = !u->headers_in.connection_close;
> >              }
> 
> I can't say I like the idea of connection cacheability to depend 
> on timing and buffer size factors.
> 
> Further, the "<=" comparison looks wrong: we shouldn't cache 
> connections if there are more data than expected.

Thank you, that's a good point.


Best, Jan Prachař