This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 1a381e77891da3d5448b0780f5044b2adb03f42e Author: opencode <[email protected]> AuthorDate: Sun Oct 25 23:19:25 2026 +0100 Claim pooled connections before maintenance actions on them The checkAbandoned(), checkIdle() and testAllIdle() methods guard their mutations with the per-connection lock, which is disabled by default and only implicitly enabled together with the pool sweeper. Invoked through the public or JMX API on a running pool with the sweeper switched off, the check-then-act sequences raced with borrowers, which are unlocked in that configuration as well; a sweep could release a connection a borrower had just taken out of the idle queue, and every later use of it then failed with "A connection once released, can't be reestablished". The methods now first claim a connection atomically from the queue that publishes it and skip it when the claim fails. Forcing the lock on the maintenance side instead would not close the race, because the queue lock is the only mutual exclusion both the maintenance thread and the borrower always honour. The remaining, transient overlap between an unlocked external testAllIdle() and a concurrent borrower is documented. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 32 +++++- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 7 +- .../jdbc/pool/TestUnlockedMaintenanceSweep.java | 124 +++++++++++++++++++++ 3 files changed, 159 insertions(+), 4 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 f598e098ec..bb91775617 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 @@ -1207,7 +1207,13 @@ public class ConnectionPool { long time = con.getTimestamp(); long now = System.currentTimeMillis(); if (shouldAbandon() && (now - time) > con.getAbandonTimeout()) { - locked.remove(); + // claim the connection before acting on it; without + // a real lock the checks above may already be stale, + // and a connection the application returned to the + // pool in the meantime must not be abandoned + if (!busy.remove(con)) { + continue; + } abandon(con); setToNull = true; } else if (sto > 0 && (now - time) > (sto * 1000L)) { @@ -1265,9 +1271,15 @@ public class ConnectionPool { } long time = con.getTimestamp(); if (shouldReleaseIdle(now, con, time)) { + // claim the connection before acting on it; without + // a real lock the checks 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; + } releasedIdleCount.incrementAndGet(); release(con); - unlocked.remove(); setToNull = true; } else { //do nothing @@ -1308,6 +1320,14 @@ 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. */ public void testAllIdle() { testAllIdle(false); @@ -1340,8 +1360,14 @@ public class ConnectionPool { release = !reconnectIfExpired(con) || !con.validate(PooledConnection.VALIDATE_IDLE); } 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; + } releasedIdleCount.incrementAndGet(); - unlocked.remove(); release(con); } } finally { 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 487a744cef..e37aae2022 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 @@ -712,7 +712,12 @@ public class DataSourceProxy implements PoolConfiguration { } /** - * Performs a validation on idle connections + * 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 + * {@link ConnectionPool#testAllIdle()}. */ public void testIdle() { try { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestUnlockedMaintenanceSweep.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestUnlockedMaintenanceSweep.java new file mode 100644 index 0000000000..46a2ecc88f --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestUnlockedMaintenanceSweep.java @@ -0,0 +1,124 @@ +/* + * 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.sql.Connection; +import java.sql.Statement; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * Exercises the externally-invoked idle maintenance (the documented + * {@link DataSourceProxy#checkIdle()} API) on a running pool while the + * per-connection lock is disabled, which is the default configuration when + * the pool sweeper is switched off. Without a real lock, the maintenance + * methods guard their mutations with checks that a concurrent borrower can + * invalidate at any time. Before the fix, the sweep could release a + * connection that a borrower had already taken out of the idle queue, and + * the borrower's first statement on it then failed with + * "A connection once released, can't be reestablished". + */ +public class TestUnlockedMaintenanceSweep extends JdbcPoolBaseTest { + + private static final long RUN_MILLIS = 6000; + private static final int BORROWERS = 4; + + @Test + public void unlockedIdleSweepMustNotReleaseConnectionsAlreadyBorrowed() + throws Exception { + DataSource ds = newDataSource(); + PoolConfiguration p = ds.getPoolProperties(); + // the configuration under test: no sweeper, no locking + p.setTimeBetweenEvictionRunsMillis(0); + p.setUseLock(false); + p.setMaxActive(8); + p.setInitialSize(8); + p.setMinIdle(0); + // every returned connection is immediately old enough to evict, so + // the sweep is constantly trying to release connections + p.setMinEvictableIdleTimeMillis(1); + p.setTestWhileIdle(false); + p.setRemoveAbandoned(false); + p.setSuspectTimeout(-1); + p.setMaxAge(0); + p.setValidationQuery("SELECT 1"); + p.setTestOnBorrow(true); + ds.createPool(); + // let the initial connections age out + sleep(200); + + AtomicBoolean stop = new AtomicBoolean(); + AtomicLong uses = new AtomicLong(); + AtomicLong errors = new AtomicLong(); + AtomicReference<Exception> firstError = new AtomicReference<>(); + + Thread sweeper = new Thread(() -> { + while (!stop.get()) { + ds.checkIdle(); + } + }, "sweep-under-test"); + Runnable borrowerBody = () -> { + while (!stop.get()) { + try (Connection c = ds.getConnection(); + Statement st = c.createStatement()) { + st.execute("SELECT 1"); + uses.incrementAndGet(); + } catch (Exception x) { + firstError.compareAndSet(null, x); + errors.incrementAndGet(); + } + } + }; + Thread[] borrowers = new Thread[BORROWERS]; + for (int i = 0; i < BORROWERS; i++) { + borrowers[i] = new Thread(borrowerBody, "borrower-" + i); + } + + try { + sweeper.start(); + for (Thread b : borrowers) { + b.start(); + } + sleep(RUN_MILLIS); + stop.set(true); + sweeper.join(); + for (Thread b : borrowers) { + b.join(); + } + awaitUntil(() -> ds.getActive() == 0); + + assertTrue(ds.getReleasedIdleCount() > 0, + "the sweep must actually have released aged connections, " + + "otherwise this test exercises nothing"); + assertEquals(0, errors.get(), () -> "the unlocked idle sweep broke " + + errors.get() + " of " + uses.get() + + " successful borrows, first failure: " + firstError.get()); + assertEquals(ds.getSize(), ds.getIdle(), + "the idle sweep must not strand connections outside both queues"); + } finally { + stop.set(true); + ds.close(true); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
