Fix leaky connections #223
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
djc
reviewed
Oct 15, 2024
|
Thanks for your work on this! |
tneely
force-pushed
the
tneely/leaky-connections
branch
from
298ad54
to
86c8a50
Compare
djc
reviewed
Oct 16, 2024
tneely
force-pushed
the
tneely/leaky-connections
branch
from
86c8a50
to
c679d79
Compare
Fixes djc#221 It's possible to trigger more approvals than are necessary, in turn grabbing more connections than we need. This happens when we drop a connection. The drop produces a notify, which doesn't get used until the pool is empty. The first `Pool::get()` call on an empty pool will spawn an connect task, immediately complete `notify.notified().await`, then spawn a second connect task. Both will connect and we'll end up with 1 more connection than we need. Rather than address the notify issue directly, this fix introduces some bookkeeping that tracks the number of open `pool.get()` requests we have waiting on connections. If the number of pending connections >= the number of pending gets, we will not spawn any additional connect tasks.
tneely
force-pushed
the
tneely/leaky-connections
branch
from
c679d79
to
382d18b
Compare
|
See #226. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #221
It's possible to trigger more approvals than are necessary, in turn
grabbing more connections than we need. This happens when we drop a
connection. The drop produces a notify, which doesn't get used until the
pool is empty. The first
Pool::get()call on an empty pool will spawnan connect task, immediately complete
notify.notified().await, thenspawn a second connect task. Both will connect and we'll end up with 1
more connection than we need.
Rather than address the notify issue directly, this fix introduces some
bookkeeping that tracks the number of open
pool.get()requests we havewaiting on connections. If the number of pending connections >= the
number of pending gets, we will not spawn any additional connect tasks.
I have additionally changed
See #225notify.notify_waiters();to anotify.notify_one();call in the broken connection branch. A singlebroken connection should only need to notify one pending task to spawn a
new connect task, not all of them.