GH-103472: close response in HTTPConnection._tunnel by graingert · Pull Request #103473 · python/cpython

graingert · 2023-04-12T13:01:11Z

Issue: ResourceWarning due to unclosed response in http.client.HTTPConnection._tunnel #103472

…6bOHv.rst

arhadthedev · 2023-04-25T11:47:51Z

@gpshead (as a committer into Lib/http/client.py)

orsenthil

This piece of code has been there for a while, so making changes here for intermittent failure seems to require bit more understanding on the nature of the error.

bedevere-bot · 2023-04-30T00:39:26Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

graingert · 2023-04-30T08:54:34Z

I have made the requested changes; please review again

bedevere-bot · 2023-04-30T08:54:38Z

Thanks for making the requested changes!

@orsenthil: please review the changes made to this pull request.

orsenthil · 2023-05-01T18:51:40Z

I have made the requested changes; please review again

Did you push any commits? I don't see it.

graingert · 2023-05-01T19:05:26Z

@orsenthil I responded to your comments I didn't need to make any changes

gpshead · 2023-05-01T21:28:14Z

overall, having tried to review the http.client logic in depth, I think what you're doing in this PR makes sense. Our http.client code is... quite complicated.

As you noted, it is the HTTPResponse sock.makefile() having a side "copy" of the socket as a file object that makes always calling response.close() not mess things up in the _tunnel() case. ("Closing the file object returned by makefile() won’t close the original socket unless all other file objects have been closed and socket.close() has been called on the socket object." - socket docs)

orsenthil

LGTM too. Thank you.

miss-islington · 2023-05-02T03:59:46Z

Thanks @graingert for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

bedevere-bot · 2023-05-02T03:59:54Z

GH-104077 is a backport of this pull request to the 3.11 branch.

…03473) Avoid a potential `ResourceWarning` in `http.client.HTTPConnection` by closing the proxy / tunnel's CONNECT response explicitly. --------- (cherry picked from commit 9de0cf2) Co-authored-by: Thomas Grainger <tagrain@gmail.com> Co-authored-by: Gregory P. Smith <greg@krypto.org>

#104077) GH-103472: close response in HTTPConnection._tunnel (GH-103473) Avoid a potential `ResourceWarning` in `http.client.HTTPConnection` by closing the proxy / tunnel's CONNECT response explicitly. --------- (cherry picked from commit 9de0cf2) Co-authored-by: Thomas Grainger <tagrain@gmail.com> Co-authored-by: Gregory P. Smith <greg@krypto.org>

* main: pythongh-103822: [Calendar] change return value to enum for day and month APIs (pythonGH-103827) pythongh-65022: Fix description of tuple return value in copyreg (python#103892) pythonGH-103525: Improve exception message from `pathlib.PurePath()` (pythonGH-103526) pythongh-84436: Add integration C API tests for immortal objects (pythongh-103962) pythongh-103743: Add PyUnstable_Object_GC_NewWithExtraData (pythonGH-103744) pythongh-102997: Update Windows installer to SQLite 3.41.2. (python#102999) pythonGH-103484: Fix redirected permanently URLs (python#104001) Improve assert_type phrasing (python#104081) pythongh-102997: Update macOS installer to SQLite 3.41.2. (pythonGH-102998) pythonGH-103472: close response in HTTPConnection._tunnel (python#103473) pythongh-88496: IDLE - fix another test on macOS (python#104075) pythongh-94673: Hide Objects in PyTypeObject Behind Accessors (pythongh-104074) pythongh-94673: Properly Initialize and Finalize Static Builtin Types for Each Interpreter (pythongh-104072) pythongh-104016: Skip test for deeply neste f-strings on wasi (python#104071)

* main: (760 commits) pythonGH-104102: Optimize `pathlib.Path.glob()` handling of `../` pattern segments (pythonGH-104103) pythonGH-104104: Optimize `pathlib.Path.glob()` by avoiding repeated calls to `os.path.normcase()` (pythonGH-104105) pythongh-103822: [Calendar] change return value to enum for day and month APIs (pythonGH-103827) pythongh-65022: Fix description of tuple return value in copyreg (python#103892) pythonGH-103525: Improve exception message from `pathlib.PurePath()` (pythonGH-103526) pythongh-84436: Add integration C API tests for immortal objects (pythongh-103962) pythongh-103743: Add PyUnstable_Object_GC_NewWithExtraData (pythonGH-103744) pythongh-102997: Update Windows installer to SQLite 3.41.2. (python#102999) pythonGH-103484: Fix redirected permanently URLs (python#104001) Improve assert_type phrasing (python#104081) pythongh-102997: Update macOS installer to SQLite 3.41.2. (pythonGH-102998) pythonGH-103472: close response in HTTPConnection._tunnel (python#103473) pythongh-88496: IDLE - fix another test on macOS (python#104075) pythongh-94673: Hide Objects in PyTypeObject Behind Accessors (pythongh-104074) pythongh-94673: Properly Initialize and Finalize Static Builtin Types for Each Interpreter (pythongh-104072) pythongh-104016: Skip test for deeply neste f-strings on wasi (python#104071) pythongh-104057: Fix direct invocation of test_super (python#104064) pythongh-87092: Expose assembler to unit tests (python#103988) pythongh-97696: asyncio eager tasks factory (python#102853) pythongh-84436: Immortalize in _PyStructSequence_InitBuiltinWithFlags() (pythongh-104054) ...

pythonGH-103472: close response in HTTPConnection._tunnel
86952ce

bedevere-bot mentioned this pull request Apr 12, 2023
ResourceWarning due to unclosed response in http.client.HTTPConnection._tunnel #103472
Closed

bedevere-bot added the awaiting review label Apr 12, 2023

📜🤖 Added by blurb_it.
6508762

graingert marked this pull request as ready for review April 12, 2023 13:07

graingert commented Apr 12, 2023
View reviewed changes

Comment threadMisc/NEWS.d/next/Library/2023-04-12-13-04-16.gh-issue-103472.C6bOHv.rst Outdated

graingert added 2 commits April 12, 2023 15:08

Update Misc/NEWS.d/next/Library/2023-04-12-13-04-16.gh-issue-103472.C…
f88793c
…6bOHv.rst

Merge branch 'main' into close-response-in-http-client-tunnel
959db96

pquentin approved these changes Apr 25, 2023
View reviewed changes

Merge branch 'main' into close-response-in-http-client-tunnel
5500734

test that HTTPConnection._tunnel closes the response on exceptions
2aa460c

graingert force-pushed the close-response-in-http-client-tunnel branch from 86c29f5 to 2aa460cCompare April 25, 2023 14:55

orsenthil requested changes Apr 30, 2023
View reviewed changes

Comment threadLib/http/client.py
Comment threadLib/http/client.py

bedevere-bot added awaiting changes and removed awaiting review labels Apr 30, 2023

orsenthil requested a review from pquentin April 30, 2023 00:39

bedevere-bot added awaiting change review and removed awaiting changes labels Apr 30, 2023

bedevere-bot requested a review from orsenthil April 30, 2023 08:54

gpshead reviewed May 1, 2023
View reviewed changes

Comment threadLib/test/test_httplib.py

gpshead self-assigned this May 1, 2023

gpshead added the needs backport to 3.11 only security fixes label May 1, 2023

graingert commented May 1, 2023
View reviewed changes

Comment threadLib/http/client.py

gpshead approved these changes May 2, 2023
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 2, 2023

gpshead added 2 commits May 1, 2023 20:35

minor wording and ReST update to NEWS
c2171c7

word wrap news.
bd079b1

gpshead enabled auto-merge (squash) May 2, 2023 03:36

orsenthil approved these changes May 2, 2023
View reviewed changes

gpshead merged commit 9de0cf2 into python:mainMay 2, 2023

bedevere-bot removed the awaiting merge label May 2, 2023

bedevere-bot removed the needs backport to 3.11 only security fixes label May 2, 2023

graingert deleted the close-response-in-http-client-tunnel branch May 2, 2023 11:26

Uh oh!

Uh oh!

Conversation

graingert commented Apr 12, 2023• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

arhadthedev commented Apr 25, 2023

Uh oh!

orsenthil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Apr 30, 2023

Uh oh!

graingert commented Apr 30, 2023

Uh oh!

bedevere-bot commented Apr 30, 2023

Uh oh!

orsenthil commented May 1, 2023

Uh oh!

graingert commented May 1, 2023

Uh oh!

Uh oh!

gpshead commented May 1, 2023

Uh oh!

Uh oh!

orsenthil left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented May 2, 2023

Uh oh!

bedevere-bot commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

graingert commented Apr 12, 2023•
edited by bedevere-bot
Loading