Open minac opened 3 years ago
Removing the two gzip lines from here fixes it for me: https://github.com/google/ngx_brotli/blob/9aec15e2aa6feea2113119ba06460af70ab3ea62/static/ngx_http_brotli_static_module.c#L135-L142
As far as I understand it:
.br
file checked. If it doesn't exist, continue.@eustas can the static module be changed to move those two lines down to somewhere like here? https://github.com/google/ngx_brotli/blob/9aec15e2aa6feea2113119ba06460af70ab3ea62/static/ngx_http_brotli_static_module.c#L268 At that stage the .br file has been checked to exist, has been read, and the handler is ready to respond.
I tested with your repo and confirmed this should fix it.
Just for reference, how the gzip_ok/tested flags are used in nginx: https://github.com/nginx/nginx/blob/ca9bf16f09ef2b0755bfe880c68dc71b9c46f879/src/http/modules/ngx_http_gzip_filter_module.c#L256-L263 If not tested, run ngx_http_gzip_ok (this checks for A-E: gzip, and then sets tested/ok). If already tested, check if gzip is ok. If false, skip. So ngx_brotli sets gzip_tested to 1 (skipping the gzip check) and gzip_ok to 0 (preventing gzip from running).
Interestingly -- gzip_static
does not use gzip_tested/ok. I see you have a static .gz file in your repo. If you gzip_static on;
and remove the brotli file, the static gz file will be served even without this brotli patch.
https://github.com/nginx/nginx/blob/ca9bf16f09ef2b0755bfe880c68dc71b9c46f879/src/http/modules/ngx_http_gzip_static_module.c#L112-L113
It does the header check only.
Hi @adamburgess! Thanks for the deep dive here. I agree with everything you wrote. To confirm, the problem is not with static gzipped files, but with dynamic gzipping. I hope your fix can go into the module so we can start using it! Thanks 🙏
It is strange that the problem described above does not exist after testing multiple times.
I just re-ran the repro at https://github.com/minac/nginx_brotli_test and the problem definitely still exists...
Context In my organization, we have projects that have some statically compressed assets (with gzip and brotli versions), and some not compressed. We recently decided to add Brotli configuration to our nginx to serve those statically compressed assets for clients that ask for them. At this point, we do not want dynamic Brotli compression until we understand the CPU cost associated with it.
Statically compressed files work well Testing nginx with the
ngx_brotli
module statically compiled and a basic Brotli configuration I can see that calling the assets with uncompressed, gzipped, and brotli'ed versions on disk and the headerAccept-Encoding: gzip,deflate,br
works as expected.Dynamic Brotli configuration woks as expected If I enable Brotli configuration to dynamically compress assets, it also works as expected.
Uncompressed files do not get gzipped if brotli is in the
Accept-Encoding
header Assets that do NOT have compressed versions on disk are the problem. When called with the headerAccept-Encoding: gzip,deflate
, the assets come gzipped. But with the headerAccept-Encoding: gzip,deflate,br
they come completely uncompressed. It's as if the Brotli header disables gzip.I created a little repository with a docker image that includes nginx and this module and added the most basic configuration and a random set of assets to test this: https://github.com/minac/nginx_brotli_test.
Questions
Thank you.