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

DisableCompression is ignored in http2 requests #39

Closed
jsnjack opened this issue Apr 20, 2023 · 3 comments
Closed

DisableCompression is ignored in http2 requests #39

jsnjack opened this issue Apr 20, 2023 · 3 comments

Comments

@jsnjack
Copy link
Contributor

jsnjack commented Apr 20, 2023

When retrieving a resource via http2, fhttp library always decompresses the body, even if DisableCompression is set to true. According to the option description this should not happen:

	// DisableCompression, if true, prevents the Transport from
	// requesting compression with an "Accept-Encoding: gzip"
	// request header when the Request contains no existing
	// Accept-Encoding value. If the Transport requests gzip on
	// its own and gets a gzipped response, it's transparently
	// decoded in the Response.Body. However, if the user
	// explicitly requested gzip it is not automatically
	// uncompressed.

I looked into parts which read body in http1 and http2 and compared them. http1 has addedGzip flag which is checked before calling DecompressBody function (https://github.com/bogdanfinn/fhttp/blob/959a7f72d1ca6d6319e1ed82a26eea431658d3b3/transport.go#L2190). In http2 we have requestedGzip flag, which acts in similar way. However, requestedGzip is not checked (https://github.com/bogdanfinn/fhttp/blob/master/http2/transport.go#L2259).

The following change seem to solve the issue bogdanfinn/fhttp@8237b3b

@bogdanfinn
Copy link
Owner

@jsnjack thank you for supplying the fix for that.
will include that in the next release and i think about adding an option to the tlsclient itself to be able to set DisableCompression to either true or false.

@jsnjack
Copy link
Contributor Author

jsnjack commented Apr 21, 2023

Thanks!

By the way, tlsclient already has this option. It can be set via TransportOptions

@bogdanfinn
Copy link
Owner

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