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

handle chunked encoding responses that lack content length header #6891

Merged
merged 1 commit into from Aug 23, 2019

Conversation

@mwrock
Copy link
Contributor

commented Aug 22, 2019

This is a bug introduced in the migration to reqwest but has nothing to do with that crate but is instead related to some refactoring of the line that gets the content_length header. We used to fall back to 0 if there was no header but now it will throw the no header error.

It will not be uncommon for on prem builders to use chunked transfer encoding which lacks a content_length header. This should ignore the lack of that header.

I validated this by "hacking" the package URL with http://anglesharp.azurewebsites.net/Chunked which sends content in chunks.

Signed-off-by: mwrock matt@mattwrock.com

@mwrock mwrock requested review from chefsalim and raskchanky as code owners Aug 22, 2019
@chef-expeditor

This comment has been minimized.

Copy link

commented Aug 22, 2019

Hello mwrock! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

if let Ok(header) = resp.get_header(CONTENT_LENGTH) {
let size = header.parse().map_err(Error::ParseIntError)?;
progress.size(size);
}

This comment has been minimized.

Copy link
@raskchanky

raskchanky Aug 22, 2019

Member

I notice in both this case and the one above, when progress is None, we just call io::copy. In this case, we're creating a BroadcastWriter that takes progress but if there's no CONTENT_LENGTH header, then progress never has its size set, right?

Phrased differently, in the case where there's no CONTENT_LENGTH length header, and thus we can't size progress, does it make sense to just call io::copy like we do when progress is None, and skip creating the BroadcastWriter?

This comment has been minimized.

Copy link
@mwrock

mwrock Aug 23, 2019

Author Contributor

good point. I just changed this to bail on the BroadcastWriter when there is no content length.

Signed-off-by: mwrock <matt@mattwrock.com>
@mwrock mwrock force-pushed the no_length branch from d43a329 to 2d82102 Aug 23, 2019
Copy link
Member

left a comment

Nice catch @mwrock !

@mwrock mwrock merged commit 073659f into master Aug 23, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3227 passed (29 minutes, 44 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #348 passed (3 minutes, 34 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@mwrock mwrock deleted the no_length branch Aug 23, 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.