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

Get rid of HTTP response size check #8116

Merged
merged 1 commit into from Aug 21, 2019

Conversation

@gshuflin
Copy link
Contributor

commented Jul 27, 2019

This check that the size in bytes of the downloaded file is the same as
the size given in the HTTP Content-Length header is broken when
downloading a compressed file. cf.
https://2.python-requests.org/en/master/user/quickstart/#response-content
the iter_content method is iterating over the uncompressed bytes of
the file, but the Content-Length header gives the compressed lengths
of the file. So we don't expect these two lengths to match when
downloading a compressed file, and shouldn't be testing that they do.

@stuhood
Copy link
Member

left a comment

Given that requests (claims?) to only automatically decode gzip and deflate, would it be worth checking for gzip/deflate, and only disabling this check in those cases?

@gshuflin gshuflin force-pushed the gshuflin:fetcher branch 2 times, most recently from ad94fb9 to ae9e6ab Aug 13, 2019

@gshuflin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Updated this to check whether gzip/deflate is being used, and only do the check if not

@gshuflin gshuflin force-pushed the gshuflin:fetcher branch 3 times, most recently from bc5486a to cd44502 Aug 14, 2019

Don't check HTTP response size if gzip'd
This check that the size in bytes of the downloaded file is the same as
the size given in the HTTP Content-Length header is broken when
downloading a compressed file. cf.
https://2.python-requests.org/en/master/user/quickstart/#response-content
the iter_content method is iterating over the *uncompressed* bytes of
the file, but the Content-Length header gives the *compressed* lengths
of the file. So if the transfer-encoding is gzip or deflate, we don't
expect these two lengths to match and shouldn't be testing that they do
@benjyw

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@stuhood ping?

@stuhood

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@benjyw : There was a failing test in CI before, and it's currently red. Figured I'd wait until that got cleaned up.

@benjyw

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Those were unrelated CI flakes, and it's green now. Thanks!

@stuhood stuhood merged commit fc131d3 into pantsbuild:master Aug 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gshuflin gshuflin deleted the gshuflin:fetcher branch Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.