Skip to content

Fix ClientSession.close() hanging with HTTPS proxy connections #11289

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
Merged
merged 6 commits into from
Jul 9, 2025
Merged

Fix ClientSession.close() hanging with HTTPS proxy connections #11289

merged 6 commits into from
Jul 9, 2025

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Jul 9, 2025 •
edited
Loading

What do these changes do?

This PR fixes a regression where ClientSession.close() would hang indefinitely when HTTPS requests were made through an HTTP proxy. The issue was caused by CONNECT tunnel connections being pooled with proxy=None in the connection key, and their closed futures never completing. The leaking connections have been an issue for some time, but only became apparent when we fixed the waiting issue in 3.12.4.

The fix introduces a special _ConnectTunnelConnection class that overrides the release() method to prevent these connections from being pooled. This allows the connections to be properly used for TLS upgrade without interfering with session cleanup.

Are there changes in behavior for the user?

Users will no longer experience hanging when calling session.close() after making HTTPS requests through HTTP proxies. The behavior is restored to how it worked before we started waiting for the connections to close. They had been leaking into the connection pool for some time.

Is it a substantial burden for the maintainers to support this?

No. The fix is minimal and isolated:

  • Adds a small subclass of Connection with a single method override
  • Only affects CONNECT tunnel connections for HTTPS through HTTP proxies
  • Includes comprehensive tests to prevent regression
  • The special handling is clearly documented in the code

Related issue number

Fixes #11273

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes (no user-facing API changes)
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the CHANGES/ folder

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jul 9, 2025
@bdraco bdraco added backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot backport-3.13 Trigger automatic backporting to the 3.13 release branch by Patchback robot labels Jul 9, 2025
Dreamsorcerer
Dreamsorcerer reviewed Jul 9, 2025
View reviewed changes
@bdraco bdraco marked this pull request as ready for review July 9, 2025 20:13
@bdraco bdraco requested review from webknjaz and asvetlov as code owners July 9, 2025 20:13
@codecov Codecov
Copy link

codecov bot commented Jul 9, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.84%. Comparing base (9b0153c) to head (7413fc0).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #11289   +/-   ##
=======================================
  Coverage   98.84%   98.84%           
=======================================
  Files         131      131           
  Lines       43257    43285   +28     
  Branches     2326     2326           
=======================================
+ Hits        42756    42785   +29     
  Misses        346      346           
+ Partials      155      154    -1
Flag Coverage Δ
CI-GHA 98.73% <100.00%> (+<0.01%) ⬆️
OS-Linux 98.46% <100.00%> (+<0.01%) ⬆️
OS-Windows 96.79% <100.00%> (+<0.01%) ⬆️
OS-macOS 97.69% <100.00%> (+0.01%) ⬆️
Py-3.10.11 97.33% <96.55%> (+<0.01%) ⬆️
Py-3.10.18 97.81% <96.55%> (-0.01%) ⬇️
Py-3.11.13 97.99% <96.55%> (-0.01%) ⬇️
Py-3.11.9 97.52% <96.55%> (+<0.01%) ⬆️
Py-3.12.10 97.62% <96.55%> (-0.01%) ⬇️
Py-3.12.11 98.10% <96.55%> (+<0.01%) ⬆️
Py-3.13.3 98.36% <96.55%> (-0.01%) ⬇️
Py-3.9.13 97.21% <100.00%> (+<0.01%) ⬆️
Py-3.9.23 97.70% <100.00%> (+<0.01%) ⬆️
Py-pypy7.3.16 94.35% <100.00%> (+17.88%) ⬆️
VM-macos 97.69% <100.00%> (+0.01%) ⬆️
VM-ubuntu 98.46% <100.00%> (+<0.01%) ⬆️
VM-windows 96.79% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jul 9, 2025

CodSpeed Performance Report

Merging #11289 will not alter performance

Comparing avoid_release (7413fc0) with master (9b0153c)

Summary

✅ 59 untouched benchmarks

@Dreamsorcerer Dreamsorcerer merged commit e38220f into master Jul 9, 2025
40 checks passed
@Dreamsorcerer Dreamsorcerer deleted the avoid_release branch July 9, 2025 22:49
@patchback Patchback
Copy link
Contributor

patchback bot commented Jul 9, 2025 •
edited
Loading

Backport to 3.12: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.12/e38220fc4ed59c9de0dbe23da48e9cfd287c2ed7/pr-11289

Backported as #11291

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jul 9, 2025
@bdraco @patchback
Fix ClientSession.close() hanging with HTTPS proxy connections (#11289)
7c9f900 
(cherry picked from commit e38220f)
@patchback Patchback
Copy link
Contributor

patchback bot commented Jul 9, 2025 •
edited
Loading

Backport to 3.13: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.13/e38220fc4ed59c9de0dbe23da48e9cfd287c2ed7/pr-11289

Backported as #11292

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback patchback bot mentioned this pull request Jul 9, 2025
Merged
5 tasks
patchback bot pushed a commit that referenced this pull request Jul 9, 2025
@bdraco @patchback
Fix ClientSession.close() hanging with HTTPS proxy connections (#11289)
5ad2f73 
(cherry picked from commit e38220f)
@patchback patchback bot mentioned this pull request Jul 9, 2025
Merged
5 tasks
Dreamsorcerer pushed a commit that referenced this pull request Jul 9, 2025
@patchback @bdraco
[PR #11289/e38220fc backport][3.13] Fix ClientSession.close() hanging…
8790eb0 
… with HTTPS proxy connections (#11292)

**This is a backport of PR #11289 as merged into master
(e38220f).**

---------

Co-authored-by: J. Nick Koston <nick@koston.org>
Dreamsorcerer pushed a commit that referenced this pull request Jul 9, 2025
@patchback @bdraco
[PR #11289/e38220fc backport][3.12] Fix ClientSession.close() hanging…
edf2abd 
… with HTTPS proxy connections (#11291)

**This is a backport of PR #11289 as merged into master
(e38220f).**

---------

Co-authored-by: J. Nick Koston <nick@koston.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dreamsorcerer Dreamsorcerer Dreamsorcerer approved these changes

@webknjaz webknjaz Awaiting requested review from webknjaz webknjaz is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees
No one assigned
Labels
backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot backport-3.13 Trigger automatic backporting to the 3.13 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression when closing session
2 participants
@bdraco @Dreamsorcerer