Automatic Gzip Decoding.

[echochamber]

Hey, got a chance to work on this again. Fixed all the merge conflicts and verified its working by testing it manually same as before.

Old PR: #25
Resolves Issue 3 #3

I'm having some difficulty using the server macro to create the response. Basically I know I'm not appending the gzipped bytes after the headers correctly but can't figure out how to do it correctly. Been banging my head against the problem long enough I thought I'd ask for help.

---- test_gzip_response stdout ----
	thread 'test_gzip_response' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Custom(Custom { kind: UnexpectedEof, error: StringError("failed to fill whole buffer") }) }', src/libcore/result.rs:860
stack backtrace:
   1:        0x101a65f49 - std::sys::imp::backtrace::tracing::imp::write::h9559bd7cb15d72ad
   2:        0x101a682ee - std::panicking::default_hook::{{closure}}::h4b3f2b69c9ce844d
   3:        0x101a67f1b - std::panicking::default_hook::h61d415f2381a7336
   4:        0x101a68767 - std::panicking::rust_panic_with_hook::h8e6300d8e8aca457
   5:        0x101a68604 - std::panicking::begin_panic::h08622fbe5a379aac
   6:        0x101a68572 - std::panicking::begin_panic_fmt::ha00d3aa9db91f578
   7:        0x101a684d7 - rust_begin_unwind
   8:        0x101a8e0a0 - core::panicking::panic_fmt::h58d018e87f211baf
   9:        0x101942dd0 - core::result::unwrap_failed::h8153acf35e8abd11
  10:        0x101937e5d - <core::result::Result<T, E>>::unwrap::hc7367165b5eb9345
  11:        0x101968e37 - reqwest::client::Decoder::from_hyper_response::h0cf3242a33a93308
  12:        0x10196737e - reqwest::client::RequestBuilder::send::hbb2cc738b3a79214
  13:        0x101912965 - reqwest::get::h56292d95415903cd
  14:        0x10191d5a1 - client::test_gzip_response::h247f29aed93f9d80
  15:        0x10197f34e - <F as test::FnBox<T>>::call_box::h0f96e4510234e16d
  16:        0x101a6971a - __rust_maybe_catch_panic
  17:        0x101973538 - std::panicking::try::do_call::h98f9aeef6f87f29b
  18:        0x101a6971a - __rust_maybe_catch_panic
  19:        0x10197a25e - <F as alloc::boxed::FnBox<A>>::call_box::h8926b341faa3dfd7
  20:        0x101a679d4 - std::sys::imp::thread::Thread::new::thread_start::h80e9dc7cc1dfe0d2
  21:     0x7fff902b3aaa - _pthread_body
  22:     0x7fff902b39f6 - _pthread_start

I know its failing on this line when unwrapping the GzDecoder (likely because the content wasn't valid gzipped bytes because I am appending them incorrectly).

decoder: GzDecoder::new(res).unwrap()

Hopefully its an easy fix.

[seanmonstar]

You're telling reqwest that the length of the response body is 0, and hyper will enforce this. After reading the headers, when trying to read the body from hyper, it will report that the body is empty.

• So, the easy fix is to change this to the length of the body (in this case, 105) (a wonderful example of how compression actually explodes small values, 12 -> 105!).
• I'm wondering if this specific case should also be fix. If there is no body, should the read just return Ok(0)? Or should point out that decoding an empty body doesn't work, and so the response is malformed? I'm probably leaning towards don't error (however, a warn! log may be appropriate.)

[echochamber]

@seanmonstar Oh, thanks for pointing that out. Fixed now, tests are passing.

I can do a little digging to see what other libs do when trying to gzip decode a non-gzipped body if you'd like.

[seanmonstar]

We probably want an option to allow someone to enable or disable this behavior. Either on the Client, or on an individual request. Considering that all the other configuration is on the Client, probably should be there.

let mut client = Client::new();
client.gzip(false);

[seanmonstar]

I'd actually keep the previous Debug version, it provides just the information that someone would want when writing logs.

[echochamber]

The old version of debug has been implemented on Decoder since the Decoder::Plaintext variant is now wrapping the hyper request. So deriving debug here makes sense since all Request is doing is wrapping a Decoder now. See changes on line 430

[seanmonstar]

Hm, what I meant was that the debug output probably doesn't need to leak the details of what kind of decoder it is using.

println!("res = {:?}", res);

Ideally that prints something like:

res = Response { status: Ok, headers: { ... }, version: Http11, }

[echochamber]

Ah, gotcha. Sounds good by me, I'll make the change.

[echochamber]

@seanmonstar

On the topic of making automatically ungzipping configurable, I see two places where the config option can go: reqwest::Client and ClientRef.

If its put on ClientRef then we don't need to change request builder at all because it has access to the property on ClientRef. However, since ClientRef is stored in an Arc, I'd need to wrap the config value in a mutex just like RedirectPolicy currently is.

If we put the property on reqwest::Client, then we also need to add the property on RequestBuilder (or else we'd need to make it an argument of RequestBuilder::send() which seems pretty cumbersome to me.

Which would you prefer?

[seanmonstar]

I'd say put it in the ClientRef object, as an AtomicBool for now.

On a separate note, I've noticed that this fails on Windows msys (thanks AppVeyor!). It seems like it cannot be fixed in flate2 crate, based on rust-lang/flate2-rs#42.

We might have to use either https://github.com/sile/libflate or https://github.com/PistonDevelopers/inflate

[echochamber]

I don't mind swapping in one of those libs. Seems fairly effortless to do so. The alternative would be to do what @alexcrichton suggested in his comment. I'd prefer to defer to you on this decision.

[echochamber]

@seanmonstar

From outdated change:

I'm wondering if this specific case should also be fix. If there is no body, should the read just return Ok(0)? Or should point out that decoding an empty body doesn't work, and so the response is malformed? I'm probably leaning towards don't error (however, a warn! log may be appropriate.)

This look good? Error message could probably be improved.

[seanmonstar]

This looks good to me!

[seanmonstar]

Yea, I don't think requiring that people install MSVC is something I want to do. If they have Rust working in mingw, then reqwest should hopefully work fine for them. One of the main goals of reqwest is to be extremely easy to setup and do the Right Thing.

[echochamber]

@seanmonstar Sounds good, I'll swap out flate2 for one of the alternatives listed above and ping you whe its ready for review again.

[echochamber]

@seanmonstar Replaced flate2-rs with libflate and updated the debug impl as you requested. Let me know if you'd like any other changes or if all looks good so I can squash the commits before merging.

[seanmonstar]

Looks perfect, thanks!

…

On Sat, Feb 25, 2017, 11:36 PM Jason Schein ***@***.***> wrote: @seanmonstar <https://github.com/seanmonstar> Replaced flate2-rs with libflate and updated the debug impl as you requested. Let me know if you'd like any other changes or if all looks good so I can squash the commits before merging. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADJF9d0oBpLTwggyf6yihlOMzDum3E5ks5rgStngaJpZM4MFT0g> .

[echochamber]

@seanmonstar Squashed and ready.

[seanmonstar]

Thanks so much @echochamber for keeping up on this, this will be a nice feature to release!

[echochamber]

@seanmonstar Thanks for the regular help and feedback along the way. I Doubt I would have actually been able to get this working without it!