This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 46b18ba7973a5cd6937ed4babf44f5edb1646724 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 11:08:42 2026 +0100 Track slow query statistics by pool name only Pools now carry unique names, so the pool-keyed registry in SlowQueryReport no longer adds anything over the name-keyed store. Keep one statistics store per pool name, let PoolStatistics hold no reference to the pool, and resolve every borrow through the pool's registration name. A pool start creates a fresh store, so a pool that reuses an abandoned pool's name does not inherit its statistics. The JMX state continues to hang from the same name-keyed store, and the duplicate-pool-name warning and message are removed. Tests that assumed same-named pools keep separate stores are replaced with tests for distinct pools, and the JMX same-name scenarios now use distinct names. --- .../jdbc/pool/interceptor/SlowQueryReport.java | 226 ++++++--------------- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 22 +- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 - .../TestSlowQueryJmxChainCloseRelease.java | 31 ++- .../interceptor/TestSlowQueryJmxNotifications.java | 129 ++++++------ .../TestSlowQueryJmxSlowQueriesCDRace.java | 10 +- ...ames.java => TestSlowQueryReportPoolNames.java} | 92 +++------ 7 files changed, 186 insertions(+), 325 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 ddad5169c5..054d16c85a 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 @@ -16,7 +16,6 @@ */ package org.apache.tomcat.jdbc.pool.interceptor; -import java.lang.ref.WeakReference; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Collections; @@ -24,7 +23,6 @@ import java.util.Comparator; import java.util.Locale; import java.util.Map; import java.util.TimeZone; -import java.util.WeakHashMap; import java.util.concurrent.ConcurrentHashMap; import javax.management.openmbean.CompositeDataSupport; @@ -44,26 +42,21 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** * Slow query report interceptor. Tracks timing of query executions. * <p> - * The statistics belong to a pool: a pool registers its own store when it starts and releases it when it closes, and - * every connection of that pool books its executions into that one store. The store is published under the name the - * pool started with - see {@link ConnectionPool#getRegistrationName()} - which is how it is read by name - * (see {@link #getPoolStats(String)}), 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. A reporting definition the interceptor list gains - * after the start is not stranded either: the list is mutable at runtime through the pool's own MBean, so its first - * borrow registers the store its start would have registered, and the close that releases the store is told about that - * definition as well. + * The statistics belong to the pool the interceptor serves: a pool registers its store when it starts and releases it + * when it closes, and every connection of that pool books its executions into that one store. The store is read by the + * name the pool started with - see {@link ConnectionPool#getRegistrationName()} and {@link #getPoolStats(String)}. + * The name identifies the pool: a container names every pool it manages uniquely, and the default name of a + * programmatically built pool is unique as well. Renaming a running pool, which the pool's own JMX allows, neither + * detaches the pool from its statistics nor strands what its start published. * </p> * <p> - * The name is a view over pool-keyed registrations, not the registration itself. Two live pools configured with the - * same name therefore each keep their own statistics instead of reporting into one another's numbers: the view answers - * with the store of the pool that started under the name last, and the pool that started there first keeps collecting - * into its own store, reachable by its own connections and by no name. Sharing a name between live pools is not - * supported - a container guarantees a container-managed pool a name of its own, so what is left is programmatic - * configuration - and a pool that starts under a name another live pool holds says so in the log. + * A reporting definition the interceptor list gains after the start is not stranded either: the list is mutable at + * runtime through the pool's own MBean, so its first borrow registers the store its start would have registered, and + * the close that releases the store is told about that definition as well. * </p> * <p> - * The registrations hold the pool weakly: a pool abandoned without closing is never pinned by them and can be - * collected with its open sessions, leaving only the bounded statistics store published under its name until a pool + * The registrations hold no reference to the pool, so a pool abandoned without closing is never pinned by them and can + * be collected with its open sessions, leaving only the bounded statistics store published under its name until a pool * starting under that name takes the name over. * </p> * <p> @@ -99,20 +92,12 @@ public class SlowQueryReport extends AbstractQueryReport { protected static final String LOG_FAILED = "logFailed"; /** - * The view of the statistics stores by the name the owning pool started with. The registration itself is keyed by - * pool instance (see {@link #perPoolStatistics}); this map exists for {@link #getPoolStats(String)} and - * {@link SlowQueryReportJmxMBean#getPoolNames()}, which address statistics by name. It holds one store per name: - * the store of the pool that started under that name last. + * The statistics store published under each pool name. The name identifies the pool - a container names the pools + * it manages uniquely, and the default name of a programmatically built pool is unique as well - so this one + * registry resolves both the store a pool's connections book into and the store {@link #getPoolStats(String)} and + * {@link SlowQueryReportJmxMBean#getPoolNames()} read. */ protected static final ConcurrentHashMap<String, PoolStatistics> perPoolStats = new ConcurrentHashMap<>(); - /** - * The statistics store of every live pool, keyed by pool instance: pools are what the statistics belong to, so - * two pools configured with the same name keep two stores and never book into each other's numbers. The map is - * keyed weakly - a pool the application abandoned without closing must drain out of this JVM-lifetime registry - * together with everything it reaches, exactly as it did before the registry keyed anything by pool. - */ - private static final Map<ConnectionPool, PoolStatistics> perPoolStatistics = Collections - .synchronizedMap(new WeakHashMap<ConnectionPool, PoolStatistics>()); /** * The statistics store this instance books to, resolved from the pool it serves; {@code null} when that pool has * no store. See {@link #reset(ConnectionPool, PooledConnection)}. @@ -155,10 +140,10 @@ public class SlowQueryReport extends AbstractQueryReport { /** * Returns the query statistics published under the given pool name. * <p> - * The name is a view over the pool-keyed registrations (see {@link #perPoolStats}): it publishes the store of the - * pool that started under that name last, and is given up with that pool's close. The one store that outlives the - * rule is a pool that was abandoned without closing: its bounded statistics stay published under its name until a - * pool starting under the name replaces it, because the close that would give them up never ran. + * The store is published under the name the pool started with and is given up with that pool's close. The one + * store that outlives the rule is a pool that was abandoned without closing: its bounded statistics stay + * published under its name until a pool starting under the name replaces it, because the close that would give + * them up never ran. * </p> * * @param poolname - the name of the pool we want to retrieve stats for @@ -180,13 +165,6 @@ public class SlowQueryReport extends AbstractQueryReport { /** * Returns the statistics store registered for the given pool. - * <p> - * The name view answers whenever it publishes this pool's own store, which is the case for every pool the - * container names and for every programmatic pool whose name no other live pool shares; for those the read is the - * one concurrent lookup it has always been. Only a pool whose name a second live pool has taken over - the - * configuration {@link #poolStarted(ConnectionPool)} warns about - is reached through the pool-keyed registry, - * whose lookup is synchronised. An unsupported configuration pays for itself; the healthy borrow path does not. - * </p> * * @param pool the pool whose store is wanted, possibly {@code null} * @@ -198,13 +176,7 @@ public class SlowQueryReport extends AbstractQueryReport { return null; } String name = pool.getRegistrationName(); - if (name != null) { - PoolStatistics published = perPoolStats.get(name); - if (published != null && published.isFor(pool)) { - return published; - } - } - return perPoolStatistics.get(pool); + return name == null ? null : perPoolStats.get(name); } /** @@ -363,19 +335,18 @@ public class SlowQueryReport extends AbstractQueryReport { /** * Registers the statistics store of a pool that has started. * <p> - * The store belongs to this pool alone and is published twice: in the pool-keyed registration, which is what the - * pool's own connections book through, and in the view by the name the pool started with, which is what - * {@link #getPoolStats(String)} and the JMX read surface answer from. A name is published by one store at a time, - * so a pool that starts under a name another live pool holds takes over that view - it does not merge with the - * other pool's statistics, and the warning its start writes says which of the two pools now answers for the name. + * The store is published under the name the pool started with, which is what {@link #getPoolStats(String)} and + * the JMX read surface answer from. The name identifies the pool, so a start always begins a fresh store: a pool + * that starts under a name an abandoned pool left behind does not inherit the abandoned pool's numbers. * </p> * <p> - * A pool that carries no name is refused. The name is not one setting among others here: it is the key of the view - * the store is published in and of the ObjectName the JMX subclass publishes its MBean under, and neither can be - * keyed under no name. Failing the start with a message that names the setting is therefore the honest outcome, - * and it is what the pool reports as the reason its start failed - where the missing key gave a start failure that - * said nothing. A container names every pool it starts, so reaching this is a programmatic pool configured without - * a name; the check stays as the library's own guarantee that what it registers always has a key. + * A pool that carries no name is refused. The name is not one setting among others here: it is the key of the + * registry the store is published in and of the ObjectName the JMX subclass publishes its MBean under, and + * neither can be keyed under no name. Failing the start with a message that names the setting is therefore the + * honest outcome, and it is what the pool reports as the reason its start failed - where the missing key gave a + * start failure that said nothing. A container names every pool it starts, so reaching this is a programmatic pool + * configured without a name; the check stays as the library's own guarantee that what it registers always has a + * key. * </p> * * @param pool the connection pool that has started @@ -396,55 +367,20 @@ public class SlowQueryReport extends AbstractQueryReport { // ConcurrentHashMap would refuse the key before anything else could throw new IllegalStateException(MessageFactory.get("slowQueryReport.poolNameRequired")); } - PoolStatistics own; - synchronized (perPoolStatistics) { - own = registerStatistics(pool, name); - } + PoolStatistics own = new PoolStatistics(name); + perPoolStats.put(name, own); this.poolStatistics = own; this.queries = own.getQueryStats(); } - /** - * Registers a statistics store for the given pool, unless it holds one already. - * <p> - * The pool's monitor is the caller's to hold: {@link #poolStarted(ConnectionPool)} takes it for the start, and the - * first borrow of a definition that joined the chain after the start takes it for the registration its start never - * came. Registering rather than only reading is what keeps that second case alive - the interceptor property is - * mutable at runtime, through the pool's own MBean as much as in code, and a reporting definition added to a - * running pool has no start left to register with but every right to monitor the pool it was added to. - * </p> - * <p> - * The registration is idempotent per pool: a store that is still publishing is kept, so the two definitions of the - * recommended two-thresholds configuration, and every borrow of a late definition, share the one store the first of - * them registered instead of each starting a statistics map of its own. - * </p> - * - * @param pool the pool to register a store for - * @param name the name the pool started with, which the store is published under in the view - * - * @return the store of that pool - the one it held, or the one this call registered - */ - private static PoolStatistics registerStatistics(ConnectionPool pool, String name) { - PoolStatistics existing = perPoolStatistics.get(pool); - if (existing != null && !existing.isReleased()) { - return existing; - } - PoolStatistics own = new PoolStatistics(pool, name); - perPoolStatistics.put(pool, own); - PoolStatistics displaced = perPoolStats.put(name, own); - if (displaced != null && displaced != own && displaced.isLive() && log.isWarnEnabled()) { - log.warn(MessageFactory.get("slowQueryReport.poolNameInUse", name)); - } - return own; - } - /** * Resolves the store the borrow this instance serves is to book into, registering the pool's store when this * instance is the first component of that pool to need one. * <p> - * The cheap read comes first, and it is the whole cost for a healthy pool: a pool named by its container answers - * from the lock-free view. Only a borrow that finds nothing looks further, and only for a pool that is still live - * and carries a name to publish under. + * The read comes first, and it is the whole cost for a healthy pool. Only a borrow that finds nothing looks + * further, and only for a pool that is still live and carries a name to publish under. That second case is a + * reporting definition the interceptor list gained after the start: the definition has no start left to register + * with, but it still monitors the pool it was added to, and every borrow of it resolves the one store of that pool. * </p> * <p> * The liveness check is read under the registry's monitor, which is the monitor the close of the store's release @@ -470,11 +406,16 @@ public class SlowQueryReport extends AbstractQueryReport { // else; poolStarted says so when the pool has a start to say it at return null; } - synchronized (perPoolStatistics) { + synchronized (perPoolStats) { if (parent.isClosed()) { return null; } - return registerStatistics(parent, name); + own = perPoolStats.get(name); + if (own == null) { + own = new PoolStatistics(name); + perPoolStats.put(name, own); + } + return own; } } @@ -484,9 +425,7 @@ public class SlowQueryReport extends AbstractQueryReport { * 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 store to * release can only be identified through the closing pool passed here, not through this instance's own state. The - * release is idempotent and needs no decision about anyone else: the store being given up is the closing pool's - * own, so a second pool that was configured with the same name never loses anything through it, and every - * definition of the closing pool's chain can run the release without consuming a signal a later definition needed. + * release is idempotent, and every definition of the closing pool's chain can run it. * </p> * * @param pool the connection pool that has been closed @@ -498,9 +437,8 @@ public class SlowQueryReport extends AbstractQueryReport { } /** - * Gives up the statistics store registered for the given pool: the store stops publishing its statistics, the pool - * is taken out of the pool-keyed registration, and the name view is freed unless a later pool has taken the name - * over in the meantime. + * Gives up the statistics store registered for the given pool: the store stops publishing its statistics and is + * taken out of the registry. * * @param pool the connection pool that has been closed * @@ -511,21 +449,20 @@ public class SlowQueryReport extends AbstractQueryReport { // 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 - PoolStatistics own; - synchronized (perPoolStatistics) { - own = perPoolStatistics.remove(pool); - } - if (own == null) { + String name = pool.getRegistrationName(); + if (name == null) { // The pool never got far enough into its start to record a name, or // its close has already been delivered, so nothing is left for it to // release return null; } - own.release(); - // The name view is freed only while it still publishes this pool's own - // store: a pool that started under the name later owns the view now, and - // this close has no claim on it. - perPoolStats.remove(own.getName(), own); + PoolStatistics own; + synchronized (perPoolStats) { + own = perPoolStats.remove(name); + } + if (own != null) { + own.release(); + } return own; } @@ -828,25 +765,17 @@ public class SlowQueryReport extends AbstractQueryReport { /** - * The query statistics of one pool: the map its connections book into, the name the map is published under in the - * view, and the pool the statistics belong to. + * The query statistics of one pool: the map its connections book into and the name the map is published under. * <p> - * The pool is held with a weak reference: the store outlives the pool only in the sense that the bounded map under - * its name stays published, and a store that held its pool strongly would pin an abandoned pool together with - * everything it reaches - its queues, its pooled connections and their open physical sessions - which is what the - * registrations did before the pool was held weakly. A store's own identity is what the name view is keyed by - * (see {@link #perPoolStats}) and what a {@link SlowQueryReportJmx} interceptor's per-pool JMX state hangs from, - * so neither has to know the pool to know which pool it is serving. + * The store holds no reference to the pool. The name identifies the pool - a container names its pools uniquely, + * and the default name of a programmatic pool is unique as well - so the store can outlive a pool abandoned without + * closing without pinning that pool, together with everything it reaches: its queues, its pooled connections and + * their open physical sessions. * </p> */ protected static class PoolStatistics { /** - * the pool these statistics belong to, held weakly so that a pool abandoned without closing drains out of the - * JVM-lifetime registries that reach this store - */ - private final WeakReference<ConnectionPool> owner; - /** - * the name this store is published under in the view - the registration name of the pool at its start + * the name this store is published under - the registration name of the pool at its start */ private final String name; /** @@ -855,8 +784,7 @@ public class SlowQueryReport extends AbstractQueryReport { private final ConcurrentHashMap<String, QueryStats> stats = new ConcurrentHashMap<>(); /** * What the reporting subclass has published for this pool with JMX - its broadcaster and, in the - * {@code notifyPool=false} mode, its MBean. The state is 1:1 with this store, which is 1:1 with the pool, which - * is why it hangs from here instead of from a registry that would have to tell two pools of one name apart. + * {@code notifyPool=false} mode, its MBean. The state is 1:1 with this store. */ private volatile SlowQueryReportJmx.PoolJmx jmxState; /** @@ -869,38 +797,14 @@ public class SlowQueryReport extends AbstractQueryReport { /** * Creates the statistics store of one pool. * - * @param pool the pool the statistics belong to - * @param name the name the store is published under in the view + * @param name the name the store is published under */ - protected PoolStatistics(ConnectionPool pool, String name) { - owner = new WeakReference<>(pool); + protected PoolStatistics(String name) { this.name = name; } /** - * Returns whether this is the store of the given pool. - * - * @param pool the pool to look for - * - * @return {@code true} if this store belongs to the pool - */ - protected boolean isFor(ConnectionPool pool) { - return owner.get() == pool; - } - - /** - * Returns whether the pool this store belongs to is still live - which it is not once the store has been - * released with its close, and not once a pool that was abandoned without closing has been collected. A store - * whose pool a caller still holds always reads as live: a weak reference to a reachable pool is not cleared. - * - * @return {@code true} if a live pool still owns this store - */ - protected boolean isLive() { - return !released && owner.get() != null; - } - - /** - * Returns the name this store is published under in the view. + * Returns the name this store is published under. * * @return the pool name recorded when the store was created */ 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 31c15c91c2..d3c5d02eb4 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 @@ -63,16 +63,10 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * first interceptor instance of a pool to borrow a connection publishes them, and every other instance of that pool - * its other connections, the second definition of the recommended two-thresholds configuration - reports through them. * They are published under the name the pool started with - see {@link ConnectionPool#getRegistrationName()} - which - * is what makes the published MBean and the statistics it exposes read as one identifier; 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. The close of that pool gives both up. - * </p> - * <p> - * What one pool publishes is therefore never read, released, or adopted by another pool, and a pool configured with a - * name another live pool holds does not report through that other pool's MBean: its own publication is refused by the - * MBeanServer, which is reported once, retried on later borrows, and leaves the other pool's monitoring alone. A - * container names every container-managed pool of its own, so a name collision between two live pools is left to - * programmatic configuration. + * identifies the pool, because a container names every pool it manages uniquely and the default name of a + * programmatically built pool is unique as well. 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. + * The close of that pool gives both up. * </p> * <p> * The published resource is a {@link Registration}, not an interceptor instance: it holds the name, the statistics @@ -143,9 +137,9 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * publication the MBeanServer refused. * <p> * The state hangs from the pool's statistics store (see {@link PoolStatistics#getJmxState()}), which is 1:1 with - * the pool, so nothing here has to tell two pools of one name apart and nothing a pool published can be given up by - * another pool's close. It is built by the first borrower that needs it and given up by the release of the store it - * hangs from, which is what the close of the pool is. + * the pool, and nothing a pool published can be given up by another pool's close. It is built by the first + * borrower that needs it and given up by the release of the store it hangs from, which is what the close of the + * pool is. * </p> * <p> * Publication and release are serialised against each other by this object's monitor, and the MBeanServer is @@ -724,7 +718,7 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE /** * The names currently holding recorded statistics - the read behind {@link #getPoolNames()} and the registration's * own {@code PoolNames} attribute. These are the names of the pools that publish a statistics store, one store per - * pool; a name two live pools were configured with appears once, under the pool that started there last. + * pool. * * @return the names of the pools that currently hold a statistics map */ diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 2628fc0e85..61fbde92c7 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -207,7 +207,6 @@ slowQueryReport.invalidLogFailed=The configured logFailed [{0}] is neither true slowQueryReport.invalidLogSlow=The configured logSlow [{0}] is neither true nor false and is not applied: slow query logging keeps the setting it carries, [{1}]. Correct the value in the jdbcInterceptors configuration to change what is logged there. slowQueryReport.invalidMaxQueries=The configured maxQueries [{0}] is not a number of queries and is not applied: statistics keep being stored for at most {1} queries per pool. Correct the value in the jdbcInterceptors configuration to size the statistics store that was meant to be configured. slowQueryReport.invalidThreshold=The configured threshold [{0}] is not a number of milliseconds and is not applied: executions slower than the current threshold of {1} milliseconds keep being reported as slow queries. Correct the value in the jdbcInterceptors configuration to report the threshold that was meant to be configured. -slowQueryReport.poolNameInUse=Connection pool name [{0}] is already held by another live pool. Statistics and monitoring belong to the pool that registered them, so this pool keeps its own statistics apart from that pool's, while the read by name answers with this pool alone: give every pool a name of its own, as a container does for the pools it starts. slowQueryReport.poolNameRequired=A connection pool configured with the slow query report interceptor must have a name: the interceptor publishes its query statistics and, for its subclass, its JMX registration under that name, so a pool that carries none has nowhere to report to. Set the name property of the pool, or leave the reporting interceptor out of the jdbcInterceptors configuration. A container-managed pool always carries a name the container generated, so this is only reached by [...] slowQueryReport.slowQueryReport=Slow Query Report SQL={0}; time={1} ms; slowQueryReport.unknownProperty=The interceptor property [{0}] is not recognized and is not applied: property names are matched exactly, and a name this interceptor does not know leaves the setting it was meant to configure at the value it carries. Check the jdbcInterceptors configuration for the name that was meant to be configured. 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 index e671d3003a..9d23bed3a4 100644 --- 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 @@ -54,7 +54,7 @@ class TestSlowQueryJmxChainCloseRelease extends JdbcPoolBaseTest { "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. */ + * itself so the test only observes the interceptor's own publication. */ private DataSource poolNamed(String name, String interceptors) { DataSource ds = newDataSource(); ds.getPoolProperties().setName(name); @@ -118,22 +118,22 @@ class TestSlowQueryJmxChainCloseRelease extends JdbcPoolBaseTest { } @Test - void eachOfTwoSameNamedChainedPoolsReleasesOnlyItsOwnRegistration() throws Exception { + void closingOneChainedPoolReleasesOnlyItsOwnRegistration() throws Exception { // The publication belongs to the pool that made it: the close of that - // pool takes it down even while a pool configured with the same name is - // still running, and that other pool - whose own publication the - // MBeanServer refused - keeps its own statistics either way - String poolName = "chain-survivors-" + System.nanoTime(); + // pool takes it down while another, differently named pool is still + // running, and that other pool keeps its own statistics either way + String publisherName = "chain-publisher-" + System.nanoTime(); + String survivorName = "chain-survivor-" + System.nanoTime(); String chain = SQR + ";" + SQRJ + "(notifyPool=false)"; - DataSource publisher = poolNamed(poolName, chain); - DataSource refused = poolNamed(poolName, chain); + DataSource publisher = poolNamed(publisherName, chain); + DataSource survivor = poolNamed(survivorName, chain); publisher.createPool(); - refused.createPool(); + survivor.createPool(); run(publisher, "SELECT 1"); - run(refused, "SELECT 2"); + run(survivor, "SELECT 2"); MBeanServer server = ManagementFactory.getPlatformMBeanServer(); - ObjectName oname = interceptorName(poolName); + ObjectName oname = interceptorName(publisherName); Assertions.assertTrue(server.isRegistered(oname), "the premise: the first pool's borrow published the MBean"); @@ -141,14 +141,13 @@ class TestSlowQueryJmxChainCloseRelease extends JdbcPoolBaseTest { Assertions.assertFalse(server.isRegistered(oname), "the close of the pool that published the MBean takes it down, whatever" - + " position the JMX definition has in its chain and whatever other" - + " pool was configured with the same name"); - Assertions.assertNotNull(SlowQueryReport.getPoolStats(poolName), + + " position the JMX definition has in its chain"); + Assertions.assertNotNull(SlowQueryReport.getPoolStats(survivorName), "and the other pool's statistics are untouched by it"); - refused.close(); + survivor.close(); - Assertions.assertNull(SlowQueryReport.getPoolStats(poolName), + Assertions.assertNull(SlowQueryReport.getPoolStats(survivorName), "its own close releases the statistics it registered under the name"); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxNotifications.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxNotifications.java index 874b1fac36..538ae4bbe2 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxNotifications.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxNotifications.java @@ -565,51 +565,49 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } @Test - void aSameNamedPoolReportsThroughItsOwnPublicationAndNotTheOtherS() throws Exception { - String poolName = "JmxDuplicate-" + System.nanoTime(); - DataSource ownerPool = reportingPoolNamed(poolName); - DataSource otherPool = reportingPoolNamed(poolName); + void eachPoolReportsThroughItsOwnPublicationAndNotTheOtherS() throws Exception { + String ownerName = "JmxOwner-" + System.nanoTime(); + String otherName = "JmxOther-" + System.nanoTime(); + DataSource ownerPool = reportingPoolNamed(ownerName); + DataSource otherPool = reportingPoolNamed(otherName); MBeanServer server = ManagementFactory.getPlatformMBeanServer(); - ObjectName oname = new ObjectName("tomcat.jdbc:type=" - + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + ObjectName ownerOn = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + ownerName); + ObjectName otherOn = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + otherName); List<Notification> published = new CopyOnWriteArrayList<>(); - List<Notification> refused = new CopyOnWriteArrayList<>(); + List<Notification> otherPublished = new CopyOnWriteArrayList<>(); NotificationListener publishedListener = (notification, handback) -> published.add(notification); - NotificationListener refusedListener = (notification, handback) -> refused.add(notification); + NotificationListener otherListener = (notification, handback) -> otherPublished.add(notification); - // The first pool's borrow publishes the MBean under the name; the - // second pool, configured with the same name, cannot publish its own - // and is refused by the MBeanServer. It keeps running, and it keeps - // reporting - through a broadcaster of its own, because the MBean it - // was refused is not its own to report on + // Each pool publishes its own MBean under its own name, and its + // queries reach only the listeners of that publication. Connection ownerConnection = ownerPool.getConnection(); Connection otherConnection = otherPool.getConnection(); try { - server.addNotificationListener(oname, publishedListener, null, null); - interceptorOf(otherConnection, SlowQueryReportJmx.class) - .addNotificationListener(refusedListener, null, null); + server.addNotificationListener(ownerOn, publishedListener, null, null); + server.addNotificationListener(otherOn, otherListener, null, null); try (Statement st = ownerConnection.createStatement()) { st.execute(slowSql(1)); } Assertions.assertTrue(published.stream().anyMatch(n -> n.getMessage().contains("SELECT 1")), - "the publishing pool's queries reach the listeners of its MBean, received: " + "the pool's queries reach the listeners of its MBean, received: " + published); try (Statement st = otherConnection.createStatement()) { st.execute(slowSql(2)); } - Assertions.assertTrue(refused.stream().anyMatch(n -> n.getMessage().contains("SELECT 2")), - "the refused pool must still notify its own listeners, received: " + refused); + Assertions.assertTrue(otherPublished.stream().anyMatch(n -> n.getMessage().contains("SELECT 2")), + "the other pool must notify its own listeners, received: " + otherPublished); Assertions.assertFalse(published.stream().anyMatch(n -> n.getMessage().contains("SELECT 2")), "and it must not report into the other pool's MBean: a client listening to the" + " name would otherwise read another pool's queries as its own," + " received: " + published); - // Close the pool that was refused. It published nothing, so it has - // nothing to give up, and it must not take the other pool's - // publication down on its way out + // Closing one pool gives up its own publication and leaves the + // other pool's publication alone. otherConnection.close(); otherConnection = null; otherPool.close(); @@ -617,9 +615,10 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { try (Statement st = ownerConnection.createStatement()) { st.execute(slowSql(3)); } - Assertions.assertTrue(server.isRegistered(oname), - "closing a pool that was refused the name must leave the pool that holds it" - + " published"); + Assertions.assertFalse(server.isRegistered(otherOn), + "closing a pool gives up its own publication"); + Assertions.assertTrue(server.isRegistered(ownerOn), + "closing one pool must leave the other pool's publication standing"); Assertions.assertTrue(published.stream().anyMatch(n -> n.getMessage().contains("SELECT 3")), "and its clients' listeners must still be served, received: " + published); } finally { @@ -639,11 +638,16 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } ownerPool.close(); try { - server.removeNotificationListener(oname, publishedListener); + server.removeNotificationListener(ownerOn, publishedListener); } catch (ListenerNotFoundException | InstanceNotFoundException ignore) { // the teardown under test may already have dropped the binding } - Assertions.assertFalse(server.isRegistered(oname), + try { + server.removeNotificationListener(otherOn, otherListener); + } catch (ListenerNotFoundException | InstanceNotFoundException ignore) { + // the teardown under test may already have dropped the binding + } + Assertions.assertFalse(server.isRegistered(ownerOn), "closing the pool that published the MBean gives it up"); } } @@ -989,39 +993,37 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } @Test - void concurrentClosesOfSameNamedPoolsLeaveNothingBehind() throws Exception { - String poolName = "JmxRace-" + System.nanoTime(); - DataSource first = reportingPoolNamed(poolName); - DataSource second = reportingPoolNamed(poolName); - DataSource third = reportingPoolNamed(poolName); + void concurrentClosesOfDistinctPoolsLeaveNothingBehind() throws Exception { + String[] names = { "JmxRace0-" + System.nanoTime(), "JmxRace1-" + System.nanoTime(), + "JmxRace2-" + System.nanoTime() }; + DataSource[] sources = new DataSource[names.length]; + ObjectName[] onames = new ObjectName[names.length]; MBeanServer server = ManagementFactory.getPlatformMBeanServer(); - ObjectName oname = new ObjectName("tomcat.jdbc:type=" - + SlowQueryReportJmx.class.getName() + ",name=" + poolName); - - Connection one = first.getConnection(); - Connection two = second.getConnection(); - Connection three = third.getConnection(); - Assertions.assertTrue(server.isRegistered(oname), - "the MBean must be registered while the same-named pools run - published by the" - + " first of them to borrow"); - ConnectionPool[] pools = { first.getPool(), second.getPool(), third.getPool() }; + Connection[] connections = new Connection[names.length]; + for (int i = 0; i < names.length; i++) { + sources[i] = reportingPoolNamed(names[i]); + onames[i] = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + names[i]); + connections[i] = sources[i].getConnection(); + Assertions.assertTrue(server.isRegistered(onames[i]), + "the MBean must be registered while the pool runs"); + } + ConnectionPool[] pools = { sources[0].getPool(), sources[1].getPool(), sources[2].getPool() }; // return every connection first, so the concurrent closes race only // on the registries, not on pooled connections - one.close(); - two.close(); - three.close(); + for (Connection connection : connections) { + connection.close(); + } // a start gate releases all three closes together: each of them gives - // up its own pool's store and its own publication, and the name view - // is freed by the close that still holds it - none of which may block - // on, fail because of, or miss what one of the others is doing + // up its own pool's store and its own publication - none of which may + // block on, fail because of, or miss what one of the others is doing CountDownLatch startGate = new CountDownLatch(1); - final DataSource[] closing = {first, second, third}; - Throwable[] failures = new Throwable[closing.length]; - Thread[] closers = new Thread[closing.length]; - for (int i = 0; i < closing.length; i++) { - final DataSource pool = closing[i]; + Throwable[] failures = new Throwable[sources.length]; + Thread[] closers = new Thread[sources.length]; + for (int i = 0; i < sources.length; i++) { + final DataSource pool = sources[i]; final int slot = i; closers[i] = new Thread(() -> { try { @@ -1030,7 +1032,7 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } catch (Throwable x) { failures[slot] = x; } - }, "same-named-pool-closer-" + i); + }, "pool-closer-" + i); closers[i].start(); } startGate.countDown(); @@ -1046,26 +1048,25 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } awaitSessions(0); // every physical session of every pool is really gone - Assertions.assertFalse(server.isRegistered(oname), - "the close of the pool that published the MBean must release it, whatever the" - + " other same-named closes were doing at the time"); - for (ConnectionPool pool : pools) { - Assertions.assertNull(SlowQueryReportJmx.jmxStateFor(pool), + for (int i = 0; i < names.length; i++) { + Assertions.assertFalse(server.isRegistered(onames[i]), + "every close must release its own pool's MBean"); + Assertions.assertNull(SlowQueryReportJmx.jmxStateFor(pools[i]), "and every one of the closes must have given up its own pool's JMX state"); + Assertions.assertNull(SlowQueryReport.getPoolStats(names[i]), + "the statistics of every name must be freed with its pool's close"); } - Assertions.assertNull(SlowQueryReport.getPoolStats(poolName), - "the statistics view of the name must be freed with the closes"); // the name was really freed, not merely flagged: a pool restarted under // it must be able to publish, and must release the publication again // when it closes - DataSource restarted = reportingPoolNamed(poolName); + DataSource restarted = reportingPoolNamed(names[0]); try (@SuppressWarnings("unused") Connection borrowed = restarted.getConnection()) { - Assertions.assertTrue(server.isRegistered(oname), + Assertions.assertTrue(server.isRegistered(onames[0]), "a pool started under the concurrently freed name must publish its MBean"); } restarted.close(); - Assertions.assertFalse(server.isRegistered(oname), + Assertions.assertFalse(server.isRegistered(onames[0]), "closing the restarted pool must release the publication again"); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxSlowQueriesCDRace.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxSlowQueriesCDRace.java index faef5df1a0..db7efae609 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxSlowQueriesCDRace.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxSlowQueriesCDRace.java @@ -112,15 +112,13 @@ class TestSlowQueryJmxSlowQueriesCDRace { } /* - * The statistics of a pool, published under a name directly rather than - * through a live pool: the store is plain state, and the read under test - * only ever consults the map behind it. The store belongs to no pool here - * - nothing in the scenario reaches the pool an owner reference would - * point at - and the unique name keeps the static view clean across tests. + * The statistics of a pool, published under its name directly: the store is + * plain state, and the read under test only ever consults the map behind it. + * The unique name keeps the static view clean across tests. */ private static ConcurrentHashMap<String, QueryStats> seedStats(String poolName) { assertBinsInvisitOrder(); - SlowQueryReport.PoolStatistics statistics = new SlowQueryReport.PoolStatistics(null, poolName); + SlowQueryReport.PoolStatistics statistics = new SlowQueryReport.PoolStatistics(poolName); SlowQueryReport.perPoolStats.put(poolName, statistics); ConcurrentHashMap<String, QueryStats> live = statistics.getQueryStats(); live.put(MIDDLE, new QueryStats(MIDDLE)); 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/TestSlowQueryReportPoolNames.java similarity index 53% rename from modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDuplicatePoolNames.java rename to modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportPoolNames.java index 1adc709fb8..35a8e88608 100644 --- 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/TestSlowQueryReportPoolNames.java @@ -30,19 +30,12 @@ import org.apache.tomcat.jdbc.pool.DataSource; /** * The {@code SlowQueryReport} publishes a pool's statistics under the name the - * pool started with, and reads them back by that name - but the statistics - * themselves belong to the <em>pool</em>. Two live pools configured with the - * same name (a configuration only programmatic users reach: the container names - * every pool of its own, and {@code jmxEnabled=false} removes the JMX ObjectName - * clash that would otherwise catch it) therefore keep two stores, and what the - * name answers with is the store of the pool that started under it last. - * - * <p>Reproducer for defect 2 of - * {@code defects-pool/defects-org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.md}: - * what must never happen is one pool's close destroying the statistics of - * another pool, however the two were named.</p> + * pool started with, and reads them back by that name. The name identifies the + * pool - a container names every pool it manages uniquely, and the default name + * of a programmatically built pool is unique as well - so each pool has one + * store and one close releases only that store. */ -class TestSlowQueryReportDuplicatePoolNames extends JdbcPoolBaseTest { +class TestSlowQueryReportPoolNames 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)"; @@ -50,8 +43,7 @@ class TestSlowQueryReportDuplicatePoolNames extends JdbcPoolBaseTest { /* 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 + DataSource ds = newDataSource(); ds.getPoolProperties().setName(name); ds.getPoolProperties().setJdbcInterceptors("SlowQueryReport(threshold=-1)"); return ds; @@ -65,34 +57,40 @@ class TestSlowQueryReportDuplicatePoolNames extends JdbcPoolBaseTest { } @Test - void twoLivePoolsOfOneNameKeepTheirStatisticsApart() throws Exception { - String poolName = "dup-" + System.nanoTime(); - DataSource first = reportedPoolNamed(poolName); - DataSource second = reportedPoolNamed(poolName); + void twoLivePoolsKeepTheirStatisticsApart() throws Exception { + String firstName = "pool-first-" + System.nanoTime(); + String secondName = "pool-second-" + System.nanoTime(); + DataSource first = reportedPoolNamed(firstName); + DataSource second = reportedPoolNamed(secondName); first.createPool(); second.createPool(); run(first, QUERY_FIRST); run(second, QUERY_SECOND); - ConcurrentHashMap<String, SlowQueryReport.QueryStats> view = - SlowQueryReport.getPoolStats(poolName); - Assertions.assertNotNull(view, - "the running pools must publish a store under their name"); - Assertions.assertEquals(Collections.singleton(QUERY_SECOND), view.keySet(), - "the name answers with the pool that started under it last, and not" - + " with the numbers of the pool that was there before it: " + view.keySet()); + ConcurrentHashMap<String, SlowQueryReport.QueryStats> firstView = + SlowQueryReport.getPoolStats(firstName); + Assertions.assertNotNull(firstView, "the running pool must publish a store under its name"); + Assertions.assertEquals(Collections.singleton(QUERY_FIRST), firstView.keySet(), + "a pool's name must answer with that pool's own statistics: " + firstView.keySet()); + + ConcurrentHashMap<String, SlowQueryReport.QueryStats> secondView = + SlowQueryReport.getPoolStats(secondName); + Assertions.assertNotNull(secondView, "the running pool must publish a store under its name"); + Assertions.assertEquals(Collections.singleton(QUERY_SECOND), secondView.keySet(), + "a pool's name must answer with that pool's own statistics: " + secondView.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"); + Assertions.assertNull(SlowQueryReport.getPoolStats(firstName), + "closing one pool must release its own statistics"); + Assertions.assertNotNull(SlowQueryReport.getPoolStats(secondName), + "closing one pool must not throw another pool's statistics away"); run(second, QUERY_AFTER_CLOSE); ConcurrentHashMap<String, SlowQueryReport.QueryStats> surviving = - SlowQueryReport.getPoolStats(poolName); + SlowQueryReport.getPoolStats(secondName); Assertions.assertNotNull(surviving, "the surviving pool must keep collecting after the other pool closed"); Assertions.assertTrue(surviving.containsKey(QUERY_AFTER_CLOSE), @@ -104,45 +102,13 @@ class TestSlowQueryReportDuplicatePoolNames extends JdbcPoolBaseTest { second.close(); - Assertions.assertNull(SlowQueryReport.getPoolStats(poolName), + Assertions.assertNull(SlowQueryReport.getPoolStats(secondName), "closing the pool that holds the name must release its statistics"); } - @Test - void aPoolWhoseNameWasTakenOverKeepsBookingIntoItsOwnStore() throws Exception { - String poolName = "dup-displaced-" + System.nanoTime(); - DataSource displaced = reportedPoolNamed(poolName); - displaced.createPool(); - try (Connection con = displaced.getConnection()) { - SlowQueryReport reporter = interceptorOf(con, SlowQueryReport.class); - Assertions.assertNotNull(reporter, - "the chain must hold the configured reporting instance"); - - // A second pool starts under the name while the first is live: the - // name now answers with the newcomer, and the pool that was there - // first keeps running with a store no name reaches - DataSource successor = reportedPoolNamed(poolName); - successor.createPool(); - - try (Statement st = con.createStatement()) { - st.execute(QUERY_FIRST); - } - - Assertions.assertTrue(reporter.queries.containsKey(QUERY_FIRST), - "the displaced pool's own connections must still collect what they run"); - Assertions.assertTrue(SlowQueryReport.getPoolStats(poolName).isEmpty(), - "while the name publishes the successor's own store: the displaced pool's" - + " numbers must not read as the successor's, saw " - + SlowQueryReport.getPoolStats(poolName).keySet()); - - successor.close(); - } - displaced.close(); - } - @Test void aPoolStartedUnderAFreedNameCollectsIntoAFreshStore() throws Exception { - String poolName = "dup-" + System.nanoTime(); + String poolName = "reuse-" + System.nanoTime(); DataSource first = reportedPoolNamed(poolName); first.createPool(); run(first, QUERY_FIRST); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
