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 e69bbf232325d13516ba3b111a6b40a841753ea4 Author: opencode <[email protected]> AuthorDate: Mon Oct 26 10:23:56 2026 +0100 Stop query-report eviction from throwing at its caller getPoolStats() read eviction state that addSql() mutates without synchronisation, so the eviction loop could index off the end of its snapshot and raise IndexOutOfBoundsException into the caller, and the sort could observe an entry changing underneath it. Each report now sorts its own atomic snapshot and the eviction loop is bounded by it. --- .../jdbc/pool/interceptor/SlowQueryReport.java | 54 +++++- .../pool/interceptor/TestSlowQueryEviction.java | 188 +++++++++++++++++++++ 2 files changed, 234 insertions(+), 8 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 55b2121a25..368311b53d 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 @@ -54,7 +54,9 @@ public class SlowQueryReport extends AbstractQueryReport { */ protected volatile ConcurrentHashMap<String,QueryStats> queries = null; /** - * Maximum number of queries we will be storing + * Maximum number of queries we will be storing statistics for. Eviction + * keeps the number of stored queries near this amount; while queries are + * being executed concurrently it can briefly be exceeded. */ protected int maxQueries= 1000; //don't store more than this amount of queries @@ -76,7 +78,9 @@ public class SlowQueryReport extends AbstractQueryReport { /** * Returns the query stats for a given pool * @param poolname - the name of the pool we want to retrieve stats for - * @return a hash map containing statistics for 0 to maxQueries + * @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 */ public static ConcurrentHashMap<String,QueryStats> getPoolStats(String poolname) { return perPoolStats.get(poolname); @@ -250,20 +254,40 @@ public class SlowQueryReport extends AbstractQueryReport { } /** - * Sort QueryStats by last invocation time + * Evicts the least recently invoked query statistics until the map is + * back within {@code maxQueries} entries. The map is shared by all the + * connections of the pool, so the eviction runs concurrently with the + * inserts and removals of the other borrower threads and can only do as + * much work as its own snapshot allows; it then leaves the map above + * capacity for the next new query to evict further. + * * @param queries The queries map */ protected void removeOldest(ConcurrentHashMap<String,QueryStats> queries) { - ArrayList<QueryStats> list = new ArrayList<>(queries.values()); - Collections.sort(list, queryStatsComparator); + // Sort a snapshot of the statistics rather than the live entries: + // the invocation times are updated concurrently, and a comparator + // that reads mutable state may violate its own contract while the + // sort is in progress. + ArrayList<QueryStats> snapshot = new ArrayList<>(queries.size()); + for (QueryStats qs : queries.values()) { + snapshot.add(qs.snapshotForEviction()); + } + Collections.sort(snapshot, queryStatsComparator); + // The map is shared by all connections of the pool, so it may have + // grown or shrunk since the snapshot was taken. Entries that another + // thread removed already make our removal a no-op, so the snapshot + // can run out while the map is still over capacity. Eviction then + // gives up rather than running past the end of the list, leaving the + // map over capacity by about as many queries as were being inserted + // concurrently; the next new query evicts further. int removeIndex = 0; - while (queries.size() > maxQueries) { - String sql = list.get(removeIndex).getQuery(); + while (queries.size() > maxQueries && removeIndex < snapshot.size()) { + String sql = snapshot.get(removeIndex).getQuery(); + removeIndex++; queries.remove(sql); if (log.isDebugEnabled()) { log.debug("Removing slow query, capacity reached:"+sql); } - removeIndex++; } } @@ -502,6 +526,20 @@ public class SlowQueryReport extends AbstractQueryReport { this.query = query; } + /** + * Returns a snapshot of the data that orders this query for eviction. + * Only the query and the time of its last invocation are carried + * over: the snapshot exists to be sorted, and unlike the live + * statistics its ordering does not change while a sort is running. + * + * @return a snapshot of this query and its last invocation time + */ + private QueryStats snapshotForEviction() { + QueryStats result = new QueryStats(query); + result.lastInvocation = lastInvocation; + return result; + } + /** * Records the time taken to prepare this query. * diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryEviction.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryEviction.java new file mode 100644 index 0000000000..5d58ed252f --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryEviction.java @@ -0,0 +1,188 @@ +/* + * 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.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.QueryStats; + +/** + * Eviction from the pool-wide query statistics map. The map is shared by + * every connection of a pool, so an eviction races with the inserts and + * removals of the other borrower threads: the map can still be over + * capacity once the eviction has worked through its own snapshot. Running + * past the end of the snapshot used to throw an + * {@code IndexOutOfBoundsException} into the reporting code, which reaches + * the application through the unguarded report call site - for a query that + * had already been executed successfully. + */ +class TestSlowQueryEviction extends JdbcPoolBaseTest { + + /* + * Stands in for the other borrower threads: every entry the eviction + * removes is replaced by a freshly inserted one, so the map is still + * over capacity once the snapshot the eviction works through has been + * exhausted. + */ + private static final class GrowingMap extends ConcurrentHashMap<String, QueryStats> { + private static final long serialVersionUID = 1L; + private final AtomicInteger inserts = new AtomicInteger(); + private final int limit; + + GrowingMap(int inserts) { + this.limit = inserts; + } + + @Override + public QueryStats remove(Object key) { + QueryStats removed = super.remove(key); + int inserted = inserts.getAndIncrement(); + if (inserted < limit) { + String sql = "raced-" + inserted; + QueryStats stats = new QueryStats(sql); + stats.add(1, 10000 + inserted); + put(sql, stats); + } + return removed; + } + } + + @Test + void evictionGivesUpOnceItsSnapshotIsWorkedThrough() { + SlowQueryReport report = new SlowQueryReport(); + report.setMaxQueries(4); + GrowingMap stats = new GrowingMap(10); + for (int i = 0; i < 5; i++) { + QueryStats entry = new QueryStats("sql-" + i); + entry.add(1, 1000 + i); + stats.put("sql-" + i, entry); + } + + assertDoesNotThrow(() -> report.removeOldest(stats), + "an eviction that cannot catch up must not fail the caller"); + assertTrue(stats.size() <= 15, + "the map must stay bounded, was " + stats.size()); + } + + @Test + void evictionRemovesTheLeastRecentlyInvokedQueries() { + SlowQueryReport report = new SlowQueryReport(); + report.setMaxQueries(2); + ConcurrentHashMap<String, QueryStats> stats = new ConcurrentHashMap<>(); + QueryStats oldStats = new QueryStats("old"); + oldStats.add(1, 1000); + QueryStats recentStats = new QueryStats("recent"); + recentStats.add(1, 2000); + QueryStats neverUsed = new QueryStats("never-used"); + stats.put("old", oldStats); + stats.put("recent", recentStats); + stats.put("never-used", neverUsed); + + report.removeOldest(stats); + + assertEquals(2, stats.size(), "down to the configured maximum"); + assertTrue(stats.containsKey("never-used"), + "a query that was never invoked counts as the newest of all"); + assertTrue(stats.containsKey("recent"), "the more recent one survives"); + assertFalse(stats.containsKey("old"), "the least recent one is evicted"); + } + + /* + * The eviction path as the borrowers actually use it: the statistics + * collector of a running pool, hammered with distinct SQL from several + * threads at a tiny capacity so that evictions overlap constantly. The + * statistics are the pool's own, taken from the interceptor chain of a + * borrowed connection. + */ + @Test + void concurrentEvictionNeverThrowsOutOfTheStatisticsCollector() + throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport" + + "(maxQueries=16)"); + ds.createPool(); + String poolName = ds.getPoolProperties().getPoolName(); + + int threads = 48; + int perThread = 1000; + ConcurrentHashMap<String, QueryStats> stats; + ExecutorService pool = Executors.newFixedThreadPool(threads); + CopyOnWriteArrayList<Throwable> failures = new CopyOnWriteArrayList<>(); + CountDownLatch ready = new CountDownLatch(threads); + CountDownLatch start = new CountDownLatch(1); + SlowQueryReport report; + try (Connection c = ds.getConnection()) { + report = interceptorOf(c, SlowQueryReport.class); + assertNotNull(report, "the interceptor must be in the chain"); + for (int t = 0; t < threads; t++) { + int id = t; + pool.execute(() -> { + ready.countDown(); + try { + start.await(); + for (int i = 0; i < perThread; i++) { + report.getQueryStats("SELECT 1 /* " + id + '-' + i + + " */"); + } + } catch (Throwable e) { + failures.add(e); + } + }); + } + ready.await(); + start.countDown(); + pool.shutdown(); + assertTrue(pool.awaitTermination(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the borrowers must not wedge each other"); + } finally { + pool.shutdownNow(); + } + + if (!failures.isEmpty()) { + Throwable first = failures.get(0); + throw new AssertionError("collecting query statistics must never " + + "fail the caller, " + failures.size() + " thread(s) " + + "failed, first: " + first, first); + } + int size = SlowQueryReport.getPoolStats(poolName).size(); + // Eviction can only do as much work as its own snapshot allows, so a + // burst of concurrent inserts may leave the map over capacity, and two + // evictions that both see one entry too many may take it below. What + // has to hold is that the map neither runs away nor is drained; the + // margins below are comfortably wide rather than derived limits. + assertTrue(size <= 16 + threads, + "the map must stay near its configured capacity, was " + size); + assertTrue(size > 0, "the map must still hold statistics, was " + size); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
