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.

Sign up for GitHub

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

Jump to bottom

Make HTTPResponse.read1 close response when all data is read #3235

Merged
merged 11 commits into from
Jan 22, 2024
Merged

Make HTTPResponse.read1 close response when all data is read #3235

merged 11 commits into from
Jan 22, 2024

Conversation

illia-v
Copy link
Member

@illia-v illia-v commented Dec 15, 2023

In #3216, we discovered that CPython's http.client.HTTPResponse.read1 never closes the response after all data has been read and content length was known.

This PR adds a fix for the issue to _raw_read near a similar old fix that closes a response under a different condition.
I'll create a PR to fix this in CPython too most likely.

@illia-v illia-v added the Skip Changelog Pull requests that don't require a changelog entry label Dec 15, 2023
This was referenced Dec 15, 2023
Open
Closed
pquentin
pquentin previously approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! Congrats in getting your CPython fix in.

src/urllib3/response.py Outdated Show resolved Hide resolved
test/test_response.py Outdated Show resolved Hide resolved
sethmlarson
sethmlarson requested changes Dec 19, 2023
View reviewed changes
src/urllib3/response.py Outdated Show resolved Hide resolved
@sethmlarson sethmlarson self-requested a review December 19, 2023 19:24
sethmlarson
sethmlarson reviewed Dec 19, 2023
View reviewed changes
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying the behavior of read1() to me! I only had one review comment then this LGTM:

test/test_response.py Show resolved Hide resolved
@illia-v
Copy link
Member Author

illia-v commented Jan 7, 2024

@sethmlarson @pquentin can we merge this?

sethmlarson
sethmlarson previously approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, I'm sad we're having to get so specific for http.client but since we're working around a bug in that implementation it makes sense. Thanks Illia!

test/test_response.py Show resolved Hide resolved
test/test_response.py Outdated Show resolved Hide resolved
@illia-v illia-v merged commit 6b2b377 into urllib3:main Jan 22, 2024
30 of 32 checks passed
@illia-v illia-v deleted the read1-close branch January 22, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sethmlarson sethmlarson sethmlarson approved these changes

@pquentin pquentin Awaiting requested review from pquentin

Assignees
No one assigned
Labels
Skip Changelog Pull requests that don't require a changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@illia-v @pquentin @sethmlarson