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 7ad92c6327f78196dbf2e5098c877f45b534a548 Author: opencode <[email protected]> AuthorDate: Wed Nov 4 08:54:18 2026 +0100 Release a chained report definition's JMX state on close SlowQueryReportJmx releases the MBean and notification broadcaster published for its pool name only behind the lastPoolOfNameClosed signal, which SlowQueryReport.poolClosed sets from releaseStatistics. Pool closure delivers the close to one freshly instantiated interceptor per configured definition, in declaration order, and releaseStatistics hands its true return to whichever definition runs first. With a base SlowQueryReport definition configured above the JMX one - a legal jdbcInterceptors string - the base definition consumed the signal, its definition has no use for it, and the definition that owns the JMX state saw false and skipped the release: closing the only pool of the name left the MBean registered forever, delegating to a dead instance, the broadcaster's listeners never released, and a later same-named pool start refused with InstanceAlreadyExists. Reproduced with a real pool: registered after close with the statistics already gone. Releasing on the statistics being gone. The consumer of the signal can not rely on receiving it, so the JMX definition now also releases when the name's statistics are no longer registered - the statistics are freed exactly when the last live pool of the name deregisters, which is the same fact seen from the other side. releaseJmxRegistration already re-verifies the name before taking anything down, so a pool that started under the name between the removal and this check has taken the registration back and keeps it, and a same-named survivor keeps the shared registration: closing one of two chained same-named pools still leaves the MBean and the statistics published until the last one closes. The lastPoolOfNameClosed javadoc now states that exactly one definition per close sees the signal and that this need not be the definition able to act on it. --- .../jdbc/pool/interceptor/SlowQueryReport.java | 7 +- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 36 +++-- .../TestSlowQueryJmxChainCloseRelease.java | 152 +++++++++++++++++++++ 3 files changed, 181 insertions(+), 14 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 6bc2ed6ec5..f45527bddc 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 @@ -283,7 +283,12 @@ public class SlowQueryReport extends AbstractQueryReport { * last live pool under its name, meaning everything else registered for that name - the interceptor's MBean and * broadcaster, for instance - can and must now be released with it. The deciding * {@link #releaseStatistics(ConnectionPool)} call is atomic, so across concurrent closes of same-named pools and - * across the interceptor definitions of one close exactly one caller ever sees {@code true}. + * across the interceptor definitions of one close exactly one caller ever sees {@code true}. Which one that is + * follows only the declaration order of the definitions: {@code ConnectionPool.close()} delivers the callback to + * a freshly instantiated interceptor per definition, and the first one consumes the signal, so the definition + * that sees {@code true} need not be the one that can act on it - the holder of the name's JMX state, for + * instance, sees {@code false} when a base definition is declared above it and must read the gone statistics as + * the same signal (see {@link SlowQueryReportJmx#poolClosed(ConnectionPool)}). */ protected boolean lastPoolOfNameClosed = false; 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 f79c457120..bd044d4f0c 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 @@ -257,17 +257,24 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE /** * Called when the connection pool is closed; releases the JMX registration of the pool's name - the MBean and the - * broadcaster its clients' listeners are attached to - only once that pool was the last live pool registered under - * the name. Both belong to the name and are shared by every live pool of that name, so taking them down while a - * survivor is still running would orphan the survivor's monitoring; releasing them on every close but a decided - * last one would unregister a shared registration that other closing or live pools still rely on. Whether this + * broadcaster its clients' listeners are attached to - once no live pool is registered under the name any more. + * Both belong to the name and are shared by every live pool of that name, so taking them down while a + * survivor is still running would orphan the survivor's monitoring; releasing them while a survivor still runs + * would unregister a shared registration that other closing or live pools still rely on. Whether this * close is the last one is decided atomically by {@link SlowQueryReport#releaseStatistics(ConnectionPool)} on the * way through {@code super.poolClosed}, so concurrent closes of same-named pools cannot all see a survivor and * leave the name registered for ever. * <p> - * The decision and the release are separate steps, and a pool that starts under the name between them has taken - * the name - and the registration still published under it - back; {@link #releaseJmxRegistration(ConnectionPool)} - * therefore re-verifies the name before anything is taken down. + * {@code ConnectionPool.close()} delivers this callback to a freshly instantiated interceptor per configured + * definition, in declaration order, and {@code releaseStatistics} hands the {@code true} signal to whichever + * definition happens to run first - which need not be this one. A chain of a base {@link SlowQueryReport} + * definition above this one therefore has the base consume the signal and this definition see {@code false} + * although the statistics, and with them the last live pool of the name, are already gone. The statistics being + * gone is the same signal seen from the other side, so it releases as well; what remains safe is that the + * release only ever takes down a registration no live pool of the name holds, because + * {@link #releaseJmxRegistration(ConnectionPool)} re-verifies the name before anything is taken down - a pool + * that started under the name between the statistics removal and this check has taken the registration back and + * owns it from then on. * </p> * * @param pool the connection pool being closed @@ -276,12 +283,15 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE public void poolClosed(ConnectionPool pool) { this.poolName = pool.getName(); super.poolClosed(pool); - if (lastPoolOfNameClosed) { - // The statistics of the name were freed by this very close, - // so no live pool is registered under it any more - and the - // registered MBean, whose delegate may be an interceptor - // instance of an already closed pool, goes with them, unless - // a pool has taken the name back in the meantime + // Either this instance's releaseStatistics call deregistered the + // closing pool as the last live pool of the name, or an earlier + // definition of this very close consumed that signal and the name's + // statistics are already gone - which means no live pool is + // registered under it either. In both cases the registered MBean, + // whose delegate may be an interceptor instance of an already closed + // pool, goes with them, unless a pool has taken the name back in the + // meantime - which releaseJmxRegistration re-verifies. + if (lastPoolOfNameClosed || getPoolStats(poolName) == null) { releaseJmxRegistration(pool); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxChainCloseRelease.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxChainCloseRelease.java new file mode 100644 index 0000000000..3e8a1f9012 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxChainCloseRelease.java @@ -0,0 +1,152 @@ +/* + * 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.management.ManagementFactory; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; + +import javax.management.MBeanServer; +import javax.management.ObjectName; + +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 SlowQueryReportJmx} interceptor publishes its MBean and + * notification broadcaster for the pool <em>name</em> and must take them + * down once the last live pool of that name closes. Pool closure delivers + * the close to one freshly instantiated interceptor per configured + * definition, in declaration order, and the statistics release hands its + * "last pool of the name closed" signal to whichever definition runs + * first - which a base {@link SlowQueryReport} definition above the JMX + * one consumes before the definition that owns the JMX state ever sees it. + * The JMX definition must therefore read the name's freed statistics as + * the same signal, or the registration of a pool that is already gone + * stays published forever, pinning the dead delegate and blocking a later + * same-named pool from registering. + * + * <p>Reproducer for the release-signal ordering defect of + * {@code defects-reports/defects-org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.md}.</p> + */ +class TestSlowQueryJmxChainCloseRelease extends JdbcPoolBaseTest { + + private static final String SQR = + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport"; + private static final String SQRJ = + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx"; + + /* A pool of the given name with the given chain, JMX off for the pool + * itself so same-named pools dodge the pool MBean ObjectName clash. */ + private DataSource poolNamed(String name, String interceptors) { + DataSource ds = newDataSource(); + ds.getPoolProperties().setName(name); + ds.getPoolProperties().setJdbcInterceptors(interceptors); + ds.getPoolProperties().setInitialSize(0); + return ds; + } + + private static void run(DataSource ds, String sql) throws SQLException { + try (Connection pooled = ds.getConnection(); + Statement st = pooled.createStatement()) { + st.execute(sql); + } + } + + private static ObjectName interceptorName(String poolName) throws Exception { + return new ObjectName("tomcat.jdbc:type=" + SlowQueryReportJmx.class.getName() + + ",name=" + poolName); + } + + @Test + void closingTheOnlyPoolReleasesJmxBehindABaseReportDefinition() throws Exception { + // The order that used to leak: the base definition runs first on + // close, consumes the last-pool-of-name signal and the JMX + // definition below it must still release from the freed statistics + String poolName = "chain-base-first-" + System.nanoTime(); + DataSource ds = poolNamed(poolName, SQR + ";" + SQRJ + "(notifyPool=false)"); + ds.createPool(); + run(ds, "SELECT 1"); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = interceptorName(poolName); + Assertions.assertTrue(server.isRegistered(oname), + "the interceptor MBean must be registered once the pool is in use"); + + ds.close(); + + Assertions.assertFalse(server.isRegistered(oname), + "closing the only pool of the name must take the interceptor MBean down," + + " whatever position the JMX definition has in the chain"); + } + + @Test + void closingTheOnlyPoolReleasesJmxAheadOfABaseReportDefinition() throws Exception { + // The reverse order always worked: pin it so a fix for the other + // order cannot move the release off the signalled path + String poolName = "chain-jmx-first-" + System.nanoTime(); + DataSource ds = poolNamed(poolName, SQRJ + "(notifyPool=false);" + SQR); + ds.createPool(); + run(ds, "SELECT 1"); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = interceptorName(poolName); + Assertions.assertTrue(server.isRegistered(oname), + "the interceptor MBean must be registered once the pool is in use"); + + ds.close(); + + Assertions.assertFalse(server.isRegistered(oname), + "closing the only pool of the name must take the interceptor MBean down"); + } + + @Test + void closingOneOfTwoSameNamedChainedPoolsKeepsTheRegistration() throws Exception { + // The statistics-gone release must not fire while a same-named + // survivor still runs: the registration belongs to the name + String poolName = "chain-survivors-" + System.nanoTime(); + String chain = SQR + ";" + SQRJ + "(notifyPool=false)"; + DataSource first = poolNamed(poolName, chain); + DataSource second = poolNamed(poolName, chain); + first.createPool(); + second.createPool(); + run(first, "SELECT 1"); + run(second, "SELECT 2"); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = interceptorName(poolName); + + first.close(); + + Assertions.assertTrue(server.isRegistered(oname), + "closing one of two live pools of the same name must keep the shared" + + " registration for the survivor"); + Assertions.assertNotNull(SlowQueryReport.getPoolStats(poolName), + "and the statistics of the name"); + + second.close(); + + Assertions.assertFalse(server.isRegistered(oname), + "closing the last pool of the name must take the registration down"); + Assertions.assertNull(SlowQueryReport.getPoolStats(poolName), + "and the statistics with it"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
