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

Add transparent compression handling in the Response struct. #14

Closed
szank opened this issue Dec 3, 2015 · 10 comments
Closed

Add transparent compression handling in the Response struct. #14

szank opened this issue Dec 3, 2015 · 10 comments

Comments

@szank
Copy link

szank commented Dec 3, 2015

Hi,
I was wondering if adding transparent compression handling in the code is feasible.
Currently we check if the "Content-Encoding" header have a "gzip" value and decompress the data ourselves.
I might do it myself if I have some free time on my hands, but I was wondering if this change would be even accepted

@valyala
Copy link
Owner

valyala commented Dec 4, 2015

Currently the client doesn't set Accept-Encoding headers to gzip or deflate, so conforming server should never send compressed response with 'Content-Encoding' response header.

Transparent compression may be added to the client as opt-in setting in Client and HostClient. EnableCompression is a good name for this setting. You may create pull request for this feature.

Enabling transparent compression in fasthttp client by default is no go, since default compression implementation in Go is slooow ;)

@szank
Copy link
Author

szank commented Dec 4, 2015

My idea was to check for the 'Content-Encoding' header in the response, and decompress the body if the header was present.
I didn't indent do modify the request sending path, only receiving. We could enable this behaviour by default, and disable it by setting 'AutoDecompress' to false in the client/response, where the value from the response would take precedence.

Now, We could add a EnableCompression parameter also, to transparently compress the requests, but this is a story for another pull request.

In our use case, we use haproxy to compress the responses sent from the server ( We do set 'Content-Encoding' in the request header explicitly), and then check for the 'Content-Encoding' in the response.

@valyala
Copy link
Owner

valyala commented Dec 4, 2015

Nice idea! I'll try implementing it on the next week.

@valyala
Copy link
Owner

valyala commented Dec 25, 2015

@szank , I decided adding helper functions for body compression instead of transparent compression. This allows using response body compression only when needed.

See Response.BodyGunzip and Response.BodyInflate.

valyala added a commit that referenced this issue Dec 25, 2015
@valyala
Copy link
Owner

valyala commented Dec 25, 2015

Added CompressHandler for transparent response compression on server side.

@dibu28
Copy link

dibu28 commented Feb 10, 2016

There is github.com/klauspost/compress a fully gzip compatible drop in replacement for go standard gzip/zip/zlib packages.
Claimed to be up to 3x faster for data compression.
Description and benchmarks by the author:
http://blog.klauspost.com/go-gzipdeflate-benchmarks/
http://blog.klauspost.com/gzip-performance-for-go-webservers/

pgzip Go parallel gzip compression/decompression. This is a fully gzip compatible drop in replacement for "compress/gzip". Faster for big files more than 1MB.

cgzip wraps zlib using cgo.

valyala added a commit that referenced this issue Feb 10, 2016
…rd compress/*, since it provides better performance on average
@sschepens
Copy link
Contributor

What about transparent decompression of incoming requests to server?

valyala added a commit that referenced this issue Feb 18, 2016
@valyala
Copy link
Owner

valyala commented Feb 18, 2016

Just added Request.BodyGunzip helper.

Transparent compression / decompression won't be implemented in fasthttp, since this may be abused by specially crafted malicious requests / responses, which may eat a lot of CPU and memory resources. So fasthttp users must manually compress / decompress request / response bodies when needed.

@valyala
Copy link
Owner

valyala commented Feb 18, 2016

I think this issue may be closed now. Feel free creating new issues for unresolved topics mentioned above.

@valyala valyala closed this as completed Feb 18, 2016
@sschepens
Copy link
Contributor

Thanks! Isn't BodyInflate() missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants