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
Issue with GZip compression? #5417
Comments
i can confirm this on 0.22.7, 0.22.6 works fine |
Thanks for reporting. Yep, that would be the similar #5368 that targeted 0.22. |
If either of you has an opportunity, would you be able to make a replication? Obviously this is something that is slipping through both the fs2 and http4s test suites 😕 |
I made a reproducer, unfortunately not as simple test case because it requires that another HTTP implementation is doing a request, so it spins up a http4s server and uses the AHC/Netty client to make a request to it: object Main extends IOApp with Http4sDsl[IO] {
val routes: HttpRoutes[IO] = HttpRoutes.of[IO] {
case req => IO.println(req.headers) >> Ok("Foo")
}
override def run(args: List[String]): IO[ExitCode] =
EmberServerBuilder.default[IO]
.withHost(ipv4"0.0.0.0")
.withPort(port"8080")
.withHttpApp(GZip(routes).orNotFound)
.build
.use { _ =>
AsyncHttpClient.resource[IO](new DefaultAsyncHttpClientConfig.Builder().setCompressionEnforced(true).build()).use { client =>
client.expect[String]("http://localhost:8080/").flatMap(IO.println)
}.as(ExitCode.Success)
}
} Fails with:
|
Thank you very much!!! This is great. |
I had a look at this.. and it's fun.
If you squeeze your eyes, you can see that the header flags byte The spec says:
Now, it seems that the fs2 implementation is just fine (computing the CRC32 and getting the least significant bytes..), so I checked netty. I'm not too familiar with this stuff, so please take it with a grain of salt... |
Nice research! We've seen failures with native iOS clients as well, so if that's a bug on the client side of things, it's not only Netty. Which would probably lead to the question whether we need some configurability here to allow clients that don't support the full spec. Or is there maybe some RFC that discourages the CRC16 in HTTP contexts? Edit: https://www.rfc-editor.org/rfc/rfc2616#section-3.5 could be read as such hint (specifying values of
|
Oh boy...
maybe supporting a config that lets the client choose to enable that additional check or not may be a good tradeoff. Also, if it turns out the clients are actually wrong, it may take time to get them fixed.
I think that sentence is more related to the trailer, which must always contain the CRC32 for gzip, as per https://www.ietf.org/rfc/rfc1952.txt - section 2.3. |
Thanks for chasing this up, this is fantastic. And seeing that netty hasn't rejected your PR at face value I'd say you're definitely on to something 😉 IIUC this So whether we like it or not, it seems to me we need to make configurable whether to enable/disable this feature/flag, and make the old behavior the default in http4s for compatibility. This will need to be a change in fs2, ideally on both its series 2.x and 3.x branches for http4s 0.22 and 0.23, respectively. Relevant LOC: |
Perfectly summarized.
I can pick this, while I'm at it :) |
An additional data point: the client-side issues we saw on iOS were limited to iOS 12. We could not reproduce the issue on iOS 13 onwards. I have tried to find additional information but since iOS is closed source I can't really know why however, it could play into the idea that it's a client-side issue. |
Cool, just saw that netty/netty#11805 is merged. Great work! |
@vilu @ybasket thanks to @AL333Z's fantastic work on multiple fronts, if you use http4s 0.23.6 with fs2 @Ravenow never fear, a backport to fs2 2.x for http4s 0.22 is on the way in typelevel/fs2#2701. |
@Ravenow Now available for http4s 0.22 in fs2 |
Where does that leave us? Leave this open until we have fs2 releases with the fix, but no code change on our end? |
Yes, the default behavior was changed in fs2 so we don't need any code changes here. Up to you when to close the issue :) |
Eh, no further action on our part, and people have a (snapshot) solution toward the bottom of this. Closing. Great job tracking this one down. |
For those afraid of hashes, this fix is now available in fs2 3.2.3: |
And now (finally!) in fs2 2.5.11 as well: |
Hi, we noticed an issue when upgrading from 0.23.4 to 0.23.6. Some clients started failing and blaming the gzip compression. One example would be
io.netty.handler.codec.compression.DecompressionException: CRC value mismatch.
The text was updated successfully, but these errors were encountered: