Add background async health check for unhealthy pooled resources - #391
Open
mdedetrich wants to merge 1 commit into
Open
mdedetrich wants to merge 1 commit into
mdedetrich wants to merge 1 commit into
Conversation
mdedetrich
force-pushed
the
add-healthCheckTimeout
branch
4 times, most recently
from
August 15, 2026 12:17
29c79a9 to
8c66b50
Compare
mdedetrich
force-pushed
the
add-healthCheckTimeout
branch
2 times, most recently
from
August 16, 2026 11:31
cdd6b06 to
5bdb6fd
Compare
mdedetrich
force-pushed
the
add-healthCheckTimeout
branch
from
August 16, 2026 11:35
5bdb6fd to
99cbda8
Compare
Signed-off-by: Matthew de Detrich <mdedetrich@gmail.com>
mdedetrich
force-pushed
the
add-healthCheckTimeout
branch
from
August 16, 2026 11:36
99cbda8 to
47032fb
Compare
This file contains hidden or 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
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.
Resolves: #285
I did my best to take into account the concerns raised in r2dbc/r2dbc-pool#187 and I also ported a relevant test from HikariCP just to be doubly sure that everything works as expected.
The currently existing
evictionPredicatewasn't suitable since it had aBiPredicatesignature which is synchronous/non blocking so a new one had to be made (i.e. r2dbc-pool needs to do async requests against databases). The implementation works on the currently existingSimpleDequePoolby adding health check ticks, making sure that while a resource is being checked it cannot be acquired (as well as other edge cases).The expected configuration knobs have been added, I also added a
TODO//to remind ourselves to remove unnecessary overrides when v2 gets released.While working on this I have found a race condition which while it may not be problematic before, is now due to the newly added healthcheck i.e., previously there was
Individually
e.isEmpty()ande.poll()are atomic (whereeis aConcurrentLinkedDeque) but combined in this way they are not, another thread could remove the last element and at the same timee.pollis accessed and you would getnull(causing NPE) and this case happens with the PR's newhealthCheckInBackground()(which runs in another thread) and it could have also happened with another future feature where another thread would be accessinge.