Skip to content

Fix max_keepalive_connections - #1000

Merged
lovelydinosaur merged 1 commit into
encode:masterfrom
blazewicz:blazewicz/fix-max-keepalive-connections
Oct 13, 2025
Merged

Fix max_keepalive_connections#1000
lovelydinosaur merged 1 commit into
encode:masterfrom
blazewicz:blazewicz/fix-max-keepalive-connections

Conversation

@blazewicz

@blazewiczblazewicz commented Mar 26, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

This commit fixes a bug in both sync and async connection pools where idle connections were dropped from the pool even when max_keepalive_connections limit has not been reached. This happened because the check compared this setting's value to the total number of connections, not only of the idle ones.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

blazewicz
blazewicz commented Mar 26, 2025
View reviewed changes
Comment threadtests/_async/test_connection_pool.py
blazewicz
blazewicz commented Mar 26, 2025
View reviewed changes
Comment threadtests/_sync/test_connection_pool.py
@blazewicz
blazewiczforce-pushed the blazewicz/fix-max-keepalive-connections branch 2 times, most recently from 0aa2ee1 to 6414fd8CompareMarch 29, 2025 18:12
@VictorPrinsVictorPrins mentioned this pull request Sep 14, 2025
Open
3 tasks
zanieb
zanieb approved these changes Sep 22, 2025
View reviewed changes

@zaniebzanieb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This does seem like a fairly straightforward oversight in the implementation.

@blazewicz
Fix max_keepalive_connections
7fb2052
This commit fixes a bug in both sync and async connection pools
where idle connections were dropped from the pool even when
max_keepalive_connections limit has not been reached. This
happened because the check compared this number to the total
number of connections, not only the idle ones.
@blazewicz
blazewiczforce-pushed the blazewicz/fix-max-keepalive-connections branch from 6414fd8 to 7fb2052CompareSeptember 23, 2025 09:31
@lovelydinosaur

lovelydinosaur commented Sep 23, 2025

Copy link
Copy Markdown
Contributor

Ah yeah apologies... this is a nice simple fix, and worth getting in.

@lovelydinosaur
lovelydinosaur merged commit 10a6582 into encode:masterOct 13, 2025
7 checks passed
@springmeyer

springmeyer commented Nov 6, 2025•
edited
Loading

Copy link
Copy Markdown

Will this be released soon in a formal release? Perhaps a 1.0.10?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zaniebzaniebzanieb approved these changes
+1 more reviewer
@methanemethanemethane approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@blazewicz@lovelydinosaur@springmeyer@methane@zanieb