Nginx Brunzip

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Nginx Brunzip

Mathew Heard
Hi all,

I'm the maintainer of an open source module ngx_brunzip_module (https://github.com/splitice/ngx_brunzip_module/). Effectively the same as the gunzip module (and based off that source) but with Brotli.

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?

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?

Regards,
Mathew


_______________________________________________
nginx mailing list
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx
Reply | Threaded
Open this post in threaded view
|

Re: 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/
_______________________________________________
nginx mailing list
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx
Reply | Threaded
Open this post in threaded view
|

Re: Nginx Brunzip

Mathew Heard-2
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 <[hidden email]> 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
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx

_______________________________________________
nginx mailing list
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx
Reply | Threaded
Open this post in threaded view
|

Re: Nginx Brunzip

Mathew Heard-2
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx

_______________________________________________
nginx mailing list
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx
Reply | Threaded
Open this post in threaded view
|

Re: Nginx Brunzip

Mathew Heard-2
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx

_______________________________________________
nginx mailing list
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx
Reply | Threaded
Open this post in threaded view
|

Re: Nginx Brunzip

Mathew Heard-2
Disregard * 2 I understand now.

On Thu, 16 Apr 2020 at 13:14, Mathew Heard <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx

_______________________________________________
nginx mailing list
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx
Reply | Threaded
Open this post in threaded view
|

Re: Nginx Brunzip

Mathew Heard-2
In reply to this post by Maxim Dounin
Maxim,

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

Is beause of https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L513 correct? Because FLUSH_FLUSH always resets state to FLUSH_NOFLUSH.

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

I'll continue working to figure out the filter system.

Regards,
Mathew

On Thu, 16 Apr 2020 at 03:12, Maxim Dounin <[hidden email]> 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
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx

_______________________________________________
nginx mailing list
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx
Reply | Threaded
Open this post in threaded view
|

Re: Nginx Brunzip

Maxim Dounin
Hello!

On Thu, Apr 16, 2020 at 03:48:27PM +1000, Mathew Heard wrote:

> Maxim,
>
> > 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.
>
> Is beause of
> https://github.com/splitice/ngx_brunzip_module/blob/master/ngx_http_brunzip_filter_module.c#L513
> correct?
> Because FLUSH_FLUSH always resets state to FLUSH_NOFLUSH.

No.  Because of the "ctx->flush != FLUSH_NOFLUSH" condition at the
very start of the same function.  In the particular place the
ctx->field is guaranteed to be set to FLUSH_NOFLUSH, and there is
no need to set it again.

As you can see in the original code, there is no assignment.  
Instead, it simply states that "ctx->flush == Z_NO_FLUSH" in a
comment.

(http://hg.nginx.org/nginx/file/3a860f22c879/src/http/modules/ngx_http_gunzip_filter_module.c#l365)

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx mailing list
[hidden email]
http://mailman.nginx.org/mailman/listinfo/nginx