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 2abcbb3deefaa85aa8c06ff29f88f9b969e7b1c4 Author: opencode <[email protected]> AuthorDate: Fri Oct 30 15:04:26 2026 +0100 Release a pool name's JMX state only when the release still owns it The statistics release that decides to deregister a pool name's MBean is atomic, but the release itself was keyed by name without ownership checks: a closing pool could remove a registration a pool starting under the same name had just made, unregister the live MBean and orphan its notification listeners, and it derived the unregister ObjectName from its own class and properties instead of the name the registration was published under, leaking override-published MBeans permanently and guaranteeing InstanceAlreadyExistsException after a restart. Re-verify before releasing that no live pool re-registered under the name, remove registry entries only while they still hold the value read, and unregister the ObjectName the registration owner recorded. Deliberately lock-free: a release must never make a starting pool wait behind a closing one; the remaining window between two map reads is documented. --- .../jdbc/pool/interceptor/SlowQueryReport.java | 14 ++ .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 126 +++++++++++++-- .../interceptor/TestSlowQueryJmxNotifications.java | 174 ++++++++++++++++++++- 3 files changed, 301 insertions(+), 13 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 9cece12b1f..1c159b1eaf 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 @@ -477,6 +477,20 @@ public class SlowQueryReport extends AbstractQueryReport { return livePools.size() > 1; } + /** + * Returns whether the given pool is one of the live pools registered under this name. The live-pool set is + * concurrent, so this plain membership read is safe outside the {@code ConcurrentHashMap.compute} that mutates + * it: it tells a caller, without making a concurrent start wait behind a lock, whether the name is still held + * live by the given pool. + * + * @param pool the pool to look for + * + * @return {@code true} if the pool is registered as live under this name + */ + protected boolean hasLivePool(ConnectionPool pool) { + return livePools.contains(pool); + } + /** * Returns the statistics map published under this name. * 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 edd7aaffcb..5a2af6eb94 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 @@ -54,7 +54,9 @@ import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; * to borrow a connection registers them, and every other instance sharing the name - the other connections of that * pool, the second interceptor of the recommended two-thresholds configuration, or a second pool configured with the * 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. + * 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. * </p> * <p> * A JMX client's listener subscription belongs to the registration it was added through. JMX cannot carry a @@ -111,6 +113,15 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE */ protected static final ConcurrentHashMap<String, ObjectName> registrationFailuresLogged = new ConcurrentHashMap<>(); + /** + * The ObjectName this instance published itself under. {@link #registerJmx()} records it on the instance that + * took the registry slot, before the publish, and clears it again when the MBeanServer refused the MBean. The + * release in {@link #deregisterJmx()} unregisters exactly the name recorded on the registration the registry + * held, whatever class and {@code objectName} property the releasing instance carries - a closing pool delivers a + * freshly instantiated interceptor whose definitions need not match those of the pool that registered. + */ + protected volatile ObjectName registeredUnder; + /** * Notification broadcaster used before the pool name is known; once the pool name is known, {@link #getNotifier()} @@ -252,6 +263,11 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * 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. + * </p> * * @param pool the connection pool being closed */ @@ -263,9 +279,48 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE // 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 - deregisterJmx(); + // instance of an already closed pool, goes with them, unless + // a pool has taken the name back in the meantime + releaseJmxRegistration(pool); + } + } + + /** + * Completes the release of the current pool name whose statistics the given pool has just freed: drops the MBean + * registration with the name's broadcaster and registration-failure marker through {@link #deregisterJmx()} - + * unless a live pool is registered under the name again that does not include the releasing one. A pool that + * started under the name after the release was decided has taken the name over, adopting the registration still + * published under it, and it - not this close - owns the name's JMX state from then on; the last live pool of the + * name is the one to release it. + * <p> + * The statistics are read the plain way here, without a lock, deliberately: a check computed inside a + * {@code ConcurrentHashMap.compute} on the name would be atomic against a start but would also make that start + * wait behind the release, and a release must never block a pool that is coming up. Without the lock the check + * cannot see a start that begins after it has read - the {@link #deregisterJmx()} that follows then still takes + * down only the registration the registry held at its own read, never a fresher one, and only the name's + * broadcaster and failure marker can be hit early by a start landing inside that window; see + * {@link #deregisterJmx()}. The one case the value guard does not spare is a start whose fresh registration + * lands in the slot between the read here and the read in {@link #deregisterJmx()}: that release takes down + * the value it read, leaving the newcomer to republish on its next borrow - a window narrower than the one + * that existed before this guard, measured in the gap between two map reads. The guard likewise proceeds + * when the statistics still hold the very pool that is closing, which only happens when the same pool + * instance is restarted while this close is still in flight; the restarted generation then republishes its + * registration on its first borrow, like any pool whose registration was released. + * </p> + * + * @param releasedPool the pool whose statistics release decided this release of the name + */ + protected void releaseJmxRegistration(ConnectionPool releasedPool) { + PoolStatistics stats = perPoolStats.get(poolName); + if (stats != null && !stats.hasLivePool(releasedPool)) { + // A pool has started under the name since this close released the + // statistics: the name is live again and the new pool owns + // everything registered under it now - the MBean it adopted, the + // broadcaster its clients' listeners are attached to, and the + // bookkeeping. Leave them for the last live pool of the name. + return; } + deregisterJmx(); } /** @@ -472,22 +527,33 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE } /** - * Releases the MBean registration and the shared broadcaster held for the current pool name unconditionally - even - * when the pool name is still owned by live pools. Normal pool shutdown releases through {@code super.poolClosed} - * plus {@link #lastPoolOfNameClosed} in {@link #poolClosed(ConnectionPool)}, which keeps the registration for any - * surviving same-named pool. + * Releases the MBean registration and the shared broadcaster held for the current pool name - even when the pool + * name is still owned by live pools. Normal pool shutdown releases through {@code super.poolClosed} plus + * {@link #lastPoolOfNameClosed} in {@link #poolClosed(ConnectionPool)}, which keeps the registration for any + * surviving same-named pool and skips the release when a new pool has taken the name back + * ({@link #releaseJmxRegistration(ConnectionPool)}). + * <p> + * The registry slot is dropped only while it still holds the registration the registry was read at: a registration + * a pool installing itself behind this release puts into the slot belongs to that pool and keeps its MBean + * published. The unregistration uses the ObjectName that registration recorded when it was published, so it never + * depends on the class or the {@code objectName} property of the instance running this release - which a closing + * pool delivers freshly, and which need not share either with the registration it releases. + * </p> * <p> * The broadcaster and the registration-failure marker belong to the name as much as the MBean does, so the release * drops them even when this pool never held the registry slot: a pool whose every registration attempt was refused * still reported through the broadcaster, and its throttle marker must not outlive the name and hush the first - * failure of a later failure cycle under the name. + * failure of a later failure cycle under the name. Both entries are created lazily - by {@link #getNotifier()} and + * by listeners a client adds through interceptor handles it still holds - so a client subscribing to the released + * name concurrently with this release can have its late subscription re-insert an entry nothing registered under + * the name uses any more; a fresh registration under the name re-uses or replaces it. * </p> */ protected void deregisterJmx() { try { - if (mbeans.remove(poolName) != null) { - ObjectName oname = getObjectName(getClass(), poolName); - JmxUtil.unregisterJmx(oname); + SlowQueryReportJmxMBean registered = mbeans.get(poolName); + if (registered != null && mbeans.remove(poolName, registered)) { + JmxUtil.unregisterJmx(registrationObjectName(registered)); } notifiers.remove(poolName); registrationFailuresLogged.remove(poolName); @@ -497,6 +563,38 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE } + /** + * Returns the ObjectName the given registry registration published itself under, as the registry owner recorded it + * when {@link #registerJmx()} took the slot. Unregistering this - rather than a name recomputed from the class and + * properties of the instance running the release - is what keeps the unregistration on the name the MBean actually + * occupies: two pools configured with one name may carry interceptor definitions that compute different + * ObjectNames, and the release run by one of them must not give up on the name the other one's registration was + * published under and leave it registered for ever. + * + * @param registered the registry value this release took out of the registry + * + * @return the ObjectName to unregister + * + * @throws MalformedObjectNameException if a fallback ObjectName cannot be formed + */ + protected ObjectName registrationObjectName(SlowQueryReportJmxMBean registered) + throws MalformedObjectNameException { + if (registered instanceof SlowQueryReportJmx) { + SlowQueryReportJmx owner = (SlowQueryReportJmx) registered; + ObjectName published = owner.registeredUnder; + if (published != null) { + return published; + } + // The owner never got to record its name; fall back to the name + // its own registration would be attempted under - the owner's + // class and properties, not this releasing instance's + return owner.getObjectName(owner.getClass(), poolName); + } + // A value this class never registered - the protected registry is open + // to subclasses; the caller's own name shape is all there is to go by + return getObjectName(getClass(), poolName); + } + /** * Returns the JMX ObjectName for the given class and pool name. @@ -541,12 +639,18 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE ObjectName oname = getObjectName(getClass(), poolName); if (mbeans.putIfAbsent(poolName, this) == null) { try { + // Record the name this instance takes the slot for + // before the publish: a release racing this + // registration unregisters the very name being + // published, not one of its own choosing + registeredUnder = oname; ManagementFactory.getPlatformMBeanServer().registerMBean(this, oname); // A registration under the name works again - drop // the marker so a later failure cycle reports loudly registrationFailuresLogged.remove(poolName); } catch (Exception x) { mbeans.remove(poolName, this); + registeredUnder = null; if (oname.equals(registrationFailuresLogged.putIfAbsent(poolName, oname))) { // The loud report went out with the first // refusal of this name; repeats ride along at 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 10b8127b16..b2972d16d0 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 @@ -71,6 +71,13 @@ import org.apache.tomcat.jdbc.pool.JdbcInterceptor; * release the name's broadcaster and failure-report bookkeeping along * with the slot.</p> * <p> + * The release of a name is decided atomically but runs as a later step, + * so it must neither tear down a registration that a newly started + * same-named pool has taken over, nor stop at anything but the ObjectName + * the registration was published under: the registry records the name its + * owner registered, and a release that finds a live pool other than the + * releasing one under the name leaves the whole name with that pool.</p> + * <p> * The notifications themselves must also be deliverable beyond this * JVM: {@code Notification} marshals its source, so the source has to be * serializable - the ObjectName the MBean is registered under, or, when @@ -445,17 +452,25 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { Assertions.assertSame(SlowQueryReportJmx.mbeans.get(poolName), interceptorOf(next, SlowQueryReportJmx.class), "the borrowing connection's interceptor instance must take over the slot"); + Assertions.assertEquals(oname, interceptorOf(next, SlowQueryReportJmx.class).registeredUnder, + "the taking-over registration must record the ObjectName it was" + + " published under, so its release unregisters exactly that"); } } } /* A live pool that reports every query through the JMX interceptor under the given name. */ private DataSource reportingPoolNamed(String name) { + return reportingPoolNamed(name, "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=0)"); + } + + /* A live pool whose reporting interceptor is configured by the given string. */ + private DataSource reportingPoolNamed(String name, String interceptorConfig) { DataSource ds = newDataSource(); // jmxEnabled=false removes the // pools' own ObjectName clash; the interceptor registers by name ds.getPoolProperties().setName(name); - ds.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" - + "(notifyPool=false,threshold=0)"); + ds.setJdbcInterceptors(interceptorConfig); ds.getPoolProperties().setInitialSize(0); return ds; } @@ -843,4 +858,159 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { Assertions.assertFalse(server.isRegistered(oname), "closing the restarted pool must release the registration again"); } + + @Test + void theLastReleaseOfTheNameUnregistersTheNameTheRegistrationWasPublishedUnder() throws Exception { + String poolName = "JmxOnameOwner-" + System.nanoTime(); + // the grammar of jdbcInterceptors keeps everything after the first + // '=' as the property value, so - as in the override test above - + // this single-key-property objectName is configurable + String customName = "tomcat.jdbc:type=SlowQueryReleased_" + System.nanoTime(); + DataSource publishingPool = reportingPoolNamed(poolName, + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=0,objectName=" + customName + ")"); + DataSource releasingPool = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName customOname = new ObjectName(customName); + ObjectName defaultOname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + try { + // the publishing pool's first borrow takes the per-name registry + // slot and publishes the MBean under its configured override + try (@SuppressWarnings("unused") Connection borrowing = publishingPool.getConnection()) { + Assertions.assertTrue(server.isRegistered(customOname), + "the publishing pool's borrow must publish under the configured override"); + } + + // the sharing pool's instances would name the default ObjectName, + // but they never publish - the registry slot is taken + Connection sharingConnection = releasingPool.getConnection(); + Assertions.assertFalse(server.isRegistered(defaultOname), + "the sharing pool's instances must not publish a second MBean"); + Assertions.assertTrue(server.isRegistered(customOname), + "the registration both pools report through stays published"); + sharingConnection.close(); + + // close the publishing pool while the sharing pool is still live: + // the registration stays with the survivor + publishingPool.close(); + Assertions.assertTrue(server.isRegistered(customOname), + "closing one of two live pools of the same name must leave the" + + " registration with the survivor"); + + // the survivor is the last live pool of the name; its close + // delivers the release on a fresh instance of its own interceptor + // definition - a definition that computes the default ObjectName + // and published nothing. The release must unregister the ObjectName + // the registration was published under, not the one the releasing + // instance happens to compute + releasingPool.close(); + Assertions.assertFalse(server.isRegistered(customOname), + "the last close of the name must unregister the ObjectName the" + + " registration was actually published under - the other" + + " pool's configured override - instead of giving up on a" + + " name this pool's own definition computes but never" + + " registered, which leaks the published MBean for ever"); + Assertions.assertFalse(server.isRegistered(defaultOname), + "and no registration may linger under the default shape either"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "the per-name registry slot must be released"); + } finally { + for (ObjectName candidate : new ObjectName[] { customOname, defaultOname }) { + try { + server.unregisterMBean(candidate); + } catch (InstanceNotFoundException ignore) { + // nothing registered left to clean up under this name + } + } + } + } + + @Test + void aLateNameReleaseLeavesAPoolThatTookTheNameOverUntouched() throws Exception { + String poolName = "JmxLateRelease-" + System.nanoTime(); + DataSource oldPool = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + List<Notification> received = new CopyOnWriteArrayList<>(); + NotificationListener listener = (notification, handback) -> received.add(notification); + try { + // the old pool's first borrow publishes the MBean under the name, + // a slow query creates the name's broadcaster, and a client + // attaches its listener through the registration + Connection oldConnection = oldPool.getConnection(); + SlowQueryReportJmx oldOwner = interceptorOf(oldConnection, SlowQueryReportJmx.class); + try (Statement st = oldConnection.createStatement()) { + server.addNotificationListener(oname, listener, null, null); + st.execute(slowSql(1)); + } + oldConnection.close(); + Assertions.assertTrue(server.isRegistered(oname), + "the old pool's registration must be published under the name"); + + // The old pool's close now decides that it was the last live pool + // of the name - exactly the decision SlowQueryReport.poolClosed + // takes through releaseStatistics - but the release that follows + // the decision has not run yet. That is the state a pool starting + // under the name can catch the closing pool in; the window inside + // one close call is a thread race that no test can force + // deterministically, so the test drives the two decided steps + // apart, in the order the race leaves them. + Assertions.assertTrue(SlowQueryReport.releaseStatistics(oldPool.getPool()), + "the test's premise: the old pool must have been the last live pool of the name"); + + // meanwhile a new pool starts under the name, takes the statistics + // back and adopts the still-published registration the way a + // same-named sharing pool adopts it; its queries reach the client's + // listener through it + DataSource newPool = reportingPoolNamed(poolName); + try (Connection newConnection = newPool.getConnection(); + Statement st = newConnection.createStatement()) { + st.execute(slowSql(2)); + Assertions.assertTrue(received.stream().anyMatch(n -> n.getMessage().contains("SELECT 2")), + "the new pool's queries must reach the listener through the" + + " registration it adopted, received: " + received); + + // the old close now gets to its release, completing the + // decided statistics release the way poolClosed does + SlowQueryReportJmx lateRelease = new SlowQueryReportJmx(); + lateRelease.poolName = poolName; + lateRelease.releaseJmxRegistration(oldPool.getPool()); + + // the name was claimed back before the release ran: the new + // pool owns everything registered under it now + Assertions.assertTrue(server.isRegistered(oname), + "a release that finds the name taken back by a live pool must" + + " not unregister the MBean the new pool reports through"); + Assertions.assertSame(SlowQueryReportJmx.mbeans.get(poolName), oldOwner, + "the registry must still hold the registration the new pool adopted"); + Assertions.assertNotNull(SlowQueryReportJmx.notifiers.get(poolName), + "the broadcaster the new pool's clients are attached to must stay in place"); + + st.execute(slowSql(3)); + Assertions.assertTrue(received.stream().anyMatch(n -> n.getMessage().contains("SELECT 3")), + "the client's listener must still receive the new pool's" + + " notifications after the old pool's late release," + + " received: " + received); + } + + // and the taken-over name stays releasable by its last live pool + newPool.close(); + Assertions.assertFalse(server.isRegistered(oname), + "the last live pool of a taken-over name must still release the registration"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "the registry slot of the taken-over name must be released"); + Assertions.assertNull(SlowQueryReportJmx.notifiers.get(poolName), + "the broadcaster of the taken-over name must be released"); + } finally { + try { + server.removeNotificationListener(oname, listener); + } catch (ListenerNotFoundException | InstanceNotFoundException ignore) { + // the teardown under test may already have dropped the binding + } + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
