Skip to content

Add background async health check for unhealthy pooled resources - #391

Open
mdedetrich wants to merge 1 commit into
reactor:mainfrom
mdedetrich:add-healthCheckTimeout
Open

mdedetrich wants to merge 1 commit into
reactor:mainfrom
mdedetrich:add-healthCheckTimeout

Conversation

@mdedetrich

@mdedetrich mdedetrich commented Aug 14, 2026

Copy link
Copy Markdown

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 evictionPredicate wasn't suitable since it had a BiPredicate signature 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 existing SimpleDequePool by 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

while (!e.isEmpty()) {
    QueuePooledRef<POOLABLE> ref = e.poll();
    if (ref.markDestroy()) {   // <- NPE here
        ...
    }
}

Individually e.isEmpty() and e.poll() are atomic (where e is a ConcurrentLinkedDeque) but combined in this way they are not, another thread could remove the last element and at the same time e.poll is accessed and you would get null (causing NPE) and this case happens with the PR's new healthCheckInBackground() (which runs in another thread) and it could have also happened with another future feature where another thread would be accessing e.

@mdedetrich
mdedetrich force-pushed the add-healthCheckTimeout branch 4 times, most recently from 29c79a9 to 8c66b50 Compare August 15, 2026 12:17
@mdedetrich mdedetrich changed the title Add background async health check for idle pooled resources Add background async health check for invalid pooled resources Aug 15, 2026
@mdedetrich
mdedetrich force-pushed the add-healthCheckTimeout branch 2 times, most recently from cdd6b06 to 5bdb6fd Compare August 16, 2026 11:31
@mdedetrich mdedetrich changed the title Add background async health check for invalid pooled resources Add background async health check for unhealthy pooled resources Aug 16, 2026
@mdedetrich
mdedetrich force-pushed the add-healthCheckTimeout branch from 5bdb6fd to 99cbda8 Compare August 16, 2026 11:35
Signed-off-by: Matthew de Detrich <mdedetrich@gmail.com>
@mdedetrich
mdedetrich force-pushed the add-healthCheckTimeout branch from 99cbda8 to 47032fb Compare August 16, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow POOLABLE health check verifications

1 participant