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 48ba2a96ba612c3db26c6f1f60875ec69d02ad8d Author: opencode <[email protected]> AuthorDate: Thu Nov 5 15:27:57 2026 +0100 Make the lost-store warning atomic SlowQueryReport.closedOrAbandonedReported was a volatile boolean with a non-atomic check-and-set, so two executions that lost the statistics store at the same time could both write the once-per-loss warning. Use an AtomicBoolean compareAndSet for the report and set(false) on recovery, and add a test that races many executions on the same lost store. --- .../jdbc/pool/interceptor/SlowQueryReport.java | 16 ++++----- .../TestSlowQueryReportDeadSessionWarning.java | 41 ++++++++++++++++++++++ 2 files changed, 47 insertions(+), 10 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 576b99905e..1f3554b41f 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.TimeZone; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; import javax.management.openmbean.CompositeDataSupport; import javax.management.openmbean.CompositeType; @@ -111,9 +112,10 @@ public class SlowQueryReport extends AbstractQueryReport { /** * Whether this instance has already reported that it cannot resolve a statistics store. One loss of the store is * one fact about the session this instance serves, so it is reported once; see - * {@link #getQueryStats(String)} for why the report cannot simply be made on every execution. + * {@link #getQueryStats(String)} for why the report cannot simply be made on every execution. The atomic compare + * and set makes the check and the marking one step, so concurrent executions cannot both report the same loss. */ - private volatile boolean closedOrAbandonedReported; + private final AtomicBoolean closedOrAbandonedReported = new AtomicBoolean(); /** * 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. This sizes the statistics @@ -521,11 +523,7 @@ public class SlowQueryReport extends AbstractQueryReport { * reported */ private boolean reportClosedOrAbandoned() { - if (closedOrAbandonedReported) { - return false; - } - closedOrAbandonedReported = true; - return true; + return closedOrAbandonedReported.compareAndSet(false, true); } /** @@ -557,9 +555,7 @@ public class SlowQueryReport extends AbstractQueryReport { // This instance books on a store again, so a later loss is a fresh // event - see reportClosedOrAbandoned(). Guarded so that the common, // healthy path stays a plain read of the flag. - if (closedOrAbandonedReported) { - closedOrAbandonedReported = false; - } + closedOrAbandonedReported.set(false); QueryStats qs = queries.get(sql); if (qs == null) { qs = new QueryStats(sql); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDeadSessionWarning.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDeadSessionWarning.java index 73fdbbd61a..0731fc2cf4 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDeadSessionWarning.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDeadSessionWarning.java @@ -21,6 +21,8 @@ import java.sql.Statement; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Handler; import java.util.logging.Level; import java.util.logging.LogRecord; @@ -162,6 +164,45 @@ class TestSlowQueryReportDeadSessionWarning extends JdbcPoolBaseTest { } } + @Test + void concurrentLossesAreReportedOnce() throws Exception { + DataSource ds = newDataSource(); + ds.setName("DeadSessionConcurrent-" + System.nanoTime()); + ds.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport" + + "(threshold=60000,logSlow=false,logFailed=false)"); + ds.getPoolProperties().setInitialSize(0); + + Connection con = ds.getConnection(); + SlowQueryReport report = reportOf(con); + try (DeadSessionLogCapture logs = new DeadSessionLogCapture()) { + releaseStatisticsOf(report); + + int threads = 16; + CyclicBarrier barrier = new CyclicBarrier(threads); + AtomicReference<Throwable> failure = new AtomicReference<>(); + Thread[] workers = new Thread[threads]; + for (int i = 0; i < threads; i++) { + workers[i] = new Thread(() -> { + try { + barrier.await(); + report.getQueryStats("SELECT concurrent-loss"); + } catch (Throwable t) { + failure.compareAndSet(null, t); + } + }, "dead-session-report-" + i); + workers[i].start(); + } + for (Thread worker : workers) { + worker.join(AWAIT_TIMEOUT_MS); + } + Assertions.assertNull(failure.get(), () -> "the reporters must not fail: " + failure.get()); + logs.assertDeadSessionWarnings(1, + "concurrent executions on a lost store must still report the loss once"); + } finally { + con.close(); + } + } + /** * Counts the WARNs {@link SlowQueryReport} writes for a session whose * statistics store is gone, following the handler idiom of the pool's --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
