Nginx Brunzip

[Mathew Heard]

Disregard * 2 I understand now.

On Thu, 16 Apr 2020 at 13:14, Mathew Heard <me at mheard.com> wrote:

> Disregard the previous email, there was a typo there.
>
> Maxim,
>
> I'm doing line by line documentation in my module. I hope by doing this I
> will get a good understanding of what is going on. If allowed I intend to
> commit this as a resource for others also intending to learn the body
> filter system (I will however check with this mailing list to see if anyone
> has an issue with my descriptions).
>
> While doing this I noticed that ctx->busy does not appear to ever be true
> in your gunzip module. Am I missing something here?
>
> Regards,
> Mathew
>
>
> On Thu, 16 Apr 2020 at 13:11, Mathew Heard <me at mheard.com> wrote:
>
>> Maxim,
>>
>> I'm doing line by line documentation in my module. I hope by doing this I
>> will get a good understanding of what is going on. If allowed I intend to
>> commit this as a resource for others also intending to learn the body
>> filter system (I will however check with this mailing list to see if anyone
>> has an issue with my descriptions).
>>
>> While doing this I noticed that ctx->flush does not appear to ever be
>> true in your gunzip module. Am I missing something here?
>>
>> Regards,
>> Mathew
>>
>> On Thu, 16 Apr 2020 at 10:49, Mathew Heard <me at mheard.com> wrote:
>>
>>> Maxim,
>>>
>>> Which clause of the 2 clause BSD license am I violating? It's not my
>>> intention to violate any. If need be I will remove this project from
>>> distribution and take it closed source. It would be a shame but if it needs
>>> to be done...
>>>
>>> >> Redistribution and use in source and binary forms, with or without
>>> modification, are permitted provided that the following conditions are met:
>>>
>>> Agreed, distribution with modification.
>>>
>>> >> Redistributions of source code must retain the above copyright
>>> notice, this list of conditions and the following disclaimer.
>>>
>>> https://github.com/splitice/ngx_brunzip_module/blob/master/LICENSE.md
>>>
>>> >>  Redistributions in binary form must reproduce the above copyright notice,
>>> this list of conditions and the following disclaimer in the documentation
>>> and/or other materials provided with the distribution.
>>>
>>> No binary distribution is performed.
>>>
>>> >> [etcetera]
>>>
>>> There is no express intent to apply any warranty to you or Nginx inc.
>>>
>>> I'll add some copyright lines at the top of that file as those should
>>> probably be there, I'm not sure they have any legal implication (copyright
>>> is inheint) but better to give credit for the functions used as a reference.
>>>
>>> As for the assistance I am digesting that now. I see what you mean
>>> though regarding available_out == 0, that's indeed a problem. I'll go
>>> through the path for my < 64 patch too, that was not the intent.
>>>
>>> I'm aware of that documentation, I guess I was hoping that there was
>>> more.
>>>
>>> Regards,
>>> Mathew
>>>
>>> On Thu, 16 Apr 2020 at 03:12, Maxim Dounin <mdounin at mdounin.ru> wrote:
>>>
>>>> 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/
>>>> _______________________________________________
>>>> nginx mailing list
>>>> nginx at nginx.org
>>>> http://mailman.nginx.org/mailman/listinfo/nginx
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx/attachments/20200416/6dccaafd/attachment-0001.htm>