igrigorik / em-http-request

Asynchronous HTTP Client (EventMachine + Ruby)
1.22k stars 221 forks source link

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

Open drbrain opened 7 years ago

drbrain commented 7 years ago

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 commented 7 years ago

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

igrigorik commented 7 years ago

/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 commented 7 years ago

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 commented 7 years ago

Gotcha, thanks Eric. We'll investigate..