-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Avoid starting connection timeout when a connection is already available #9600
Conversation
CodSpeed Performance ReportMerging #9600 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #9600 +/- ##
=======================================
Coverage 98.62% 98.63%
=======================================
Files 113 113
Lines 35299 35306 +7
Branches 4189 4191 +2
=======================================
+ Hits 34814 34823 +9
+ Misses 325 323 -2
Partials 160 160
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Backport to 3.11: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply e6187f6 on top of patchback/backports/3.11/e6187f6808d647243d8cee115ec24c3eb1421183/pr-9600 Backporting merged PR #9600 into master
🤖 @patchback |
…hen a connection is already available (#9607)
Tagging for backport to 3.10 as well since we need it to fix #9670 since a deadlock takes precedence of the potential risks from this PR |
Backport to 3.10: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply e6187f6 on top of patchback/backports/3.10/e6187f6808d647243d8cee115ec24c3eb1421183/pr-9600 Backporting merged PR #9600 into master
🤖 @patchback |
…hen a connection is already available (#9673)
What do these changes do?
Since connection re-use is common, we can avoid starting the
ceil_timeout
when we have a connection immediately available. Previously when a connection is already available for reuse,ceil_timeout
had to create aTimerHandle
, schedule it on the event loop, and then immediately cancel it, leaving the loop to have to remove it from the heap. See #8608 (comment) and #8613 for more history on the overhead of this set of operations.If the server supports keep-alive, the hit rate on avoiding the timeout is over 80% in testing. 31b9637
closes #9598
Are there changes in behavior for the user?
If
BaseConnector.connect
was replaced with a custom connect function, the timeout must now be handled in this function. The only abstract method the docs specify to be overwritten is_create_connection
. While it seems extremely unlikely that someone would write out a customconnect
function since it would likely break all the protected calls back into the connector fromConnection
, I think this change is likely safe to backport.To be on the safe side I only tagged it for 3.11 (tested in #9601) in case someone is doing that.This was originally planned not to backport to 3.10, but its needed to fix #9670Is it a substantial burden for the maintainers to support this?
no
Benchmarks
See script in #9598
aiohttp 3.10.0, yarl 1.9.5
aiohttp 3.11.0b0 yarl 1.17.0
aiohttp 3.11.0b0+ this change yarl 1.17.0
aiohttp 3.11.0b0+ this change yarl 1.17.1