Nginx Brunzip

[Maxim Dounin]

Hello!

On Wed, Apr 15, 2020 at 10:21:09AM +1000, Mathew Heard wrote:

> Hi all,
> 
> I'm the maintainer of an open source module ngx_brunzip_module (
> https://github.com/splitice/ngx_brunzip_module/
> <https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c>).
> Effectively the same as the gunzip module (and based off that source) but
> with Brotli.

If it's based on the gunzip module code, you are violating 
copyrights on the code, including mine.  Please fix.

> I've been scratching my head for 2 days regarding some high CPU usage
> within the chain code. It appears that some spinning is possible. I must
> admit I only have a basic understanding of the filter chain in nginx (still
> gaining experience).
> 
> 1. I was wondering if someone could take a look at the code and give me
> some pointers?

Likely unrelated, but "ctx->input" and "ctx->output" are 
meaningless and never used.

Likely unrealted, but "ctx->flush = FLUSH_NOFLUSH" at
https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L393 
is meaningless.

> 2. Also I've added some code to prevent further filling of mostly full
> buffers (as it appears brotli is quite expensive to start) at
> https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L408
> is
> this valid? How does nginx determine when backpressure from full output
> chains is relieved? Is there any in-depth documentation of the filter chain
> architecture?

No, it's not valid, and your code will throw away such mostly 
filled output buffers without linking them to the output chain as 
normally happens in ngx_http_brunzip_filter_inflate() at

https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L485

Further, the test in question looks incorrect, as it doesn't 
take into account the edge case when amount of the output returned 
by BrotliDecoderDecompressStream() exactly matches the output 
buffer size, so ctx->available_out is 0, but rc is not 
BROTLI_DECODER_RESULT_NEEDS_MORE_OUTPUT.

As for the documentation, it looks like you are looking for the 
documentation of the code in the module.  You may want to re-read 
it to understand (and fix the copyright as requested above to 
admit that you aren't the one who wrote most of the code).  Some 
basics about buffers and chains can be found here:

http://nginx.org/en/docs/dev/development_guide.html#buffer

Some simplified example of a code to work with buffers reuse as 
used by the module can be found here:

http://nginx.org/en/docs/dev/development_guide.html#http_body_buffers_reuse

Other chapters, such as "Code style", might be helpful as well.  
Also, don't hesitate to look into the code of the functions you 
are using, it often helps.

-- 
Maxim Dounin
http://mdounin.ru/