[PATCH] Upstream: avoid closing connection when no client body needed

[Maxim Dounin]

Hello!

On Mon, Mar 07, 2016 at 01:16:13PM -0500, Justin Li wrote:

> # HG changeset patch
> # User Justin Li <jli.justinli at gmail.com>
> # Date 1457369412 18000
> #      Mon Mar 07 11:50:12 2016 -0500
> # Node ID e84a844f3c2634488285fb9677d3254a032440a2
> # Parent  c5f81dcf97a79576138917501c9a6cc6f53ee930
> Upstream: avoid closing connection when no client body needed

The term "client body" is used to refer client request body, as in 
client_body_buffer_size (http://nginx.org/r/client_body_buffer_size). 
The patch seems to be about a response body.

> Despite 4a75e1a6, it was previously possible for the connection to be closed in
> cases where a response body from the upstream should not be sent to the client.
> This is due to the conditional in ngx_http_upstream_process_request that may
> call ngx_http_upstream_finalize_request in certain cases.

There are two connections: one with the client, another one with 
the upstream server.  You may want to be more explicit to avoid 
confusion.

> Example configuration:
> 
> proxy_cache foo;
> proxy_cache_bypass 1;
> proxy_no_cache 1;
> 
> If a client sends If-None-Match, and the upstream server returns 200 with a
> matching ETag, no body should be returned to the client. At the start of
> ngx_http_upstream_send_response proxy_no_cache is not yet tested, thus cacheable
> is still 1 and downstream_error is set.
> 
> However, by the time the downstream_error check is done in process_request,
> proxy_no_cache has been tested and cacheable is set to 0. The client connection
> is then closed, regardless of keepalive.

So you are trying to optimize a specific case with proxy_no_cache 
in addition to what 35990c69b3ac fixed, right?  It might worth to 
make it clear from the very start.  Better understanding of what 
is fixed might also help to develop better fix, see below.

> The fix is to avoid using the p->downstream_error flag, and instead repurpose
> p->downstream_done to indicate the event pipe should be drained. Additionally,
> a check is added for this flag in ngx_http_upstream_check_broken_connection to
> prevent a 499 if the client validly closes the connection after receiving the
> headers.

The fix looks wrong, see below.

> diff -r c5f81dcf97a7 -r e84a844f3c26 src/event/ngx_event_pipe.c
> --- a/src/event/ngx_event_pipe.c	Thu Mar 03 21:14:19 2016 +0300
> +++ b/src/event/ngx_event_pipe.c	Mon Mar 07 11:50:12 2016 -0500
> @@ -502,7 +502,7 @@

Please add 

[diff]
showfunc=1

to your ~/.hgrc, this greatly simplifies review process.

>      flushed = 0;
>  
>      for ( ;; ) {
> -        if (p->downstream_error) {
> +        if (p->downstream_error || p->downstream_done) {
>              return ngx_event_pipe_drain_chains(p);
>          }
>  

This is incorrect.  The ngx_event_pipe_drain_chains() call will 
corrupt busy buffers, that is, already sent to output filter 
chain, leading to undefined behaviour.

(See also ticket #918, https://trac.nginx.org/nginx/ticket/918, 
which demonstrates that ngx_event_pipe_drain_chains() isn't 
completely safe even in case of errors.)

> diff -r c5f81dcf97a7 -r e84a844f3c26 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c	Thu Mar 03 21:14:19 2016 +0300
> +++ b/src/http/ngx_http_upstream.c	Mon Mar 07 11:50:12 2016 -0500
> @@ -1172,6 +1172,10 @@
>      }
>  #endif
>  
> +    if (u->pipe->downstream_done) {
> +        return;
> +    }
> +

This is incorrect as well, as it will fail to remove appropriate 
events and will result in CPU hog when using level-triggered events.

It is also believed that if the response is not cacheable there 
are no reasons why we should keep reading from the upstream 
connection.  Normally we just close the upstream connection once we 
sent headers to the client and found that we don't need to sent 
anything else (r->header_only is set).

What about explicitly handling the proxy_no_cache case right 
after the proxy_no_cache cache check, much like it is already done 
with cache object (see ngx_http_file_cache_free() call)?

(Additionally, the code between the no_cache test and the 
ngx_http_file_cache_free() call also shows that lack of 
proxy_cache_valid and/or explicit cache validity headers can lead 
to the same behaviour.)

-- 
Maxim Dounin
http://nginx.org/