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

EventMachine::HttpDecoders::GzipHeader is unnecessary, Zlib::Inflate can do this for you #312

Open
drbrain opened this issue Jul 7, 2017 · 4 comments

Comments

@drbrain
Copy link

drbrain commented Jul 7, 2017

Zlib provides automatic gzip detection for you so you don't need to skip over it by hand. From inflateInit2 in the zlib manual (also reflected in the Zlib::Inflate.new documentation):

Add 32 to windowBits to enable zlib and gzip decoding with automatic header detection, or add 16 to decode only the gzip format.

So you can replace L228 of EventMachine::HttpDecoders::Gzip with:

# zlib with automatic gzip detection
@zstream ||= Zlib::Inflate.new(32 + Zlib::MAX_WBITS)

And delete the code in GzipHeader to prevent bugs like #284/#301

@drbrain
Copy link
Author

drbrain commented Jul 7, 2017

Note, this is what Net::HTTP uses for all zlib-based content encodings

@igrigorik
Copy link
Owner

/cc @martoche @eriwen @boxofrad, who've contributed most of the gzip streaming logic.

@drbrain thanks for the heads up. It's not clear to me yet if this will break any of the underlying (streaming) use cases.. shouldn't, right? Also, is there a particular reason you raised this issue now -- are you experiencing some trouble with existing code?

@drbrain
Copy link
Author

drbrain commented Jul 11, 2017

I'm not using it but was inspired to look at the code due to a blog post I read that used this gem instead of using Zlib::Inflate directly. I struggled with the same problem as the GzipHeader skipping code this gem uses for Net::HTTP and noticed the special flag in the zlib documentation.

@igrigorik
Copy link
Owner

Gotcha, thanks Eric. We'll investigate..

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

No branches or pull requests

2 participants