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. 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?

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.

> 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:

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.

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.

In conclusion, your latest change does not even allow NULL flushing
cases that perfectly *qualify* your own definition of the requirements
"last buffer was already sent or there are busy buffers" because you
explicitly prohibit flushing when there actually *is* busy bufs in
ngx_http_write_filter_module, and just in ngx_gzip and ngx_gunzip (but
not in any other output filter modules).

Just my 2 cents :)

Regards,
-agentzh

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