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

443 - Fix Transfer-Encoding: chunked for downloads #444

Closed
wants to merge 1 commit into from
Closed

443 - Fix Transfer-Encoding: chunked for downloads #444

wants to merge 1 commit into from

Conversation

th317erd
Copy link

@th317erd th317erd commented Sep 27, 2019

PR for issue #443

This may not be the best fix... open to suggestions for a better fix.

@ryam4u
Copy link

ryam4u commented Sep 27, 2019

This fixes the bug, please merge it

@sonudoo
Copy link

sonudoo commented Sep 27, 2019

Yes. This does fix the regression. But it is a bit of hacky way to do it. Can you please add a comment for the if statement before it gets merged?

@Traviskn
Copy link

I reverted #396 which introduced the regression, I am curious if there is any way to know the expected content length when using chunked transfer encoding?

@sonudoo
Copy link

sonudoo commented Sep 27, 2019

In that case, we have to check for a End marker which can be an empty body or EOL. I have to check what exactly we get in response. We can introduce another variable that keeps track of this End marker and return true is it was received.

@th317erd
Copy link
Author

Yeah, I can add a comment, but it sounds like more is needed (and it also sounds like I may not be the best person to do such work). Would you still like me to fix this and have it merged (since you reverted the other "fix")? What would you like to do with this PR?

@Traviskn
Copy link

I think we will need a new pull request implementing the strategy described by @sonudoo

@Traviskn Traviskn closed this Sep 27, 2019
@sonudoo
Copy link

sonudoo commented Sep 27, 2019

I will be raising one in the next week implementing that strategy.

@sonudoo
Copy link

sonudoo commented Sep 30, 2019

Here is my attempt. Have a look at this PR: #449
@Traviskn @th317erd

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

Successfully merging this pull request may close these issues.

4 participants