This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 99bbc6e1d8a87bf10d7e8b1d140f61d394af2de5 Author: opencode <[email protected]> AuthorDate: Thu Oct 29 20:40:29 2026 +0100 Do not test idle connections the pool has not claimed The idle test validated and reconnected connections while they were still published in the idle queue, and only the release path claimed a connection out of it before acting. With the per-connection lock disabled - the configuration an administrative testIdle() runs in when the eviction timer is off and useLock is false - a borrower that polled an expired connection out of the queue while the sweep was reconnecting it ran its own disconnect-then-connect over the same pooled connection, and the loser's physical handle was overwritten by the winner's field write: a session nothing referenced any more and that therefore could never be closed, with every pool counter still looking healthy. The sweep now claims each connection out of the idle queue before testing it, the way the eviction and abandoned checks already do, and hands the connection back under the return path's maxIdle rule, releases it on a failed test, discards it on an unchecked driver failure, and - when the pool closed while the connection was claimed and the shutdown drain therefore passed it by - disconnects it instead of offering it back into the dead queue. A borrower is consequently never handed a connection while it is being reconnected, and a closing pool never gets a claimed connection offered into it. TestStatementCacheReconnect's helper asked for a 60-second sweeper interval to keep the background sweep away, which the pool clamps down to maxAge, letting the sweep race its counted reconnects; it now switches the sweeper off, which is what its comment already promised. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 76 ++++- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 6 +- .../tomcat/jdbc/pool/TestTestAllIdleClaim.java | 362 +++++++++++++++++++++ .../interceptor/TestStatementCacheReconnect.java | 8 +- 4 files changed, 431 insertions(+), 21 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index 78ed88dfd7..db50b2cc64 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -1547,10 +1547,13 @@ public class ConnectionPool { * Forces a validation of all idle connections if {@link PoolProperties#testWhileIdle} is set. * <p> * Note: when this method is invoked externally while the per-connection lock is disabled (sweeper switched off and - * {@link PoolProperties#getUseLock()} returning {@code false}), validating and reconnecting a connection can still - * overlap with a borrower that just took it out of the idle queue. That overlap is transient and surfaces to one of - * the two threads as a connection error; it is sharpest when {@link PoolProperties#getMaxAge()} is set so that an - * expired connection may be reconnected under a borrower mid-use. + * {@link PoolProperties#getUseLock()} returning {@code false}), it still serialises with borrowers through the + * queues: every connection is claimed out of the idle queue before it is validated or reconnected, and is put back + * or released afterwards, so a borrower is never handed a connection this method is working on. That claim is what + * makes an invocation concurrent with traffic safe, as {@link PooledConnection#lock()} alone protects nothing in + * this configuration. A pool closed while a connection is claimed does not get it offered back into its idle + * queue - the shutdown drain has passed it by, so the released connection is disconnected directly; the small + * window that accepts between that check and the queue hand-over is the same one the return path accepts. */ public void testAllIdle() { testAllIdle(false); @@ -1576,21 +1579,60 @@ public class ConnectionPool { if (busy.contains(con)) { continue; } - - boolean release; - if (checkMaxAgeOnly) { - release = !reconnectIfExpired(con); - } else { - release = !reconnectIfExpired(con) || !con.validate(PooledConnection.VALIDATE_IDLE); + // Claim the connection before acting on it; without a + // real lock the check above may already be stale, and a + // connection a borrower took out of the idle queue in + // the meantime must not be validated or reconnected + // here. Reconnecting one under a borrower is the worst + // of it: both threads run disconnect-then-connect on the + // same PooledConnection, the loser's physical handle is + // overwritten by the winner's field write, and a session + // nothing references any more can never be closed. + if (!idle.remove(con)) { + continue; } - if (release) { - // Claim the connection before acting on it; without - // a real lock the check above may already be stale, - // and a connection a borrower took out of the idle - // queue in the meantime must not be released here - if (!idle.remove(con)) { - continue; + boolean keep; + try { + if (checkMaxAgeOnly) { + keep = reconnectIfExpired(con); + } else { + keep = reconnectIfExpired(con) && con.validate(PooledConnection.VALIDATE_IDLE); } + } catch (RuntimeException x) { + // The checks above swallow every checked driver + // failure, but an unchecked throw from behind one of + // them - a validation ending a transaction is one + // such call - must not escape now that the + // connection is claimed: unwinding here would leave + // it in neither queue, invisible to the next sweep + // and to the close() drain alike. Take it out of + // service instead, as a failed check already does. + con.setDiscarded(true); + release(con); + log.warn("Unable to test idle connection [" + con + "], closing it.", x); + continue; + } + if (keep) { + if (isClosed()) { + // The pool was closed while this connection was + // claimed: the shutdown drain has already passed + // over the queues and a closing pool offers no + // further item to the idle queue, so return it + // through the drain's own exit, as the return + // path does - an offer would land it in a dead + // queue that nothing drains any more, stranding + // the session out of every counter + release(con); + } else if (((idle.size() >= poolProperties.getMaxIdle()) && + !poolProperties.isPoolSweeperEnabled()) || (!idle.offer(con))) { + if (log.isDebugEnabled()) { + log.debug("Connection [" + con + "] passed the idle test but will be closed and " + + "not returned to the pool, idle[" + idle.size() + "]>=maxIdle[" + + poolProperties.getMaxIdle() + "] idle.offer failed."); + } + release(con); + } + } else { releasedIdleCount.incrementAndGet(); release(con); } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java index e824cb7e8c..727eedfa2c 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java @@ -791,8 +791,10 @@ public class DataSourceProxy implements PoolConfiguration { /** * Performs a validation on idle connections. * <p> - * Note: when invoked while the per-connection lock is disabled (sweeper switched off and - * {@link PoolProperties#getUseLock()} returning {@code false}), see the residual overlap documented on + * Note: this operation is safe with concurrent traffic and with a closing pool even when the per-connection lock + * is disabled (sweeper switched off and {@link PoolProperties#getUseLock()} returning {@code false}): each idle + * connection is claimed out of the idle queue before it is validated or reconnected and is handed back, released + * or - if the pool closed in the meantime - disconnected without ever re-entering the queue, as described on * {@link ConnectionPool#testAllIdle()}. */ public void testIdle() { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestTestAllIdleClaim.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestTestAllIdleClaim.java new file mode 100644 index 0000000000..5af5e3d010 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestTestAllIdleClaim.java @@ -0,0 +1,362 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool; + +import java.lang.reflect.Field; +import java.sql.Connection; +import java.sql.Driver; +import java.sql.DriverPropertyInfo; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.Properties; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Logger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The idle test validates and reconnects connections outside the borrow + * and return paths. With the per-connection lock switched off - which is + * exactly the configuration an administrative {@code testIdle()} runs in + * when the eviction timer is off ({@code lock()} is a noop unless + * {@code useLock} or the sweeper is on) a reconnect must not proceed while + * the connection still sits in the idle queue: a borrower polling it out + * in that window runs its own disconnect-then-connect over the same + * pooled connection, and the loser's physical handle is overwritten by + * the winner's field write - a session nothing references any more and + * that therefore can never be closed. The sweep has to claim the + * connection first, the way {@code checkIdle()} and + * {@code checkAbandoned()} already claim before acting - and a pool that + * closed while a connection was claimed must not get it offered back: + * the shutdown drain cannot reach a claimed connection, so the claim + * holder releases it directly. + * + * <p>Every ordering here is enforced through latches and the pool's own + * wait-count, never through sleeps as the correctness signal. + */ +class TestTestAllIdleClaim extends JdbcPoolBaseTest { + + /* + * What the driver should do with every connect() after the very first + * one (the first always delegates, so the pool can be filled). + * Static, because the driver is instantiated by class name. + */ + private static final int DIRECT = 0; + private static final int HOLD = 1; + private static final int FAIL = 2; + private static volatile int mode = DIRECT; + private static final AtomicInteger CONNECTS = new AtomicInteger(); + /* Announced by a held connect() once it is parked, released by the test. */ + private static volatile CountDownLatch CONNECT_ATTEMPTED = new CountDownLatch(1); + private static volatile CountDownLatch CONNECT_RELEASE = new CountDownLatch(1); + + private static void resetDriver() { + mode = DIRECT; + CONNECTS.set(0); + CONNECT_ATTEMPTED = new CountDownLatch(1); + CONNECT_RELEASE = new CountDownLatch(1); + } + + /* + * One-slot pool on the controlled driver, no sweeper and no lock - + * the configuration in which lock() protects nothing and the queues + * are the only serialisation left. + */ + private DataSource oneSlotUnlockedPool() throws SQLException { + DataSource ds = newDataSource(); + ds.setDriverClassName(LatchingDriver.class.getName()); + ds.getPoolProperties().setInitialSize(0); + // exactly one slot: the claimed connection stays counted in size, + // so a borrower cannot route around it by creating another one + ds.getPoolProperties().setMaxActive(1); + // a real maxAge, so the sweep really reconnects; the test expires + // the connection itself, no clock waiting involved + ds.getPoolProperties().setMaxAge(60000); + // no timer: isPoolSweeperEnabled() stays false even with maxAge set, + // so the per-connection lock is a noop, as useLock defaults to false + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(0); + ds.getPoolProperties().setMaxWait(15000); + ds.createPool(); + return ds; + } + + /* + * Expire a connection deterministically: rewind its last-connected + * clock instead of waiting out a small maxAge. + */ + private static void forceExpired(PooledConnection pc) throws Exception { + Field lastConnected = PooledConnection.class.getDeclaredField("lastConnected"); + lastConnected.setAccessible(true); + lastConnected.setLong(pc, System.currentTimeMillis() - 120000L); + } + + @Test + void aConnectionTheIdleTestIsReconnectingIsOutOfTheQueueAndUnborrowable() throws Exception { + resetDriver(); + DataSource ds = oneSlotUnlockedPool(); + + // one idle connection on a known physical session, then expired + Connection first = ds.getConnection(); + awaitSessions(1); + setSessionMarker(first, 7); + PooledConnection pc = (PooledConnection) first.unwrap(PooledConnection.class); + first.close(); + forceExpired(pc); + + // the sweep reaches the driver's connect() and parks there: its + // disconnect() has closed the old session and the new one is not + // open yet, so whatever happens next, it happens over this window + mode = HOLD; + AtomicReference<Throwable> sweepFailure = new AtomicReference<>(); + Thread sweep = new Thread(() -> { + try { + ds.getPool().testAllIdle(); + } catch (Throwable t) { + sweepFailure.set(t); + } + }, "idle-sweep"); + try { + sweep.start(); + boolean reached = CONNECT_ATTEMPTED.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + Assertions.assertTrue(reached, () -> + "the sweep never reached the driver's connect(): idle=" + ds.getPool().getIdle() + + " size=" + ds.getPool().getSize() + " active=" + ds.getPool().getActive() + + " connects=" + CONNECTS.get() + + " expired=" + pc.isMaxAgeExpired() + " lastConnected=" + pc.getLastConnected() + + " sweepFailure=" + sweepFailure.get()); + awaitSessions(0); + + // the connection under test is claimed: it is out of the idle + // queue and cannot be handed to anybody while it is half + // reconnected + Assertions.assertEquals(0, ds.getPool().getIdle(), + "the connection being reconnected must not stay published in the idle queue"); + + // a borrower therefore waits rather than receiving it: the only + // slot is held by the claim, so waiting is the only move left + AtomicReference<Connection> handed = new AtomicReference<>(); + AtomicReference<Throwable> borrowFailure = new AtomicReference<>(); + Thread borrower = new Thread(() -> { + try { + handed.set(ds.getConnection()); + } catch (Throwable t) { + borrowFailure.set(t); + } + }, "borrower"); + borrower.start(); + try { + awaitUntil(() -> ds.getPool().getWaitCount() == 1); + // pre-fix the borrower would have polled the published + // connection and be inside a second connect() of its own + // right now; here it is parked on the empty queue and the + // driver has seen exactly one reconnect + Assertions.assertEquals(2, CONNECTS.get(), + "only the sweep may connect while the borrower waits"); + + // let the sweep's connect() finish: the claim hands the + // reconnected connection back and the parked borrower wakes + // on it + CONNECT_RELEASE.countDown(); + sweep.join(AWAIT_TIMEOUT_MS); + borrower.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(sweep.isAlive()); + Assertions.assertFalse(borrower.isAlive()); + } finally { + CONNECT_RELEASE.countDown(); + borrower.interrupt(); + borrower.join(AWAIT_TIMEOUT_MS); + } + Assertions.assertNull(sweepFailure.get()); + Assertions.assertNull(borrowFailure.get(), "the borrower was not handed the reconnected connection"); + + // the same pooled connection, now on a fresh physical session: + // the marker set on the old session is gone + Connection again = handed.get(); + Assertions.assertNotNull(again); + Assertions.assertSame(pc, again.unwrap(PooledConnection.class)); + Assertions.assertNull(sessionMarker(again)); + + // and nothing was stranded: two physical connects (fill and + // reconnect) have left exactly one live session + Assertions.assertEquals(2, CONNECTS.get()); + awaitSessions(1); + again.close(); + Assertions.assertEquals(1, ds.getPool().getIdle()); + Assertions.assertEquals(1, ds.getPool().getSize()); + } finally { + CONNECT_RELEASE.countDown(); + sweep.join(AWAIT_TIMEOUT_MS); + } + ds.close(); + awaitSessions(0); + } + + @Test + void aPoolClosedWhileAConnectionIsClaimedDoesNotGetItOfferedBack() throws Exception { + resetDriver(); + DataSource ds = oneSlotUnlockedPool(); + + Connection first = ds.getConnection(); + awaitSessions(1); + PooledConnection pc = (PooledConnection) first.unwrap(PooledConnection.class); + first.close(); + forceExpired(pc); + + // the sweep claims the connection and parks inside its reconnect: + // the claim makes the connection invisible to the shutdown drain + mode = HOLD; + final ConnectionPool pool = ds.getPool(); + AtomicReference<Throwable> sweepFailure = new AtomicReference<>(); + Thread sweep = new Thread(() -> { + try { + pool.testAllIdle(); + } catch (Throwable t) { + sweepFailure.set(t); + } + }, "idle-sweep"); + try { + sweep.start(); + Assertions.assertTrue(CONNECT_ATTEMPTED.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the sweep never reached the driver's connect()"); + Assertions.assertEquals(0, pool.getIdle(), + "the connection being reconnected must be out of the idle queue"); + + // the pool closes over the sweep's window: ordered through the + // latch, close has fully returned - drain included, over an + // empty idle queue - before the reconnect is allowed to finish + ds.close(); + + // a closing pool offers no further item to the idle queue: the + // reconnected connection must be disconnected directly, not + // offered back into a dead queue that nothing will drain again + // - there it would sit as a live session out of every count + CONNECT_RELEASE.countDown(); + sweep.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(sweep.isAlive()); + Assertions.assertNull(sweepFailure.get()); + + Assertions.assertEquals(0, pool.getIdle(), + "a closed pool must not hold a connection back in its idle queue"); + Assertions.assertEquals(0, pool.getSize()); + awaitSessions(0); + } finally { + CONNECT_RELEASE.countDown(); + sweep.join(AWAIT_TIMEOUT_MS); + ds.close(); + } + } + + @Test + void aFailedReconnectReleasesTheClaimedConnectionInsteadOfStrandingIt() throws Exception { + resetDriver(); + DataSource ds = oneSlotUnlockedPool(); + + Connection first = ds.getConnection(); + awaitSessions(1); + PooledConnection pc = (PooledConnection) first.unwrap(PooledConnection.class); + first.close(); + forceExpired(pc); + + mode = FAIL; + ds.getPool().testAllIdle(); + + // the reconnect failed, so the connection was released rather than + // put back: disconnected, out of both queues, and its slot returned + // to the size budget + Assertions.assertEquals(0, ds.getPool().getIdle()); + Assertions.assertEquals(0, ds.getPool().getActive()); + Assertions.assertEquals(0, ds.getPool().getSize()); + awaitSessions(0); + + // the single slot was not lost: the next borrow connects afresh + mode = DIRECT; + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + Assertions.assertNotSame(pc, again.unwrap(PooledConnection.class)); + } + Assertions.assertEquals(1, ds.getPool().getIdle()); + awaitSessions(1); + ds.close(); + awaitSessions(0); + } + + /** + * Driver handing out the real H2 connections, with every connect after + * the first one either parked on the release latch or failed, per the + * currently armed mode. Configured through {@code driverClassName}, so + * it must stay a public static class with a no-arg constructor. + */ + public static class LatchingDriver implements Driver { + + private final Driver delegate = new org.h2.Driver(); + + @Override + public Connection connect(String url, Properties info) throws SQLException { + if (CONNECTS.incrementAndGet() > 1) { + int armed = mode; + if (armed == HOLD) { + CONNECT_ATTEMPTED.countDown(); + try { + CONNECT_RELEASE.await(AWAIT_TIMEOUT_MS * 4, TimeUnit.MILLISECONDS); + } catch (InterruptedException x) { + Thread.currentThread().interrupt(); + throw new SQLException("held connect interrupted", x); + } + } else if (armed == FAIL) { + throw new SQLException("simulated connect failure"); + } + } + return delegate.connect(url, info); + } + + @Override + public boolean acceptsURL(String url) throws SQLException { + return delegate.acceptsURL(url); + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) throws SQLException { + return delegate.getPropertyInfo(url, info); + } + + @Override + public int getMajorVersion() { + return delegate.getMajorVersion(); + } + + @Override + public int getMinorVersion() { + return delegate.getMinorVersion(); + } + + @Override + public boolean jdbcCompliant() { + return delegate.jdbcCompliant(); + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + return delegate.getParentLogger(); + } + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReconnect.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReconnect.java index 42ed0b63f8..1975ed19aa 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReconnect.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReconnect.java @@ -83,8 +83,12 @@ class TestStatementCacheReconnect extends JdbcPoolBaseTest { ds.getPoolProperties().setMaxAge(maxAgeMillis); // keep the background sweeper out of the way: these tests trigger // the reconnect paths themselves (return, borrow, explicit - // testAllIdle) and pin the reconnect count exactly - ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(60000); + // testAllIdle) and pin the reconnect count exactly. A positive + // interval does not achieve that - the sweeper would fire on its + // own during the sleeps and race the counted reconnect (any value + // larger than maxAge is also clamped down to maxAge by + // checkPoolConfiguration) - only switching it off does + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(0); return ds; } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
