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 1e951fcfc333edc91ac0654550c1e91e658cf08b Author: opencode <[email protected]> AuthorDate: Wed Nov 4 09:23:29 2026 +0100 Stop the statistics registration from pinning abandoned pools The live-pool registration that decides when a name's statistics are released held the ConnectionPool instances strongly: a pool the application abandoned without closing - a misuse, but the one case the registration must survive - was retained by a JVM-lifetime static together with everything it reached, its queues, its pooled connections and their open physical sessions, and there is no finaliser net that would close it and deregister itself. The pool's own cleaner already holds its pool weakly, so the strong set was the sole pinning root. Before the live-pool bookkeeping was introduced, the same misuse leaked only the bounded statistics map under the name and the pool stayed collectable; this restores that, against a retention regression. Held weakly. The live-pool set becomes a weak, identity-based set wrapped for internal synchronisation. ConnectionPool does not override equals or hashCode, so membership stays by instance identity, and a pool a caller still holds is never cleared: the close path is untouched in behaviour, because the closing pool is strongly held by the close thread across the removal, the last-live-pool release decision can only be delayed by an uncollected abandoned pool and never advanced wrongly, and the reads that run outside the name's compute - the survivor check of the JMX release and the duplicate-name warning - are safe on the set's own lock. Every caller sits on a pool lifecycle path (start, close, the release behind a close), so the lock never meets a borrow or execution path. Two tests pin the rule: an abandoned pool is collected instead of pinned, and a collected pool can no longer hold its name registered past the close of a same-named survivor. --- .../jdbc/pool/interceptor/SlowQueryReport.java | 37 ++++-- .../TestSlowQueryReportAbandonedPool.java | 135 +++++++++++++++++++++ 2 files changed, 161 insertions(+), 11 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java index e83ba35abd..a4f270c928 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java @@ -24,6 +24,7 @@ import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.TimeZone; +import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import javax.management.openmbean.CompositeDataSupport; @@ -46,7 +47,10 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * The statistics are published under the pool <em>name</em> (see {@link #getPoolStats(String)}), registered when a pool * starts and released when the last live pool registered under that name closes. Pools configured with the same name - * possible with {@code jmxEnabled=false}, which removes the JMX ObjectName clash that would otherwise catch it - - * therefore share one statistics map; closing one of them never takes the shared statistics away from the others. + * therefore share one statistics map; closing one of them never takes the shared statistics away from the others. The + * live-pool registration holds the pool weakly: a pool abandoned without closing is never pinned by it and can be + * collected with its open sessions, leaving only the bounded statistics map under its name until a same-named close + * releases it. * </p> * <p> * More than one reporting definition can be configured on one pool - the two-thresholds configuration of @@ -271,7 +275,8 @@ public class SlowQueryReport extends AbstractQueryReport { /** * Initializes the query statistics map for the given connection pool. * <p> - * The map is registered under the pool name together with the pool instance itself. If a live pool is already + * The map is registered under the pool name together with a weak registration of the pool as one of the live + * pools the name is held for; the registration cannot keep an abandoned pool alive. If a live pool is already * registered under the name, both pools share one statistics map; that merging is the pre-existing behaviour, but * it is a configuration mistake worth a warning, so it is logged. * </p> @@ -543,8 +548,12 @@ public class SlowQueryReport extends AbstractQueryReport { /** * The query statistics published under one pool name, together with the live pools registered for that name. * ConnectionPool does not override {@code equals} or {@code hashCode}, so pool membership is by instance identity. - * Instances are mutated only from within a {@code ConcurrentHashMap.compute} on their name key, which serialises - * them against each other. + * The live pools are held with weak references: a pool the application abandoned without closing is not pinned by + * the registration, and can be collected together with everything it reaches - its queues, pooled connections + * and open physical sessions - the way it could before the live-pool bookkeeping existed. What the registration + * keeps alive is the bounded statistics map under the name, never the pool. The set is internally synchronised, + * which its reads outside the {@code ConcurrentHashMap.compute} on the name key need, and collection drains + * registrations through the same lock, so no external serialisation beyond the set's own lock is required. */ protected static class PoolStatistics { /** @@ -553,9 +562,12 @@ public class SlowQueryReport extends AbstractQueryReport { */ private final ConcurrentHashMap<String, QueryStats> stats = new ConcurrentHashMap<>(); /** - * the live pools registered under the name; the statistics are released once this set runs empty + * the live pools registered under the name; the statistics are released once this set runs empty. Held + * weakly, so a pool that was abandoned without closing drains out when it is garbage collected instead of + * being pinned by the static registration. */ - private final Set<ConnectionPool> livePools = ConcurrentHashMap.newKeySet(); + private final Set<ConnectionPool> livePools = Collections + .synchronizedSet(Collections.newSetFromMap(new WeakHashMap<ConnectionPool, Boolean>())); /** * Registers a pool that started under this name. @@ -567,7 +579,9 @@ public class SlowQueryReport extends AbstractQueryReport { } /** - * Deregisters a pool that closed. + * Deregisters a pool that closed. A same-named pool that was abandoned without closing has already drained + * out of the weakly-held set once it was garbage collected, so it cannot keep the name registered after its + * own session graph is gone. * * @param pool the connection pool that has been closed * @@ -590,10 +604,11 @@ public class SlowQueryReport extends AbstractQueryReport { } /** - * Returns whether the given pool is one of the live pools registered under this name. The live-pool set is - * concurrent, so this plain membership read is safe outside the {@code ConcurrentHashMap.compute} that mutates - * it: it tells a caller, without making a concurrent start wait behind a lock, whether the name is still held - * live by the given pool. + * Returns whether the given pool is one of the live pools registered under this name. The set is internally + * synchronised, so this membership read is safe outside the {@code ConcurrentHashMap.compute} that mutates + * it: it tells a caller, without making a concurrent start wait behind the name's compute lock, whether the + * name is still held live by the given pool. A registration whose pool has been garbage collected drains on + * the next read or write, but a pool a caller still holds is never cleared and reads as live. * * @param pool the pool to look for * diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportAbandonedPool.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportAbandonedPool.java new file mode 100644 index 0000000000..5715508868 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportAbandonedPool.java @@ -0,0 +1,135 @@ +/* + * 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.interceptor; + +import java.lang.ref.WeakReference; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.ConnectionPool; +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.PoolProperties; + +/** + * The per-name statistics registration lists the live pools of the name to + * decide when the statistics - and with them any JMX state - are released, + * but the list must not pin the pools it lists. Held strongly, a pool the + * application abandoned without closing (programmatic misuse, or a crashed + * container that never runs the shutdown) was retained by a JVM-lifetime + * static together with everything it reaches: its queues, its pooled + * connections and their open physical sessions. Before the live-pool + * bookkeeping, the same misuse leaked only the bounded statistics map + * under the name, and the pool stayed collectable. The registrations are + * held weakly again, so an abandoned pool drains out on its own and can + * never hold the name registered past its own collection. + * + * <p>Reproducer for the live-pool retention defect of + * {@code defects-reports/defects-org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.md}.</p> + */ +class TestSlowQueryReportAbandonedPool extends JdbcPoolBaseTest { + + private static ConnectionPool reportedPoolNamed(String name) throws SQLException { + PoolProperties properties = new PoolProperties(); + properties.setUrl(getUrl()); + properties.setDriverClassName("org.h2.Driver"); + properties.setUsername("sa"); + properties.setPassword(""); + // JMX off: the pool's own MBean registered with the platform server + // would be a second strong root, unrelated to the registration under test + properties.setJmxEnabled(false); + properties.setName(name); + properties.setInitialSize(0); + properties.setMinIdle(0); + properties.setJdbcInterceptors( + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport(threshold=-1)"); + return new ConnectionPool(properties); + } + + /* Runs one query through a borrow and returns it; the pool is then + * the only thing left holding the session graph. */ + private static void use(ConnectionPool pool) throws SQLException { + try (Connection pooled = pool.getConnection(); + Statement st = pooled.createStatement()) { + st.execute("SELECT 1"); + } + } + + /* + * Polls until the reference clears. A registration that holds strongly + * never clears, whatever the GC does, so a timeout here is the failure + * signal; a weakly held registration clears at the first full collection. + */ + private static boolean stillReachable(WeakReference<ConnectionPool> ref) throws InterruptedException { + long deadline = System.currentTimeMillis() + AWAIT_TIMEOUT_MS; + while (ref.get() != null && System.currentTimeMillis() < deadline) { + System.gc(); + Thread.sleep(20); + } + return ref.get() != null; + } + + @Test + void anAbandonedPoolIsCollectedInsteadOfPinnedByItsRegistration() throws Exception { + String poolName = "abandoned-" + System.nanoTime(); + ConnectionPool pool = reportedPoolNamed(poolName); + use(pool); + // abandoned: no close() ever runs for this pool + WeakReference<ConnectionPool> registrationProbe = new WeakReference<>(pool); + pool = null; + + Assertions.assertFalse(stillReachable(registrationProbe), + "the static statistics registration must not pin a pool that was" + + " abandoned without closing: the pool drags its queues, its" + + " pooled connections and their open physical sessions with it"); + + // what the registration legitimately keeps is the bounded statistics + Assertions.assertNotNull(SlowQueryReport.getPoolStats(poolName), + "the statistics of the name stay published - they are bounded and the" + + " pool's own data, not the pool itself"); + } + + @Test + void anAbandonedPoolCannotHoldItsNameRegisteredPastItsOwnCollection() throws Exception { + String poolName = "abandoned-heir-" + System.nanoTime(); + ConnectionPool abandoned = reportedPoolNamed(poolName); + use(abandoned); + WeakReference<ConnectionPool> probe = new WeakReference<>(abandoned); + abandoned = null; + Assertions.assertFalse(stillReachable(probe), "precondition: the abandoned pool is collected"); + + // A fresh pool under the name, closed properly, releases the name: + // a collected pool cannot sit in the live set and keep the statistics + // registered forever + DataSource survivor = newDataSource(); + survivor.getPoolProperties().setName(poolName); + survivor.getPoolProperties().setJdbcInterceptors( + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport(threshold=-1)"); + survivor.getPoolProperties().setInitialSize(0); + survivor.createPool(); + use(survivor.getPool()); + survivor.close(); + + Assertions.assertNull(SlowQueryReport.getPoolStats(poolName), + "closing the last pool of the name must release the statistics even" + + " though an abandoned same-named pool preceded it"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
