<div dir="ltr">Maxim,<div><br></div><div>> Likely unrealted, but "ctx->flush = FLUSH_NOFLUSH" at</div>> <a href="https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L393" rel="noreferrer" target="_blank">https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L393</a><br>> is meaningless.<div><br></div><div>Is beause of <a href="https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L513">https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L513</a> correct? Because FLUSH_FLUSH always resets state to FLUSH_NOFLUSH.</div><div><br></div><div>I've been working on a heavily commented module. I welcome any feedback on the comments (or the
areas marked with "???" that I am still trying to figure out).<br><br>I'll continue working to figure out the filter system.</div><div><br></div><div>Regards,<br>Mathew</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 16 Apr 2020 at 03:12, Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello!<br>
<br>
On Wed, Apr 15, 2020 at 10:21:09AM +1000, Mathew Heard wrote:<br>
<br>
> Hi all,<br>
> <br>
> I'm the maintainer of an open source module ngx_brunzip_module (<br>
> <a href="https://github.com/splitice/ngx_brunzip_module/" rel="noreferrer" target="_blank">https://github.com/splitice/ngx_brunzip_module/</a><br>
> <<a href="https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c" rel="noreferrer" target="_blank">https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c</a>>).<br>
> Effectively the same as the gunzip module (and based off that source) but<br>
> with Brotli.<br>
<br>
If it's based on the gunzip module code, you are violating <br>
copyrights on the code, including mine. Please fix.<br>
<br>
> I've been scratching my head for 2 days regarding some high CPU usage<br>
> within the chain code. It appears that some spinning is possible. I must<br>
> admit I only have a basic understanding of the filter chain in nginx (still<br>
> gaining experience).<br>
> <br>
> 1. I was wondering if someone could take a look at the code and give me<br>
> some pointers?<br>
<br>
Likely unrelated, but "ctx->input" and "ctx->output" are <br>
meaningless and never used.<br>
<br>
Likely unrealted, but "ctx->flush = FLUSH_NOFLUSH" at<br>
<a href="https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L393" rel="noreferrer" target="_blank">https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L393</a> <br>
is meaningless.<br>
<br>
> 2. Also I've added some code to prevent further filling of mostly full<br>
> buffers (as it appears brotli is quite expensive to start) at<br>
> <a href="https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L408" rel="noreferrer" target="_blank">https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L408</a><br>
> is<br>
> this valid? How does nginx determine when backpressure from full output<br>
> chains is relieved? Is there any in-depth documentation of the filter chain<br>
> architecture?<br>
<br>
No, it's not valid, and your code will throw away such mostly <br>
filled output buffers without linking them to the output chain as <br>
normally happens in ngx_http_brunzip_filter_inflate() at<br>
<br>
<a href="https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L485" rel="noreferrer" target="_blank">https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L485</a><br>
<br>
Further, the test in question looks incorrect, as it doesn't <br>
take into account the edge case when amount of the output returned <br>
by BrotliDecoderDecompressStream() exactly matches the output <br>
buffer size, so ctx->available_out is 0, but rc is not <br>
BROTLI_DECODER_RESULT_NEEDS_MORE_OUTPUT.<br>
<br>
As for the documentation, it looks like you are looking for the <br>
documentation of the code in the module. You may want to re-read <br>
it to understand (and fix the copyright as requested above to <br>
admit that you aren't the one who wrote most of the code). Some <br>
basics about buffers and chains can be found here:<br>
<br>
<a href="http://nginx.org/en/docs/dev/development_guide.html#buffer" rel="noreferrer" target="_blank">http://nginx.org/en/docs/dev/development_guide.html#buffer</a><br>
<br>
Some simplified example of a code to work with buffers reuse as <br>
used by the module can be found here:<br>
<br>
<a href="http://nginx.org/en/docs/dev/development_guide.html#http_body_buffers_reuse" rel="noreferrer" target="_blank">http://nginx.org/en/docs/dev/development_guide.html#http_body_buffers_reuse</a><br>
<br>
Other chapters, such as "Code style", might be helpful as well. <br>
Also, don't hesitate to look into the code of the functions you <br>
are using, it often helps.<br>
<br>
-- <br>
Maxim Dounin<br>
<a href="http://mdounin.ru/" rel="noreferrer" target="_blank">http://mdounin.ru/</a><br>
_______________________________________________<br>
nginx mailing list<br>
<a href="mailto:nginx@nginx.org" target="_blank">nginx@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx" rel="noreferrer" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx</a><br>
</blockquote></div>