Gzip Gunzip: always flush busy bufs when the incoming chain is NULL.

[Maxim Dounin]

Hello!

On Tue, Nov 04, 2014 at 12:42:56PM -0800, Yichun Zhang (agentzh) wrote:

> Hello!
> 
> On Mon, Nov 3, 2014 at 4:54 PM, Maxim Dounin wrote:
> > The commit log in question explains the reason for the change.
> > Work on the gzip stalls problem as fixed by 973fded4f461 clearly
> > showed that just passing NULL chains is wrong unless last buffer
> > was already sent or there are busy buffers.
> 
> AFAIK, this is the output filter caller's responsibility to ensure the
> requirements "last buffer was already sent or there are busy buffers",
> not every individual output body filter.

The problem is that caller's idea about busy buffers isn't the 
same as gzip filter's one.  Caller may call output filter chain 
with NULL because some of its buffers are busy, but since they are 
buffered by gzip filter it would be wrong to call next body filter 
with NULL unconditionally.  So in this case it's gzip filter which 
is responsible to call next body filter properly.

> Almost all the other standard
> nginx output filter modules including but not limited to ngx_addition,
> ngx_sub_filter, ngx_image_filter, ngx_chunked_filter, and
> ngx_xslt_filter. You meant to fix these modules as well?

Addition filter doesn't buffer anything.  Sub filter only calls 
next body filter with NULL if it doesn't buffer anything.  Image 
filter always releases all input buffers and so this shouldn't be 
a problem as long as a caller does the right thing.  Chunked filter 
doesn't buffer anything.  Xslt filter always releases all input 
buffers.

That is, all these modules are fine and there is no need to fix 
them.

> Also, the requirement "there are busy buffers" should not just be busy
> buffers of the output filter module itself, but rather busy buffers in
> all the other filters along the filter chain, include the busy bufs in
> r->out which is managed by ngx_http_write_filter_module at the bottom
> of the filter chain.

Technically yes.  But main purpose of calling output filter chain 
with NULL before the last buffer is sent is to free up some busy 
buffers of a calling module, and checking for the module own 
buffers should be enough - and this is what, e.g., unbuffered 
proxy do.

> > If you see an issue, you may want to share the issue details,
> > to find out how to fix it properly.
> 
> The very issue of nginx 1.7.7 caught by my ngx_lua's test suite is in
> this condition:

So this isn't a real issue, but an artificial test case which 
fails, right?

> 1. the content handler runs out of buffers (i.e., having the maximum
> number of busy buffers), and
> 2. ngx_gzip or ngx_gunzip does not have any busy buffers for itself
> (i.e., no ctx->busy), and
> 3. ngx_http_write_filter_module has pending data for flush to the
> system send buffer, that is, having busy bufs in r->out.
> 
> Upon new write events, I pass NULL to the output filter chain in order
> to flush the busy bufs of ngx_http_write_fiilter_module (in r->out),
> but ngx_gzip and ngx_gunzip quietly swallows it, wasting the write
> events, and leading to request hang. This use case of NULL chain in
> ngx_http_output_filter should considered valid because there are
> indeed busy bufs for ngx_http_write_filter_module (though not
> ngx_gzip/ngx_gunzip itself). Disabling the gzip or gunzip filter makes
> the problem go away immediately.

The questions are:

- How it happened that all content handler's buffers are busy, 
  while there are no busy buffers in gzip?

- How it happened that there are no busy buffers in gzip, but 
  there are buffers in r->out?

- Why calling next filter in gzip with NULL is expected to help 
  here?  It only can free up some gzip filter buffers which are 
  all already free, but not content handler module buffers.

> Regarding the "output chain is empty" alert (generated by
> ngx_http_write_filter_module), it should only happen when there is
> *no* busy bufs and no data in r->out, which is apparently not the case
> explained above.

Sure.  The problem is that the code change you suggest (and that 
was previously done in 1.5.7 / c52a761a2029) triggers alert in 
other valid cases, when there are no data in r->out.

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