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

[Maxim Dounin]

Hello!

On Tue, Mar 08, 2016 at 09:40:41PM -0500, Justin Li wrote:

> Another mistake in the message... that proxy_cache_bypass should be
> proxy_no_cache.

Feel free to submit an updated patch.  A bit more detailed commit 
message describing how to reproduce the problem will be also a 
good idea.

> I'm not sure I understand exactly how this was functioning beforehand. In
> the case of a cacheable response, if downstream_error is 1, then
> won't ngx_event_pipe_drain_chains be called? Why does that case not have
> the same issues with busy buffers as previously described?

If p->downstream_error is set from the very start there is no 
harm, as there are no busy buffers.  And the same is true if 
there is a real error and we don't need to send anything - these 
buffers won't be used, so they can be drained (other code have to 
be careful to don't trigger false alerts though).

The problem with your previous patch is that you've tried to drain 
buffers in the p->downstream_done case, when p->busy buffers are 
important and can contain real data (already passed to output 
filters though not yet sent to the client).

> On Tue, Mar 8, 2016 at 9:29 PM, Justin Li <jli.justinli at gmail.com> wrote:
> 
> > # HG changeset patch
> > # User Justin Li <jli.justinli at gmail.com>
> > # Date 1457490530 18000
> > #      Tue Mar 08 21:28:50 2016 -0500
> > # Node ID e9f916f271b86e446c6f1de371832432a9f2a66d
> > # Parent  c5f81dcf97a79576138917501c9a6cc6f53ee930
> > Upstream: avoid closing client conn when no response body needed

Please use trailing dot.
Please use full words, just "conn" looks wrong.

(But please make sure to keep 1st line under 67 characters.) 


> >
> > If proxy_cache is enabled, and proxy_cache_bypass tests true, it was
> > previously
> > possible for the client connection to be closed after a 304. The fix is to
> > recheck r->header_only after the final cacheability is determined, and end
> > the
> > request if no longer cacheable.
> >
> > diff -r c5f81dcf97a7 -r e9f916f271b8 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      Tue Mar 08 21:28:50 2016 -0500
> > @@ -2861,6 +2861,12 @@ ngx_http_upstream_send_response(ngx_http
> >          ngx_http_file_cache_free(r->cache, u->pipe->temp_file);
> >      }
> >
> > +    if (r->header_only && !u->cacheable && !u->store) {
> > +        u->pipe->downstream_error = 0;

There is no need to reset p->downstream_error flag, as far as I 
see.

> > +        ngx_http_upstream_finalize_request(r, u, rc);

I would rather avoid using "rc" here, as this code is far away 
from the place where rc is set.  It should be better to use just 
0.

> > +        return;
> > +    }
> > +
> >  #endif
> >
> >      p = u->pipe;
> >

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