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/

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel