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

[Maxim Dounin]

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).

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

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