Cutehacks / duperagent

A QML clone of VisionMedia's SuperAgent module.
MIT License
56 stars 23 forks source link

feature for handling gzip content encoding #18

Closed rmallah closed 6 years ago

rmallah commented 6 years ago
gzip content encoding can greatly enhance the performance of
applications by reducing the data payload in the wire. This
commit utilizes zlib to decompress a compressed response of
gzip format.
jrbarron commented 6 years ago

@rmallah Thank you very much for your contribution, but I'm wondering if this is actually needed. Qt's networking classes already take care of decoding gzip content as far as I know. Duperagent even has a unit test for it by doing a GET on httpbin.org/gzip which seems to be failing now. Is the unit test failing when you run it locally? If so, maybe you have a version of Qt built without zlib or something like that?

rmallah commented 6 years ago

Yes unit tests are failing. I will look into it . Also I feel it is needed as the decoding does not seems to be happening automatically. I will also check on it once again if Qt networking classes can do it already.

rmallah commented 6 years ago

@jrbarron , Hi James , can you please indicate how to build and run the unit tests for duperagent after cloning from the master in the cmd line.

jrbarron commented 6 years ago

@rmallah After cloning just do this:

cd tests
qmake
make
./tst_duperagent

It looks like one test is failing because an SSL certificate has changed, but the gzip one passes in master, but fails in this PR.

rmallah commented 6 years ago

thanks for the accurate instructions. I see that gzip related unit tests were not failing before and started to fail only after my changes . Shall take care of them. I do see that some more tests are also failing about which i shall post an issue.

rmallah commented 6 years ago

looks like gzip encoding is already being handled by duperagent. I will invetigate why i am not able to use it in my environment. closing this PR as it looks unnecessary.

rmallah commented 6 years ago

re-opening after adding more full-proof way of detecting gzip stream.

rmallah commented 6 years ago

Hi @jrbarron , In my environment i was setting the Accept-Encoding in request so that my server could return compressed data in response stream. in such a situation readAll() was returning compressed data and json parsing (of gzip stream) was failing.

The same is reproducible with baseline if tests include the Accept-Encoding header while requesting to httpbin.org/gzip .

in this PR the stream is being decompressed only when (1) Content-Encoding == gzip (2) Magic bytes in response confirm it is gzip stream.

it now passes the gzip related tests.

jrbarron commented 6 years ago

@rmallah Thanks for the updates, but I'm still not convinced that Duperagent is the right place for this fix though :)

QNetworkRequest automatically adds the Accept-Encoding header if you don't explicitly set it and it then sets the autoDecompress flag to true so that it will remember to decompress it later:

https://github.com/qt/qtbase/blob/5.11/src/network/access/qhttpnetworkconnection.cpp#L304-L313

If you think that Qt is not correctly detecting gzip compressed responses then I feel that this is really a bug in Qt that should be reported upstream.

I don't want to put a hard dependency to zlib in Duperagent, especially if Qt should be doing it for us.

rmallah commented 6 years ago

Thanks again for the comments and information. I will check again towards the root cause and raise the issue at appropriate place.

kind regards. Rajesh mallah.

rmallah commented 6 years ago

In the meantime can you please make travis happy.

jrbarron commented 6 years ago

@rmallah Is it ok if we close this issue? Manually setting the Accept-Encoding header definitely seems to affect how Qt internally handles the decompression. If it is just "gzip, deflate" you need though, then I am pretty sure Qt handles this case internally. If it is something else you need, then I think we need a separate issue for it.

rmallah commented 6 years ago

Pls go ahead . And thanks again. I will investigate later.