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 e8e4ea99c111a7c64ad66770f41dbd72c223f5fa Author: opencode <[email protected]> AuthorDate: Wed Nov 4 09:58:33 2026 +0100 Keep a renamed pool on the registrations it made at start The pool name property is mutable on a running pool - the pool's own JMX exposes a setName that rewrites it - while everything a pool registers for its name was keyed by the name the pool carried at the moment the registration was made. The lookups, however, resolved through the live property, so a rename detached a healthy pool from its own state, in three ways: Booking. SlowQueryReport.reset() resolved the pool's statistics map by the pool's current name. After a rename every freshly borrowed connection resolved nothing: its executions logged a false "Connection has already been closed or abandoned" warning and booked no statistics at all, while the map published at start silently froze at its last entry. Registering. SlowQueryReportJmx.reset() likewise took the current name as the name to publish under, so the first borrow after a rename took the per-name registry slot for the new name and registered a second interceptor MBean beside the one the pool had always had. Releasing. poolClosed resolved the registration to release by the name the pool carried at close time. A renamed pool therefore released a name it had never registered and left behind everything its start had published: the statistics entry, the registry slot, the MBean registered with the MBean server - and, through the MBean's delegate, the closed pool's whole object graph, pinned forever. Nulling the name made the same resolution throw into the close loop's debug-only catch and leak the same way. The pool now records the name it started under in ConnectionPool before anything is registered for it, and every lookup of those registrations - the borrow, the booking, the release and the JMX publish - goes through that record. Renaming a live pool changes what the pool calls itself and moves nothing. A pool that never got far enough into its start to record a name releases nothing, which is what a null-name close did informally by throwing into the catch. TestSlowQueryReportPoolRename pins the three cases: booking and warnings across a rename, the release of the start-time registration under a nulled name, and the JMX unregistration of a renamed pool's MBean on close. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 33 ++++ .../jdbc/pool/interceptor/SlowQueryReport.java | 30 +++- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 20 ++- .../interceptor/TestSlowQueryReportPoolRename.java | 181 +++++++++++++++++++++ webapps/docs/changelog.xml | 18 ++ 5 files changed, 271 insertions(+), 11 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index 2e059606eb..df5782640c 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -128,6 +128,19 @@ public class ConnectionPool { */ private volatile boolean closed = false; + /** + * The pool name recorded when this pool started, written by {@link #init(PoolConfiguration)} before anything is + * registered for the pool and never rewritten. The {@code name} property is mutable at runtime - the pool's own + * JMX exposes a {@code setName} that rewrites it on a running pool - while the registrations made for a pool + * (the query statistics and, for the reporting interceptors, the MBean and the broadcaster published under the + * name) stay keyed by the name the pool ran under when they were made. Resolving one of those registrations + * through the live property would move it away from a renamed pool: the renamed pool would book nothing and + * report a false "closed or abandoned" warning on every execution while the entry published at start would + * outlive every close, pinned by the registration that could no longer be found to release it. Interceptors + * that hold per-name state therefore resolve it through this record rather than through {@link #getName()}. + */ + private volatile String registrationName; + /** * Since newProxyInstance performs the same operation, over and over again, it is much more optimized if we simply * store the constructor ourselves. One constructor is cached per proxy kind (with and without the XAConnection @@ -302,6 +315,20 @@ public class ConnectionPool { return getPoolProperties().getPoolName(); } + /** + * Returns the name this pool was registered under when it started. The registrations made for a pool - its + * query statistics and the monitoring MBean the reporting interceptors publish under the name - are keyed by + * this name, not by the live {@code name} property, which {@link #getName()} returns and which management can + * rewrite on a running pool: a rename then changes what the pool calls itself and moves nothing that was + * registered under the old name. + * + * @return the name recorded at pool start, or {@code null} while the pool has not gotten far enough into its + * start to record one, or if it carried no name when it started + */ + public String getRegistrationName() { + return registrationName; + } + /** * Return the number of threads waiting for a connection * @@ -749,6 +776,12 @@ public class ConnectionPool { // Make sure the pool is properly configured checkPoolConfiguration(properties); + // Record the name this pool starts under before its MBean or the + // interceptors are told about the pool, so that everything the start + // registers under the name and everything a later lookup of it + // resolves through one record (see the field). + registrationName = properties.getPoolName(); + // Both queues are unbounded by design: membership is bounded by the // maxActive/size bookkeeping, not by queue capacity. busy = new LinkedBlockingQueue<>(); 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 a4f270c928..bb0c6593b4 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 @@ -45,7 +45,10 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * 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 - + * starts and released when the last live pool registered under that name closes. The name is the one the pool started + * with - see {@link ConnectionPool#getRegistrationName()} - so renaming a running pool, which the pool's own JMX + * allows, neither detaches the pool from its statistics nor strands what its start published. 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. The * live-pool registration holds the pool weakly: a pool abandoned without closing is never pinned by it and can be @@ -286,15 +289,20 @@ public class SlowQueryReport extends AbstractQueryReport { @Override public void poolStarted(ConnectionPool pool) { super.poolStarted(pool); + // The name is the one the pool started under, not the live property: + // everything this registration publishes is keyed by it for the whole + // life of the pool, and every later lookup of it comes back through + // the same record (see ConnectionPool#getRegistrationName()). + String name = pool.getRegistrationName(); // 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 stats = perPoolStats.compute(name, (key, existing) -> { PoolStatistics registration = (existing == null) ? new PoolStatistics() : existing; registration.addLivePool(pool); return registration; }); if (stats.hasOtherLivePools() && log.isWarnEnabled()) { - log.warn(MessageFactory.get("slowQueryReport.poolNameInUse", pool.getName())); + log.warn(MessageFactory.get("slowQueryReport.poolNameInUse", name)); } queries = stats.getQueryStats(); } @@ -345,8 +353,18 @@ public class SlowQueryReport extends AbstractQueryReport { * @return {@code true} if this call removed the last live pool registered under the name */ protected static boolean releaseStatistics(ConnectionPool pool) { + // The name the pool registered under, recorded at its start: a pool + // renamed while running - the name property is mutable at runtime - + // still releases what its start published, and not whatever it + // happens to be called by the time it closes + String name = pool.getRegistrationName(); + if (name == null) { + // The pool never got far enough into its start to record a name, + // so nothing was ever registered for it to release + return false; + } boolean[] last = new boolean[1]; - perPoolStats.computeIfPresent(pool.getName(), (name, existing) -> { + perPoolStats.computeIfPresent(name, (key, existing) -> { last[0] = existing.removeLivePool(pool); return last[0] ? null : existing; }); @@ -470,7 +488,9 @@ public class SlowQueryReport extends AbstractQueryReport { public void reset(ConnectionPool parent, PooledConnection con) { super.reset(parent, con); if (parent != null) { - queries = getPoolStats(parent.getName()); + // The name the pool's statistics were registered under, recorded + // at its start - renaming the pool does not move them + queries = getPoolStats(parent.getRegistrationName()); } else { queries = null; } 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 bd044d4f0c..3f17bad358 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 @@ -57,7 +57,10 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * same name - reports through them. They stay published until the last live pool registered under the name closes; * closing one of several same-named pools leaves the registration with the survivors. A pool that starts under the * name after that last close has decided its release - racing it - adopts the registration still published under the - * name, and the pending release notices the takeover and leaves the registration with the new pool. + * name, and the pending release notices the takeover and leaves the registration with the new pool. The name is the + * one the pool started with - see {@link ConnectionPool#getRegistrationName()} - so renaming a running pool, which + * the pool's own JMX allows, moves nothing: the MBean stays published under the name it was registered under, and the + * pool keeps reporting through it exactly as before the rename. * </p> * <p> * A JMX client's listener subscription belongs to the registration it was added through. JMX cannot carry a @@ -248,7 +251,10 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE public void reset(ConnectionPool parent, PooledConnection con) { super.reset(parent, con); if (parent != null) { - poolName = parent.getName(); + // The name the pool's registrations are keyed under, recorded at + // its start - renaming the pool does not move them, and does not + // make a borrow register a second MBean under the new name + poolName = parent.getRegistrationName(); pool = parent; registerJmx(); } @@ -281,7 +287,7 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE */ @Override public void poolClosed(ConnectionPool pool) { - this.poolName = pool.getName(); + this.poolName = pool.getRegistrationName(); super.poolClosed(pool); // Either this instance's releaseStatistics call deregistered the // closing pool as the last live pool of the name, or an earlier @@ -290,8 +296,10 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE // 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) { + // meantime - which releaseJmxRegistration re-verifies. A pool that + // never got far enough into its start to record a name never had + // anything registered, and needs no release. + if (lastPoolOfNameClosed || (poolName != null && getPoolStats(poolName) == null)) { releaseJmxRegistration(pool); } } @@ -343,7 +351,7 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE public void poolStarted(ConnectionPool pool) { this.pool = pool; super.poolStarted(pool); - this.poolName = pool.getName(); + this.poolName = pool.getRegistrationName(); } /** diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportPoolRename.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportPoolRename.java new file mode 100644 index 0000000000..297e79a8d2 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportPoolRename.java @@ -0,0 +1,181 @@ +/* + * 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.Statement; +import java.util.ArrayList; +import java.util.List; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +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 statistics are published under the pool name, registered when the + * pool starts - but the name property itself is mutable at runtime: the + * pool's own JMX exposes {@code setName}, which rewrites it on a running + * pool. Resolving the registration through the <em>current</em> name + * made a rename silently kill the statistics of a healthy pool: every + * borrow resolved no statistics map, every execution booked nothing and + * logged a false "Connection has already been closed or abandoned" WARN, + * and the close released the wrong registration (or, with the name nulled + * out, threw into the close loop's catch), leaving the start-time entry - + * and for the JMX definition the MBean, its broadcaster and the closed + * pool's whole object graph - registered forever. The registration is now + * recorded under the name the pool started with, and every lookup of it + * - borrow, booking, release, JMX registration - goes through that + * record, so renaming a live pool changes what the pool calls itself and + * nothing else. + * + * <p>Reproducer for the mutable-name-keying and null-name-release defects + * of {@code defects-reports/defects-org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.md}.</p> + */ +class TestSlowQueryReportPoolRename 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"; + + private DataSource reportedPoolNamed(String name, String interceptor) { + DataSource ds = newDataSource(); // JMX off: a renamed pool must not + // clash with the pool's own MBean ObjectName, which really is keyed + // by the live property + ds.getPoolProperties().setName(name); + ds.getPoolProperties().setJdbcInterceptors(interceptor); + ds.getPoolProperties().setInitialSize(0); + return ds; + } + + private static void run(DataSource ds, String sql) throws Exception { + try (Connection pooled = ds.getConnection(); + Statement st = pooled.createStatement()) { + st.execute(sql); + } + } + + @Test + void aRenamedLivePoolKeepsBookingAndReleasesWhatItRegistered() throws Exception { + String startName = "rename-live-" + System.nanoTime(); + DataSource ds = reportedPoolNamed(startName, SQR + "(threshold=-1)"); + ds.createPool(); + run(ds, "SELECT 1"); + + // exactly what the pool's JMX setName performs on a running pool + String laterName = "rename-live-" + System.nanoTime(); + ds.getPoolProperties().setName(laterName); + + List<LogRecord> warnings = new ArrayList<>(); + Logger logger = Logger.getLogger(SlowQueryReport.class.getName()); + Level originalLevel = logger.getLevel(); + Handler handler = new Handler() { + @Override + public void publish(LogRecord record) { + if (record.getLevel().intValue() >= Level.WARNING.intValue()) { + warnings.add(record); + } + } + + @Override + public void flush() { + // no buffering + } + + @Override + public void close() { + // nothing to release + } + }; + logger.addHandler(handler); + logger.setLevel(Level.ALL); + try { + run(ds, "SELECT 1"); + run(ds, "SELECT 1"); + } finally { + logger.removeHandler(handler); + logger.setLevel(originalLevel); + } + + Assertions.assertTrue(warnings.isEmpty(), + "a healthy pool renamed at runtime must not turn its executions into" + + " closed-or-abandoned reports, saw: " + warnings); + + // what the rename must not do is move the registration: bookings + // keep landing in the map the pool registered at start + Assertions.assertEquals(3, SlowQueryReport.getPoolStats(startName).get("SELECT 1").getNrOfInvocations(), + "executions after the rename must book into the registration made at start"); + Assertions.assertNull(SlowQueryReport.getPoolStats(laterName), + "the new name holds no registration - the statistics were never moved"); + + ds.close(); + Assertions.assertNull(SlowQueryReport.getPoolStats(startName), + "the close must release the registration the pool actually made," + + " not one keyed by the name it carries at close time"); + } + + @Test + void aPoolClosedWhileNamelessStillReleasesItsRegistration() throws Exception { + String startName = "rename-null-" + System.nanoTime(); + DataSource ds = reportedPoolNamed(startName, SQR + "(threshold=-1)"); + ds.createPool(); + run(ds, "SELECT 2"); + + // a null name property must not strand the release: the name the NPE + // used to throw from is taken, and the close loop only logs it at debug + ds.getPoolProperties().setName(null); + ds.close(); + + Assertions.assertNull(SlowQueryReport.getPoolStats(startName), + "the close must find the start-time registration even when the name" + + " property is null by close time"); + } + + @Test + void aRenamedPoolReleasesItsJmxRegistrationOnClose() throws Exception { + String startName = "rename-jmx-" + System.nanoTime(); + DataSource ds = reportedPoolNamed(startName, SQRJ + "(notifyPool=false)"); + ds.createPool(); + run(ds, "SELECT 3"); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + SlowQueryReportJmx.class.getName() + + ",name=" + startName); + Assertions.assertTrue(server.isRegistered(oname), + "the interceptor MBean is registered under the name the pool runs under"); + + ds.getPoolProperties().setName("rename-jmx-" + System.nanoTime()); + ds.close(); + + Assertions.assertFalse(server.isRegistered(oname), + "the close must take down the registration under the name it was" + + " published under, whatever the pool is called by then"); + Assertions.assertNull(SlowQueryReport.getPoolStats(startName), + "and the statistics with it - a renamed close that missed the entry" + + " pinned the closed pool's whole graph through the statics"); + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 8a993df07e..4b81ad1543 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1523,6 +1523,24 @@ booked only for the borrow the call started under, while the <code>resetTimer()</code> management operation is unchanged. (remm) </fix> + <fix> + Correct a defect where renaming a running connection pool - the pool's + own JMX exposes a <code>setName</code> that rewrites the name property + of a live pool - silently detached the <code>SlowQueryReport</code> + and <code>SlowQueryReportJmx</code> interceptors from the state their + pool had registered under its start-time name: connections borrowed + after the rename booked their statistics nowhere and logged a false + "Connection has already been closed or abandoned" warning on every + execution, a <code>SlowQueryReportJmx</code> borrow registered a + second MBean under the new name, and the pool's close released the + name it carried at close time rather than the one it had registered, + leaving the statistics, the interceptor MBean and with them the whole + closed pool's object graph registered forever. A nulling rename made + the same release throw and be swallowed, leaking the same way. The + registrations are now recorded under the name the pool started with + and every lookup of them resolves through that record, so a rename + changes what the pool calls itself and nothing else. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
