Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: compression priority #1950

Merged
merged 6 commits into from
Feb 12, 2025
Merged

fix: compression priority #1950

merged 6 commits into from
Feb 12, 2025

Conversation

inetol
Copy link
Contributor

@inetol inetol commented Feb 9, 2025

Before this PR, fasthttp would never use zstd if clients sent their Accept-Encoding header including gzip because zstd was the last option and would only be used if zstd was the only encoding available in Accept-Encoding header. This is really an issue when serving precompressed sidecars of zstd and gzip where the server preferred the latter.

The following order has been established; first brotli, then zstd, and finally gzip. It takes into account whether CompressBrotli and the new CompressZstd option are enabled to select which compression to use.

It also fixes the missing Vary header on compressed assets.

There is also an issue that has been revealed with the rework of these tests that should be addressed in a separate PR

@erikdubbelboer
Copy link
Collaborator

Nice, I like the change. It makes sense to try better algorithms before falling back to gzip.

Can you have a look are the linting issues and failing tests?

@inetol
Copy link
Contributor Author

inetol commented Feb 10, 2025

Everything should be ready now, the failed tests that appear is a specific Windows flaky thing?

@erikdubbelboer erikdubbelboer merged commit 8e25db0 into valyala:master Feb 12, 2025
13 of 15 checks passed
@erikdubbelboer
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants