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

[Justin Li]

Apologies, I referenced a git commit hash in the first sentence, the
corresponding mercurial commit is 5746:35990c69.

On Mon, Mar 7, 2016 at 1:16 PM, Justin Li <jli.justinli at gmail.com> 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
>
> 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.
>
> 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.
>
> 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.
>
> 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 @@
>      flushed = 0;
>
>      for ( ;; ) {
> -        if (p->downstream_error) {
> +        if (p->downstream_error || p->downstream_done) {
>              return ngx_event_pipe_drain_chains(p);
>          }
>
> 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;
> +    }
> +
>  #if (NGX_HAVE_KQUEUE)
>
>      if (ngx_event_flags & NGX_USE_KQUEUE_EVENT) {
> @@ -2697,7 +2701,7 @@
>              return;
>          }
>
> -        u->pipe->downstream_error = 1;
> +        u->pipe->downstream_done = 1;
>      }
>
>      if (r->request_body && r->request_body->temp_file) {
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20160307/f0777eb6/attachment.html>