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

Opt-in for FHCRC in gzip compression #2696

Merged
merged 5 commits into from
Oct 30, 2021
Merged

Opt-in for FHCRC in gzip compression #2696

merged 5 commits into from
Oct 30, 2021

Conversation

AL333Z
Copy link
Contributor

@AL333Z AL333Z commented Oct 29, 2021

Context here: http4s/http4s#5417

TL;DR:

This FHCRC flag is an optional feature that fs2 is opting-in to (with an implementation that looks good BTW), but unfortunately, many clients have/had a bugged implementation (e.g. netty/netty#11805) of it or perhaps don't even support it at all (and that's fine, as soon as they don't break 😄 ).

This PR is making it configurable, restoring a default of 0 (no FHCRC).
The client can choose to opt-in by specifying the fhCrcEnabled property in DeflateParams.

Ideally, we should backport this to 2.x (for supporting http4s 0.22 as well).

Hopefully, I preserved the binary compatibility.

@AL333Z
Copy link
Contributor Author

AL333Z commented Oct 29, 2021

Tests are failing:

image

Basically, what's happening is:

  • the test will compress the string Foo, setting the third byte (the one related to the OS running the compression) to 3 (Unix)
  • that byte will then influence the CRC16 computation, resulting in the diff we see for the next two bytes.

The values I used are actually working if the gzip compression is run on a Mac (OS byte = 7).

I'll provide an update with a decent solution shortly.

@mpilquist mpilquist merged commit 1bb4d6f into typelevel:main Oct 30, 2021
@AL333Z AL333Z deleted the opt-in-fhcrc-in-gzip-compression branch October 30, 2021 15:27
armanbilge added a commit to armanbilge/fs2 that referenced this pull request Oct 30, 2021
@armanbilge
Copy link
Member

Bakporting in #2701.

armanbilge added a commit to armanbilge/fs2 that referenced this pull request Oct 30, 2021
Co-authored-by: Alessandro Zoffoli <alessandro.zoffoli@moneyfarm.com>
mpilquist added a commit that referenced this pull request Oct 30, 2021
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.

4 participants