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 b7e459cb3334080c9d2fc89a5b8c24d33a6cf7dc Author: opencode <[email protected]> AuthorDate: Tue Oct 27 18:22:08 2026 +0100 Release SlowQueryReport stats only when last same-named pool closes SlowQueryReport keyed its static per-pool statistics by pool name and poolClosed removed the name's entry unconditionally. Pool names are user-settable - only programmatically, since the JNDI factory does not accept a name property and JNDI pools always carry the generated unique default - so two simultaneously live pools sharing a label (or a second pool created before the first was closed) share one statistics map, and closing either one silently destroyed the other's live statistics forever: the per-connection interceptors re-resolve their map by name in reset() on the next borrow, got null, and every later query logged "Connection has already been closed or abandoned" while all statistics for the healthy pool were lost, as poolStarted never runs again. A per-instance guard in poolClosed cannot work: ConnectionPool delivers poolClosed - and poolStarted at init - to freshly instantiated throwaway interceptors, so the receiving instance has never seen the map. The only identity available is the ConnectionPool argument. Fix: the per-name entry now holds the shared statistics map together with the identity set of live pools registered under that name (ConnectionPool does not override equals/hashCode). poolStarted registers the pool under a single ConcurrentHashMap.compute per name and warns when another live pool already owns the name, keeping - but making explicit - the pre-existing merged-statistics semantics for duplicate names; poolClosed de-registers the closing pool and releases the entry only when the last same-named live pool has closed. Name-keyed lookups (getPoolStats, JMX getPoolNames/getSlowQueriesCD/resetStats, the notification registry) keep their exact behavior; the single-pool close still releases the statistics. The type of the protected static perPoolStats value changes accordingly, an internal detail with no consumers in the module. Regression tests in TestSlowQueryReportDuplicatePoolNames pin that closing one of two live same-named pools neither removes the published statistics nor stops the healthy pool from collecting, that the last close releases the map, and that a pool started under a freed name collects into a fresh map. The first fails on the pre-fix code (getPoolStats returns null after the other pool's close). --- .../jdbc/pool/interceptor/SlowQueryReport.java | 135 ++++++++++++++++++--- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 4 +- .../TestSlowQueryReportDuplicatePoolNames.java | 133 ++++++++++++++++++++ 3 files changed, 251 insertions(+), 21 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 b48d8f02c7..46d6ebcaea 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 @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.TimeZone; import java.util.concurrent.ConcurrentHashMap; @@ -39,15 +40,24 @@ import org.apache.tomcat.jdbc.pool.PooledConnection; /** * Slow query report interceptor. Tracks timing of query executions. + * <p> + * 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.</p> */ public class SlowQueryReport extends AbstractQueryReport { //logger private static final Log log = LogFactory.getLog(SlowQueryReport.class); /** - * we will be keeping track of query stats on a per pool basis + * we will be keeping track of query stats on a per pool basis, keyed + * by pool name; pools registered under one name share the statistics */ - protected static final ConcurrentHashMap<String,ConcurrentHashMap<String,QueryStats>> perPoolStats = + protected static final ConcurrentHashMap<String,PoolStatistics> perPoolStats = new ConcurrentHashMap<>(); /** * the queries that are used for this interceptor. @@ -80,10 +90,13 @@ public class SlowQueryReport extends AbstractQueryReport { * @param poolname - the name of the pool we want to retrieve stats for * @return a hash map containing the statistics of the monitored queries, * kept near {@code maxQueries} entries by eviction, or - * {@code null} if the pool is not being monitored + * {@code null} if no live pool is registered under that name. + * Pools registered under one name share the map, and it is + * published until the last of them has closed */ public static ConcurrentHashMap<String,QueryStats> getPoolStats(String poolname) { - return perPoolStats.get(poolname); + PoolStatistics stats = perPoolStats.get(poolname); + return stats == null ? null : stats.getQueryStats(); } /** @@ -190,34 +203,52 @@ 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 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> * * @param pool the connection pool that has started */ @Override public void poolStarted(ConnectionPool pool) { super.poolStarted(pool); - //see if we already created a map for this pool - queries = perPoolStats.get(pool.getName()); - if (queries==null) { - //create the map to hold our stats - //however TODO we need to improve the eviction - //selection - queries = new ConcurrentHashMap<>(); - if (perPoolStats.putIfAbsent(pool.getName(), queries)!=null) { - //there already was one - queries = perPoolStats.get(pool.getName()); - } - } + //see if we already created a map for this pool - and register + //this pool as one of the live pools the map is held for + PoolStatistics stats = perPoolStats.compute(pool.getName(), + (name, existing) -> { + PoolStatistics registration = + (existing == null) ? new PoolStatistics() : existing; + registration.addLivePool(pool); + return registration; + }); + if (stats.hasOtherLivePools() && log.isWarnEnabled()) { + log.warn("Connection pool name [" + pool.getName() + "] is already in use by " + + "another live pool; SlowQueryReport statistics are keyed by pool name, " + + "so both pools report into the same statistics map."); + } + queries = stats.getQueryStats(); } /** - * Removes the query statistics map for the given connection pool. + * Releases the query statistics map for the given connection pool, + * unless another live pool is still registered under its name. + * <p> + * The callback is delivered by {@link ConnectionPool} to a freshly + * instantiated interceptor (see {@code ConnectionPool#close(boolean)}), + * which has never held a reference to the statistics, so the map to + * release can only be identified through the closing pool passed + * here, not through this instance's own state. The statistics stay + * published as long as one pool registered under the name is alive.</p> * * @param pool the connection pool that has been closed */ @Override public void poolClosed(ConnectionPool pool) { - perPoolStats.remove(pool.getName()); + perPoolStats.computeIfPresent(pool.getName(), + (name, existing) -> existing.removeLivePool(pool) ? null : existing); super.poolClosed(pool); } @@ -302,7 +333,7 @@ public class SlowQueryReport extends AbstractQueryReport { public void reset(ConnectionPool parent, PooledConnection con) { super.reset(parent, con); if (parent!=null) { - queries = perPoolStats.get(parent.getName()); + queries = getPoolStats(parent.getName()); } else { queries = null; } @@ -377,6 +408,72 @@ 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. + */ + protected static class PoolStatistics { + /** + * the statistics map shared by all pools registered under the + * name, kept near {@code maxQueries} entries by eviction + */ + private final ConcurrentHashMap<String,QueryStats> stats = + new ConcurrentHashMap<>(); + /** + * the live pools registered under the name; the statistics are + * released once this set runs empty + */ + private final Set<ConnectionPool> livePools = + ConcurrentHashMap.newKeySet(); + + /** + * Registers a pool that started under this name. + * + * @param pool the connection pool that has started + */ + protected void addLivePool(ConnectionPool pool) { + livePools.add(pool); + } + + /** + * Deregisters a pool that closed. + * + * @param pool the connection pool that has been closed + * @return {@code true} if this was the last live pool registered + * under the name, so the statistics can be released + */ + protected boolean removeLivePool(ConnectionPool pool) { + livePools.remove(pool); + return livePools.isEmpty(); + } + + /** + * Returns whether pools other than the caller's own are + * registered under this name. The caller registers its own pool + * before asking, so more than one registration means another + * live pool shares the name. + * + * @return {@code true} if another live pool shares this name + */ + protected boolean hasOtherLivePools() { + return livePools.size() > 1; + } + + /** + * Returns the statistics map published under this name. + * + * @return the shared query statistics map + */ + protected ConcurrentHashMap<String,QueryStats> getQueryStats() { + return stats; + } + } + + /** * Holds timing and invocation statistics for a single SQL query. */ diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java index f44c209590..ca184fd995 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java @@ -352,7 +352,7 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * JMX operation - remove all stats for this connection pool */ public void resetStats() { - ConcurrentHashMap<String,QueryStats> queries = perPoolStats.get(poolName); + ConcurrentHashMap<String,QueryStats> queries = getPoolStats(poolName); if (queries!=null) { Iterator<String> it = queries.keySet().iterator(); while (it.hasNext()) { @@ -368,7 +368,7 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE @Override public CompositeData[] getSlowQueriesCD() throws OpenDataException { CompositeDataSupport[] result = null; - ConcurrentHashMap<String,QueryStats> queries = perPoolStats.get(poolName); + ConcurrentHashMap<String,QueryStats> queries = getPoolStats(poolName); if (queries!=null) { Set<Map.Entry<String,QueryStats>> stats = queries.entrySet(); if (stats!=null) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDuplicatePoolNames.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDuplicatePoolNames.java new file mode 100644 index 0000000000..04f73957e9 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDuplicatePoolNames.java @@ -0,0 +1,133 @@ +/* + * 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.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.Collections; +import java.util.Map; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * The {@code SlowQueryReport} publishes its statistics under the pool + * <em>name</em>, so pools configured with the same name (possible with + * {@code jmxEnabled=false}, which removes the JMX ObjectName clash that + * would otherwise catch it) share one statistics map. Closing one of + * such pools must not destroy the statistics of the other one: the + * statistics belong to the name only as long as a live pool uses it and + * are released with the last one. + * + * <p>Reproducer for defect 2 of + * {@code defects-pool/defects-org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.md}.</p> + */ +class TestSlowQueryReportDuplicatePoolNames extends JdbcPoolBaseTest { + + private static final String QUERY_FIRST = "SELECT COUNT(*) FROM SYSTEM_RANGE(1,3)"; + private static final String QUERY_SECOND = "SELECT COUNT(*) FROM SYSTEM_RANGE(1,5)"; + private static final String QUERY_AFTER_CLOSE = "SELECT COUNT(*) FROM SYSTEM_RANGE(1,7)"; + + /* A pool of the given name reporting every query it sees. */ + private DataSource reportedPoolNamed(String name) { + DataSource ds = newDataSource(); // JMX disabled, as a same-named + // second pool needs to dodge the JMX ObjectName clash + ds.getPoolProperties().setName(name); + ds.getPoolProperties().setJdbcInterceptors("SlowQueryReport(threshold=-1)"); + return ds; + } + + private static void run(DataSource ds, String sql) throws SQLException { + try (Connection pooled = ds.getConnection(); + Statement st = pooled.createStatement()) { + st.execute(sql); + } + } + + @Test + void closingOneOfTwoPoolsOfTheSameNameKeepsTheOtherStatistics() throws Exception { + String poolName = "dup-" + System.nanoTime(); + DataSource first = reportedPoolNamed(poolName); + DataSource second = reportedPoolNamed(poolName); + first.createPool(); + second.createPool(); + + run(first, QUERY_FIRST); + run(second, QUERY_SECOND); + + Map<String, SlowQueryReport.QueryStats> shared = + SlowQueryReport.getPoolStats(poolName); + Assertions.assertNotNull(shared, + "the running pools must publish statistics under their name"); + // duplicate names share one map: the merging is the documented, + // pre-existing behaviour - what must never happen is the map + // disappearing while a pool of that name still runs + Assertions.assertTrue(shared.containsKey(QUERY_FIRST) && shared.containsKey(QUERY_SECOND), + "both pools must book into the shared map, saw " + shared.keySet()); + + first.close(); + + Assertions.assertNotNull(SlowQueryReport.getPoolStats(poolName), + "closing one of two live pools of the same name must not throw the " + + "still-running pool's statistics away"); + + run(second, QUERY_AFTER_CLOSE); + + Map<String, SlowQueryReport.QueryStats> surviving = + SlowQueryReport.getPoolStats(poolName); + Assertions.assertNotNull(surviving, + "the surviving pool must keep collecting after the other pool closed"); + Assertions.assertTrue(surviving.containsKey(QUERY_AFTER_CLOSE), + "a query run after the other pool closed must be collected, saw " + + surviving.keySet()); + Assertions.assertEquals(1, surviving.get(QUERY_FIRST).getNrOfInvocations(), + "the statistics booked before the other pool closed must survive"); + + second.close(); + + Assertions.assertNull(SlowQueryReport.getPoolStats(poolName), + "closing the last live pool of a name must release its statistics"); + } + + @Test + void aPoolStartedUnderAFreedNameCollectsIntoAFreshMap() throws Exception { + String poolName = "dup-" + System.nanoTime(); + DataSource first = reportedPoolNamed(poolName); + first.createPool(); + run(first, QUERY_FIRST); + first.close(); + Assertions.assertNull(SlowQueryReport.getPoolStats(poolName), + "closing the only pool of a name must release the statistics"); + + DataSource second = reportedPoolNamed(poolName); + second.createPool(); + run(second, QUERY_SECOND); + + Map<String, SlowQueryReport.QueryStats> stats = + SlowQueryReport.getPoolStats(poolName); + Assertions.assertNotNull(stats, "the restarted name must be monitored again"); + Assertions.assertEquals(Collections.singleton(QUERY_SECOND), stats.keySet(), + "a pool started after the name was freed must collect into a fresh map, " + + "not inherit the closed pool's statistics"); + + second.close(); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
