This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit fe26aeae113d42c380fed8571458891455e653e3 Author: opencode <[email protected]> AuthorDate: Wed Nov 4 13:29:42 2026 +0100 Make the slow query report's JMX registration hold together The reporting interceptor publishes one MBean per pool name, lazily on the first borrow, and releases it when the last live pool of the name closes. Publication and release are separate unguarded steps over a per-name registry, and this closes the ways that pairing came apart. A publish that lands behind the release of its own slot is an orphan nobody else knows of, and the registration is the only object that can still tell it apart from a live one; it now undoes such a publication - and does so about the ObjectName it occupies rather than the pool-name slot, because definitions of one name may compute different names, and an undo that read the slot left the bean standing under a name no release could ever reach. A release that cannot derive the name its registration occupies leaves that registration in place, so a later release can still take it down. Notifications are sent under the name the registration was published under rather than one recomputed from the emitting instance's properties, so the instances that never published - a two-thresholds chain, a pool that adopted a name another definition published - do not stamp them with a phantom source. A name that is already registered is noticed before the name is parsed and a publication built only to lose the race. A released name stays released: a notification still in flight while the last pool came down no longer puts the name back in the static registry, along with the listeners and the class loaders its broadcaster carries. A pool that carries no name cost the application its borrows, and a definition the pool took on after its start never met the start-time refusal meant to catch that pairing; it now loses its monitoring and nothing else, as every other unformable name does, and a value-less objectName property reads as absent for the same reason. Two neighbours carry their own change here. JmxUtil.unregisterJmx now answers whether the unregistration actually happened, with a new message for the case where the MBean is still registered afterwards - the module's callers run terminal lifecycles and still ignore the answer, but a refusal is no longer silent. ConnectionPool.close now also informs the definitions a connection chain was actually built from: a definition the mutable jdbcInterceptors property gained after the start and lost again before the close was named by neither of the sets the close worked through, yet its chains had registered state under the pool's name, so for a pool whose only reporting definition took that path the name's MBean outlived the pool by the life of the JVM. What is said about the interceptor now matches what it does: the descriptor carries the notification types the notifications have on the wire, with a test keeping the two from drifting; the module no longer claims the class needs Tomcat's JMX engine, which its own tests disprove by registering it in a plain JVM; the class javadoc opens with the notifyPool=false precondition the rest of its description silently assumes, and the branch implementing that precondition is no longer an empty if; the notification type constants are the pool MBean interface's own constants rather than copies of their text; and what a persistently refused registration costs per borrow is documented, with the reason retrying every borrow is worth paying it. Pinned by aRegistrationCompletedBehindItsOwnReleaseLeavesNothingPublished, aRegistrationCompletedBehindAReleaseAndATakeoverLeavesTheSuccessorAlone, aReleaseThatCannotDeriveTheNameKeepsTheRegistrationReleasable, aMalformedObjectNameOverrideFailsOnlyTheRegistration, everyDefinitionOfANameSendsTheNameTheRegistrationWasPublishedUnder, aNullObjectNamePropertyValueBehavesAsIfAbsent, theDefinitionThatBuiltALiveChainIsToldAboutTheCloseEvenIfThePropertyLostItAgain, aDefinitionAddedAfterTheStartAndRemovedBeforeTheCloseStillReleasesThePublishedMBean, aRegisteredNameCostsNoFurtherNameFormationPerBorrow, aLatePublishUnderANameTheRegistryNoLongerRecordsUndoesItself, aNotificationSentAfterTheNameWasReleasedDoesNotBringTheNameBack, aJmxReportingDefinitionAddedToANamelessPoolAfterTheStartStillServesItsBorrows, aSlowQueryOfANamelessPoolWithALateReportingDefinitionIsServedAndStillNotified, aSuccessfulUnregistrationComesBackAsReleased, anUnregistrationOfNothingCountsAsDone, aRefusedUnregistrationComesBackAsNotReleased, mBeanInfoAdvertisesTheNotificationTypesTheMBeanSends and theDescriptorAdvertisesTheWireTypesTheNotificationsCarry. --- modules/jdbc-pool/doc/jdbc-pool.xml | 10 +- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 56 ++- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 338 ++++++++++++-- .../jdbc/pool/interceptor/mbeans-descriptors.xml | 15 +- .../org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java | 26 +- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../TestInterceptorRemovalBeforePoolClose.java | 63 +++ .../interceptor/TestSlowQueryJmxMBeanSurface.java | 65 +++ .../interceptor/TestSlowQueryJmxNotifications.java | 500 ++++++++++++++++++++- .../TestSlowQueryReportNamelessPool.java | 117 ++++- .../tomcat/jdbc/pool/jmx/TestJmxRegistration.java | 103 ++++- 11 files changed, 1216 insertions(+), 78 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index 874824a5f8..ebf7db0242 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -889,13 +889,15 @@ <subsection name="org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx"> <p>Extends the <code>SlowQueryReport</code> and in addition to log entries it issues JMX notification for monitoring tools to react to. Inherits all the attributes from its parent class. - This class uses Tomcat's JMX engine so it won't work outside of the Tomcat container. - By default, JMX notifications are sent through the ConnectionPool mbean if it is enabled. - The <code>SlowQueryReportJmx</code> can also register an MBean if <code>notifyPool=false</code> + This class registers with the JVM's platform MBeanServer, so it works in any JVM, inside or + outside a Tomcat container. By default, JMX notifications are sent through the ConnectionPool + mbean if it is enabled; with <code>notifyPool=false</code> the <code>SlowQueryReportJmx</code> + instead publishes an MBean of its own, one per pool name, and sends the notifications through it. </p> <attributes> <attribute name="notifyPool" required="false"> - <p>(boolean as String) Set to false if you want JMX notifications to go to the <code>SlowQueryReportJmx</code> MBean + <p>(boolean as String) Set to false if you want JMX notifications to go to the <code>SlowQueryReportJmx</code> + MBean rather than to the pool's own MBean. Doing so also publishes that interceptor's MBean, described above. The default value is <code>true</code>. </p> </attribute> 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 1888916a3f..f82e013215 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 @@ -37,6 +37,7 @@ import java.util.Timer; import java.util.TimerTask; import java.util.concurrent.BlockingQueue; import java.util.concurrent.CancellationException; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; @@ -158,6 +159,20 @@ public class ConnectionPool { */ private volatile PoolProperties.InterceptorDefinition[] startedInterceptors; + /** + * The interceptor definitions a connection chain of this pool was actually built from. The close pairs with + * {@link #startedInterceptors} and with the definitions the {@code jdbcInterceptors} property lists at close + * time, but neither of those covers a definition the property gained after the start and lost again before the + * close: it never saw the start to be recorded there, and by the close it is gone from the list the close reads + * - yet a chain was built from it, and what a borrow through that chain registered under the pool's name (the + * statistics and, for the reporting interceptors, the published MBean) is only ever taken down by a close. + * Recording the definitions at chain construction is what lets the close find that third set. Entries are the + * small configuration objects the property parses, one per definition per reconfiguration of a live pool; a + * restarted pool is a new instance pairing over its own generation. + */ + private final Set<PoolProperties.InterceptorDefinition> chainDefinitions = + ConcurrentHashMap.newKeySet(); + /** * 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 @@ -472,6 +487,10 @@ public class ConnectionPool { // Set up the interceptor chain PoolProperties.InterceptorDefinition[] proxies = getPoolProperties().getJdbcInterceptorsAsArray(); for (int i = proxies.length - 1; i >= 0; i--) { + // Record what this chain was built from: the definitions the + // close has to pair with are the ones a chain ran, not only + // the ones the property lists when the pool comes down + chainDefinitions.add(proxies[i]); try { // Create a new instance JdbcInterceptor interceptor = proxies[i].getInterceptorClass().getConstructor().newInstance(); @@ -773,8 +792,13 @@ public class ConnectionPool { // gained since the start are told as well, so a definition that never // saw a start still takes down - best effort, on a fresh instance that // shares no state with the chains that served the borrows - whatever a - // borrow through it registered under the pool's name. A definition the - // start told is not told twice over its own unchanged configuration. + // borrow through it registered under the pool's name. The third set the + // close has to reach are the definitions those chains were actually + // built from: a definition the property gained after the start and lost + // again before the close is named by neither of the lists above, yet it + // armed the connections that registered state under the pool's name. + // A definition the close already told over its start or its current + // configuration is not told twice. PoolProperties.InterceptorDefinition[] started = startedInterceptors; if (started != null) { for (int i = 0; i < started.length; i++) { @@ -783,10 +807,15 @@ public class ConnectionPool { } PoolProperties.InterceptorDefinition[] proxies = getPoolProperties().getJdbcInterceptorsAsArray(); for (int i = 0; i < proxies.length; i++) { - if (!isToldAboutTheStart(proxies[i], started)) { + if (!isToldAlready(proxies[i], started)) { informInterceptorOfPoolClosure(proxies[i]); } } + for (PoolProperties.InterceptorDefinition chainDefinition : chainDefinitions) { + if (!isToldAlready(chainDefinition, started) && !isToldAlready(chainDefinition, proxies)) { + informInterceptorOfPoolClosure(chainDefinition); + } + } } @@ -813,22 +842,23 @@ public class ConnectionPool { /** - * Whether the given definition is one the pool told about its start: the very definition object the start - * read, or a re-parsed one configured the same way. The parsed list is cached and only re-parsed when the - * property is rewritten, so an unchanged list answers by object identity alone. + * Whether the close of the given definition is already covered by a set of definitions the close has told (or + * is telling): the very definition object in that set, or one configured the same way. The parsed list is cached + * and only re-parsed when the property is rewritten, so an unchanged list answers by object identity alone. * * @param definition the definition to check - * @param started the definitions the start told, or {@code null} when the pool never told any + * @param told the definitions already covered by one set of the close's notifications, or {@code null} + * when that set does not exist - a pool whose start told nothing * - * @return {@code true} if the close of this definition is already covered by the start-time set + * @return {@code true} if the close of this definition is already covered by the given set */ - private static boolean isToldAboutTheStart(PoolProperties.InterceptorDefinition definition, - PoolProperties.InterceptorDefinition[] started) { - if (started == null) { + private static boolean isToldAlready(PoolProperties.InterceptorDefinition definition, + PoolProperties.InterceptorDefinition[] told) { + if (told == null) { return false; } - for (int i = 0; i < started.length; i++) { - if (started[i] == definition || isConfiguredTheSame(started[i], definition)) { + for (int i = 0; i < told.length; i++) { + if (told[i] == definition || isConfiguredTheSame(told[i], definition)) { return true; } } 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 b4d98b1bc1..57fca1ade4 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 @@ -26,6 +26,8 @@ import java.util.concurrent.atomic.AtomicLong; import javax.management.InstanceAlreadyExistsException; import javax.management.ListenerNotFoundException; import javax.management.MBeanNotificationInfo; +import javax.management.MBeanRegistration; +import javax.management.MBeanServer; import javax.management.MalformedObjectNameException; import javax.management.Notification; import javax.management.NotificationBroadcasterSupport; @@ -44,6 +46,7 @@ import org.apache.juli.logging.LogFactory; import org.apache.tomcat.jdbc.pool.ConnectionPool; import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorProperty; import org.apache.tomcat.jdbc.pool.PooledConnection; +import org.apache.tomcat.jdbc.pool.jmx.ConnectionPoolMBean; import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; import org.apache.tomcat.jdbc.pool.util.MessageFactory; @@ -51,6 +54,14 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * JMX-enabled slow query report interceptor. Publishes slow and failed query statistics to JMX and provides * notifications when slow or failed queries are detected. * <p> + * All of the publication described below belongs to the interceptor's own MBean, which exists only when the + * interceptor is configured with {@code notifyPool=false}. In the default {@code notifyPool=true} mode this class + * registers nothing of its own: the notifications are raised on the pool's own MBean instead, through + * {@link org.apache.tomcat.jdbc.pool.jmx.ConnectionPool#notify(String, String)}, and that MBean is registered by the + * pool itself when its {@code jmxEnabled} property is on - so a pool that is neither JMX-enabled nor configured with + * {@code notifyPool=false} reports its slow and failed queries to its log alone. + * </p> + * <p> * The MBean and the notification broadcaster are published for the pool <em>name</em>: the first interceptor instance * 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 @@ -74,13 +85,28 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; */ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationEmitter, SlowQueryReportJmxMBean { /** - * Notification type for slow query events. + * Notification type for slow query events. The pool's own MBean reports the same event under the same type when + * this interceptor is configured with {@code notifyPool=true}, so this is that very type and not a copy of its + * text: a client filters notifications on the type, and the two channels have to answer to one string. */ - public static final String SLOW_QUERY_NOTIFICATION = "SLOW QUERY"; + public static final String SLOW_QUERY_NOTIFICATION = ConnectionPoolMBean.SLOW_QUERY_NOTIFICATION; /** - * Notification type for failed query events. + * Notification type for failed query events, shared with the pool's own MBean for the same reason as + * {@link #SLOW_QUERY_NOTIFICATION}. */ - public static final String FAILED_QUERY_NOTIFICATION = "FAILED QUERY"; + public static final String FAILED_QUERY_NOTIFICATION = ConnectionPoolMBean.FAILED_QUERY_NOTIFICATION; + + /** + * The notification information every broadcaster of this class is built with, so that both the published MBean and + * the interceptor's own emitter surface advertise the two types above. JMX clients discover what an MBean can send + * through {@code MBeanInfo.getNotifications()} - consoles, monitoring and bridge agents, tooling that builds a + * notification filter from the advertised types - and a broadcaster constructed without this information leaves the + * MBean advertising none of the types it actually delivers. + */ + private static final MBeanNotificationInfo[] NOTIFICATION_INFO = { new MBeanNotificationInfo( + new String[] { SLOW_QUERY_NOTIFICATION, FAILED_QUERY_NOTIFICATION }, + Notification.class.getName(), + "A slow or failed query was detected by the slow query report.") }; /** * Property key for overriding the JMX object name. @@ -132,10 +158,12 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE protected static final ConcurrentHashMap<String, ObjectName> registrationFailuresLogged = new ConcurrentHashMap<>(); /** - * Notification broadcaster used before the pool name is known; once the pool name is known, {@link #getNotifier()} - * returns the broadcaster shared by all interceptor instances of that pool. + * Notification broadcaster used before the pool name is known, and the one a notification of a name that has been + * released is sent through; once the pool name is known and live, {@link #getNotifier()} returns the broadcaster + * shared by all interceptor instances of that pool. Like the shared broadcasters, it is built with + * {@link #NOTIFICATION_INFO}, so every handle's emitter surface advertises the types it sends. */ - protected volatile NotificationBroadcasterSupport notifier = new NotificationBroadcasterSupport(); + protected volatile NotificationBroadcasterSupport notifier = new NotificationBroadcasterSupport(NOTIFICATION_INFO); /** * Returns the notification broadcaster shared by all registrants of the given pool name. @@ -145,7 +173,7 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * @return the shared broadcaster registered for that name */ private static NotificationBroadcasterSupport broadcasterFor(String name) { - return notifiers.computeIfAbsent(name, key -> new NotificationBroadcasterSupport()); + return notifiers.computeIfAbsent(name, key -> new NotificationBroadcasterSupport(NOTIFICATION_INFO)); } /** @@ -162,6 +190,35 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE return broadcasterFor(name); } + /** + * Returns the broadcaster that the notifications of this interceptor's pool are sent through. Unlike + * {@link #getNotifier()} this one reads rather than provides: a send is not a reason for a name to exist. The + * name's broadcaster is made for a live pool - including one whose registration was refused, which reports + * through it all the same - but a name that no live pool holds any more has been released, and an entry put back + * under it by a notification that was simply still in flight when the last pool came down is taken down by + * nothing ever again: not the close that already ran, and not a later one, because no later pool of that name + * exists. What such a send can still serve is whoever reached this instance, so it falls back to the + * instance's own broadcaster. + * + * @return the broadcaster to send a notification of this interceptor's pool through + */ + private NotificationBroadcasterSupport broadcasterToSendTo() { + String name = poolName; + if (name == null) { + return notifier; + } + NotificationBroadcasterSupport shared = notifiers.get(name); + if (shared != null) { + return shared; + } + if (mbeans.containsKey(name) || getPoolStats(name) != null) { + // A pool of the name is live: its broadcaster is built by the first + // thing that needs it, and a send of a live pool is one + return broadcasterFor(name); + } + return notifier; + } + /** * Adds a notification listener. * @@ -412,7 +469,7 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE jmx.notify(type, query); } } else { - NotificationBroadcasterSupport target = getNotifier(); + NotificationBroadcasterSupport target = broadcasterToSendTo(); if (target != null) { Notification notification = new Notification(type, getNotificationSource(), sequence, System.currentTimeMillis(), query); @@ -435,24 +492,44 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * serializable. This MBean is not, and neither is the pool it reports on, so the source is the {@link ObjectName} * the MBean is registered under, which names the pool and is serializable. Interceptor instances are created per * pooled connection and none of them is the published resource - that is the pool name's {@link Registration} - so - * an instance cannot name itself: every instance of a pool name sends the very name its registration was published - * under. An instance whose name cannot be formed - a malformed {@code objectName} property or a pool name that - * cannot be quoted into an ObjectName - was never registered at all, so no listener reached through a connector can - * exist for it; the source is then a string of the default name's shape, which still names the pool for the local - * listeners the broadcaster serves. + * an instance cannot name itself: the source is the name the registry's registration records as published, which + * every instance of the name sends alike. Instances of a second definition under the same name - a two-thresholds + * chain, or a pool that adopted a name another definition published - never published anything themselves and must + * not stamp the notifications with the name their own properties would compute: that name is registered under + * nothing, and a client filtering or grouping by source reads the published one. An instance whose name cannot be + * formed - a malformed {@code objectName} property or a pool name that cannot be quoted into an ObjectName - was + * never registered at all, so no listener reached through a connector can exist for it; the source is then a string + * of the default name's shape, which still names the pool for the local listeners the broadcaster serves. A pool + * that carries no name at all is the same case taken to its end: it holds no registration to name itself by, and + * the shape its source takes is the default one with no name in it - whatever survives delivery, it cannot name a + * pool that never had a name to register. * - * @return the ObjectName the registration of this interceptor's pool name is published under, or, when that name - * cannot be formed, a string of the default name's shape + * @return the ObjectName the registration of this interceptor's pool name is published under, or, when nothing is + * registered under the name or the published name cannot be told, a name formed from this instance - + * as an ObjectName where that succeeds, and as a string of the default name's shape where it does not */ protected Object getNotificationSource() { + String name = poolName; try { - return getObjectName(getClass(), poolName); - } catch (MalformedObjectNameException x) { + // A pool that carries no name holds no registration to name itself + // by: nothing can be keyed under no name (see registerJmx), and the + // registry would refuse the lookup before answering it + SlowQueryReportJmxMBean registered = name == null ? null : mbeans.get(name); + if (registered != null) { + // The name as that registration itself recorded it: the same + // derivation the release path uses to unregister exactly the + // occupied name, for the same reason - a name recomputed from + // the properties of the instance that happens to be sending + // can differ from the one the MBeanServer actually occupies + return registrationObjectName(registered); + } + return getObjectName(getClass(), name); + } catch (MalformedObjectNameException | RuntimeOperationsException x) { // The name construction getObjectName uses when no override is // configured - with a malformed name this MBean was never // registered, so this fallback is only read by local listeners, // for which it must still name the pool - return ConnectionPool.POOL_JMX_TYPE_PREFIX + getClass().getName() + ",name=" + poolName; + return ConnectionPool.POOL_JMX_TYPE_PREFIX + getClass().getName() + ",name=" + name; } } @@ -623,24 +700,41 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * 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. 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. + * failure of a later failure cycle under the name. Both entries are created lazily - by a report of a name that + * is still live 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. A notification that is merely still in flight while this release runs is not given that power, as + * broadcasterToSendTo() reads the name's broadcaster instead of providing one: an intent to listen is a reason to + * give the name a broadcaster again, a send that lost the race to the close is not. + * </p> + * <p> + * The ObjectName to unregister is derived while the registry still holds the slot, before the slot is dropped: the + * derivation can fail for a registration this class did not publish itself (see {@link + * #registrationObjectName(SlowQueryReportJmxMBean)}), and a release that cannot tell the MBeanServer which name to + * take down must leave the slot held so that a later release still can. Dropping the slot on the way to a failed + * derivation would strand the published MBean behind a registry that no longer records it - the one half-released + * state no later release of the name can repair. * </p> */ protected void deregisterJmx() { + SlowQueryReportJmxMBean registered = mbeans.get(poolName); try { - SlowQueryReportJmxMBean registered = mbeans.get(poolName); - if (registered != null && mbeans.remove(poolName, registered)) { - JmxUtil.unregisterJmx(registrationObjectName(registered)); + if (registered != null) { + // Derivation first, while the slot is still held: if it + // throws, the registry still records the published MBean and + // a later release of the name can still take it down + ObjectName name = registrationObjectName(registered); + if (mbeans.remove(poolName, registered)) { + JmxUtil.unregisterJmx(name); + } } - notifiers.remove(poolName); - registrationFailuresLogged.remove(poolName); } catch (MalformedObjectNameException | RuntimeOperationsException e) { log.warn(MessageFactory.get("slowQueryReportJmx.jmxDeregistrationFailed"), e); + } finally { + notifiers.remove(poolName); + registrationFailuresLogged.remove(poolName); } - } /** @@ -680,6 +774,14 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE /** * Returns the JMX ObjectName for the given class and pool name. + * <p> + * A property carrying no value - which the interceptor-string grammar + * cannot produce, but a programmatically built {@link + * org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorDefinition} can + * hand through - names nothing to override, and reads as if the property + * were absent, the way every sibling property of this family reads its + * null value. + * </p> * * @param clazz the interceptor class * @param poolName the name of the connection pool @@ -691,8 +793,9 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE public ObjectName getObjectName(Class<?> clazz, String poolName) throws MalformedObjectNameException { ObjectName oname; Map<String, InterceptorProperty> properties = getProperties(); - if (properties != null && properties.containsKey(objectNameAttribute)) { - oname = new ObjectName(properties.get(objectNameAttribute).getValue()); + InterceptorProperty override = properties == null ? null : properties.get(objectNameAttribute); + if (override != null && override.getValue() != null) { + oname = new ObjectName(override.getValue()); } else { oname = new ObjectName(ConnectionPool.POOL_JMX_TYPE_PREFIX + clazz.getName() + ",name=" + poolName); } @@ -709,16 +812,57 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * and skip the registration, so the pool would claim an MBean for its whole life that was never published - and * would not re-register even after whatever made the MBeanServer refuse it is gone. Releasing the slot lets a later * borrow retry; {@code JmxUtil.registerJmx} cannot be used for the attempt because it swallows every exception and - * returns a name whatever happened, so the failure of its own call is unobservable to the caller. Only the first - * refusal of a name is logged with its exception, so a persistent refusal is not reported once per borrow. + * returns a name whatever happened, so the failure of its own call is unobservable to the caller. The retry is not + * made cheaper than the attempt it repeats, and that is deliberate: the first borrow that finds the name free + * publishes it, so a clash with a stale or foreign MBean costs monitoring only for as long as that MBean is there, + * while a backoff would make every pool that ever clashed wait out a delay before its monitoring came back. What a + * persistent refusal therefore costs is one publish attempt per borrow - the name formed, a {@link Registration} + * built, and the MBeanServer's refusal - on a pool whose monitoring is misconfigured, which is the cost paid for + * that recovery. Only the first + * refusal of a name is logged with its exception, so a persistent refusal is not reported once per borrow. A borrow + * that finds the slot held is skipped before the ObjectName is formed: forming it and building a {@link + * Registration} for that race can only ever lose the race, and the healthy path pays that price once per pool + * name rather than once per borrow. A pool that carries no name is skipped before any of it: no name is not a + * key the registry takes, and the start-time refusal of such a pool (see {@link SlowQueryReport#poolStarted}) + * does not reach a definition the mutable interceptor property hands over later, so this is where that pairing + * is met - and it costs the monitoring, never the borrow. + * The one outcome this method cannot observe - a publication that lands after a release already let go of the slot + * it took - is undone by the {@link Registration} itself, which checks its own publication as the server commits + * it (see {@link Registration#postRegister(Boolean)}). */ protected void registerJmx() { try { - // Only if we notify the pool itself + // With the pool notified, the notifications go to the pool's own + // MBean and there is nothing for this interceptor to register if (isNotifyPool()) { - - } else if (getCompositeType() != null) { - ObjectName oname = getObjectName(getClass(), poolName); + return; + } + if (getCompositeType() != null) { + String name = poolName; + if (name == null) { + // The pool carries no name, and no name is not a name: the + // registry, the failure throttle and the ObjectName are all + // keyed by it, and the map takes no null key. A definition + // the pool was started with refuses this pool at its start + // with a message naming the missing setting (see + // SlowQueryReport#poolStarted); a definition the mutable + // interceptor property hands over later never runs that + // start callback, so the borrow path is where the pairing + // shows up - and a borrow is not the place to decide it. + // Declining quietly costs the monitoring, which is all a + // name that cannot be had can ever cost. + return; + } + if (mbeans.containsKey(name)) { + // The name holds a registration - published, or being + // published by a concurrent borrow: this borrow cannot + // take the slot either way, so it forms no ObjectName and + // builds no Registration just to lose the race to its own + // name's registration. A refused publish frees the slot + // again, so a retry needs no fresh look past this read. + return; + } + ObjectName oname = getObjectName(getClass(), name); // The registry and the MBeanServer keep the registration // resource, never this instance: an instance reaches the pool // (its own and the inherited pool fields), so publishing one @@ -726,8 +870,8 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE // abandoned without closing - the only path that ever takes a // registration down again is a close, which abandonment // never runs - Registration registration = new Registration(poolName, oname); - if (mbeans.putIfAbsent(poolName, registration) == null) { + Registration registration = new Registration(name, oname); + if (mbeans.putIfAbsent(name, registration) == null) { try { // The registration carries the ObjectName it is to be // published under from its construction: a release @@ -737,10 +881,10 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE .registerMBean(registration, oname); // A registration under the name works again - drop // the marker so a later failure cycle reports loudly - registrationFailuresLogged.remove(poolName); + registrationFailuresLogged.remove(name); } catch (Exception x) { - mbeans.remove(poolName, registration); - if (oname.equals(registrationFailuresLogged.putIfAbsent(poolName, oname))) { + mbeans.remove(name, registration); + if (oname.equals(registrationFailuresLogged.putIfAbsent(name, oname))) { // The loud report went out with the first // refusal of this name; repeats ride along at // debug, without a stack trace, so a persistent @@ -752,7 +896,7 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE // First refusal for the name, or the name now // fails under a different ObjectName: report it // loudly and remember to throttle the repeats - registrationFailuresLogged.put(poolName, oname); + registrationFailuresLogged.put(name, oname); if (x instanceof InstanceAlreadyExistsException) { // Retryable: the name may be held by a stale // or foreign MBean that a later borrow finds @@ -769,6 +913,17 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE SlowQueryReport.class.getName())); } } catch (MalformedObjectNameException | RuntimeOperationsException e) { + // A name that cannot be formed - a malformed objectName property, + // or a pool name carrying characters the ObjectName grammar + // reserves - fails here, before the registry slot and the + // registrationFailuresLogged throttle are reachable: there is no + // ObjectName to record against the name, so the per-name throttle + // cannot key this failure. The error is therefore reported on + // every borrow of such a pool. That is acceptable: the condition + // is a permanent misconfiguration, the stack trace is the signal + // that makes the operator fix it, and a busy pool with a + // misnamed monitoring MBean is rarer - and less costly - than a + // suppressed error nobody ever acts on. log.error(MessageFactory.get("slowQueryReportJmx.jmxRegistrationFailedNoData"), e); } } @@ -807,8 +962,22 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE * The class is public because the Standard-MBean introspection publishes the resource through its interfaces; * instances are created only by {@link #registerJmx()}. * </p> + * <p> + * A registration is valid only while the {@link #mbeans} slot of its name holds it. The publication and the + * release of a name are separate, unguarded steps, so a publication can land behind a release of its own slot: + * the release takes the slot out and unregisters a name nothing was published under yet, and the publication then + * completes with nothing left in the JVM that still knows of it. The registration is the one object that can tell + * its own publication apart from a name already let go of - {@link #postRegister(Boolean)} is the server + * announcing that its repository has taken the registration, and a registration that finds its slot gone by then + * takes that late publication back down itself, leaving the name free for the next pool to register it cleanly. + * A slot by then held by another registration is not gone, but it only settles the question for the name that + * registration occupies: a takeover published under this very name can only have got there after the release took + * this publication down, so there is nothing of this one left to undo and the takeover is left alone, while a + * takeover published under another name leaves this publication standing under a name the registry no longer + * records - which nothing but the registration itself can still give up. + * </p> */ - public static class Registration implements NotificationEmitter, RegistrationMBean { + public static class Registration implements NotificationEmitter, RegistrationMBean, MBeanRegistration { /** * The pool name this registration reports for. */ @@ -832,6 +1001,87 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE this.publishedUnder = publishedUnder; } + /* + * The registration is published under exactly the ObjectName it was + * constructed with; that name is also what a release unregisters, so + * the server is answered with the name it asked about, unchanged. + */ + @Override + public ObjectName preRegister(MBeanServer server, ObjectName name) throws Exception { + return name; + } + + /* + * A registration is published under its ObjectName and held by the + * per-name slot, and those are two different questions: two + * definitions of one pool name may compute different ObjectNames (see + * registrationObjectName), so what the slot answers is not what decides + * whether unregistering this name destroys somebody else's live MBean. + * postRegister is the MBeanServer announcing that its repository has + * taken this registration, the earliest point at which the outcome of + * the publication is known here. A publication whose slot is gone was + * decided away by a release that ran while it was in flight - the + * release took the slot out and unregistered a name nothing had been + * published under yet, so nothing else in the JVM knows to take this + * now-late publication down. A publication whose slot is held by + * another registration is only accounted for when that registration + * occupies this very name: it can have committed there only after this + * publication was already given up, which is why unregistering now + * would take down the live MBean of the pool that took the name over, + * with the registry correctly claiming that one is published. Under a + * different name the successor's commit never needed this publication + * to be given up first, the registry records only the successor's name, + * and no release of the pool can ever reach this one - so the + * registration is the only object that can still undo it. A + * registration refused before its commit was never published and keeps + * its name - if any - with whoever else holds it, which is why only a + * completed registration here undoes anything. + */ + @Override + public void postRegister(Boolean registrationDone) { + if (Boolean.TRUE.equals(registrationDone) && publicationIsLeftWithoutOwner()) { + JmxUtil.unregisterJmx(publishedUnder); + } + } + + /* + * Whether this publication, at the moment its commit is announced, + * stands under a name nothing in this JVM can give up any more. + */ + private boolean publicationIsLeftWithoutOwner() { + SlowQueryReportJmxMBean held = mbeans.get(registrationPoolName); + if (held == null) { + // The release ran while this publication was in flight: it is + // an orphan, and the name it carries is free for the next pool + return true; + } + if (held == this) { + // The registry holds this publication: it is the name's own + return false; + } + if (held instanceof Registration) { + // A takeover - and the only question that matters is whether it + // occupies this name or came to the slot by another one + return !((Registration) held).publishedUnder.equals(publishedUnder); + } + // A value a subclass put into the registry itself: the name it + // occupies cannot be told from here, so it is given the field and + // this publication leaves the name alone + return false; + } + + @Override + public void preDeregister() throws Exception { + // nothing to guard: the registry bookkeeping runs around the + // unregistration in deregisterJmx(), not in the resource + } + + @Override + public void postDeregister() { + // nothing to release: the resource holds no state beyond the two + // fields a construction gave it + } + @Override public String getPoolName() { return registrationPoolName; diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/mbeans-descriptors.xml b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/mbeans-descriptors.xml index 52eed076ce..b50d78ab1c 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/mbeans-descriptors.xml +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/mbeans-descriptors.xml @@ -20,19 +20,22 @@ "http://jakarta.apache.org/commons/dtds/mbeans-descriptors.dtd";> <mbeans-descriptors> - <mbean description="Reports " domain="tomcat.jdbc" group="jdbc-pool" name="SlowQueryReportJmx" + <mbean description="Reports slow and failed queries through JMX and publishes the statistics of every pool registered under its name" domain="tomcat.jdbc" group="jdbc-pool" name="SlowQueryReportJmx" type="org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx"> <attribute description="The name of the connection pool this Jmx bean is representing" name="PoolName" type="java.lang.String" writeable="false"/> <attribute description="List of all registered connections pools" name="PoolNames" type="[Ljava.lang.String;" writeable="false"/> - <attribute description="All the recorded query stats. " name="SlowQueriesCD" type="[Ljavax.management.openmbean.CompositeData;" writeable="false"/> + <attribute description="All the recorded query stats, as one CompositeData per recorded query" name="SlowQueriesCD" type="[Ljavax.management.openmbean.CompositeData;" writeable="false"/> - <notification description="Notification sent out by the slow query report when a query exceeds the threshold" name="slow-query"> - <notification-type>Slow query</notification-type> + <!-- The notification-type values are the wire types of the notifications the MBean + sends; they must stay equal to the SLOW_QUERY_NOTIFICATION and + FAILED_QUERY_NOTIFICATION constants, which is pinned by the surface test. --> + <notification description="Notification sent out by the slow query report when a query exceeds the threshold" name="SLOW QUERY"> + <notification-type>SLOW QUERY</notification-type> </notification> - <notification description="Notification sent out by the slow query report when a query fails execution" name="failed-query"> - <notification-type>Failed query execution</notification-type> + <notification description="Notification sent out by the slow query report when a query fails execution" name="FAILED QUERY"> + <notification-type>FAILED QUERY</notification-type> </notification> <operation description="Clears all the query stats" impact="ACTION" name="resetStats" returnType="void"/> diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java index 145599ee7f..b3dcc4c7c4 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/jmx/JmxUtil.java @@ -19,6 +19,7 @@ package org.apache.tomcat.jdbc.pool.jmx; import java.lang.management.ManagementFactory; +import javax.management.InstanceNotFoundException; import javax.management.MalformedObjectNameException; import javax.management.ObjectName; @@ -71,16 +72,33 @@ public class JmxUtil { /** * Unregister an MBean. * - * @param oname the object name to unregister + * @param oname the object name to unregister, or {@code null} to release + * nothing + * + * @return {@code true} if the name is no longer registered - either this + * call took it down, or it was already gone, or there was + * nothing to release; {@code false} if the MBean server refused + * to unregister it, which means the MBean is still published + * under that name and, since callers drop their handle + * regardless, held by nothing for the rest of the JVM's life */ - public static void unregisterJmx(ObjectName oname) { + public static boolean unregisterJmx(ObjectName oname) { if (oname == null) { - return; + return true; } try { ManagementFactory.getPlatformMBeanServer().unregisterMBean(oname); + return true; + } catch (InstanceNotFoundException e) { + // The MBean is already down - the end state this call was asked to + // bring about, not a failure, and no caller can act on it + log.debug(MessageFactory.get("jmxUtil.jmxUnregistrationFailed") + " " + oname); + return true; } catch (Exception e) { - log.error(MessageFactory.get("jmxUtil.jmxUnregistrationFailed"), e); + // The MBean is still registered, and this name is now owned by + // nobody: say so, with the name, so an operator can take it down + log.error(MessageFactory.get("jmxUtil.jmxUnregistrationRefused", oname), e); + return false; } } 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 dc03354872..8c80cdd60c 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 @@ -155,6 +155,7 @@ jmxConnectionPool.droppingNotification=Dropping notification of type [{0}] for p jmxUtil.jmxRegistrationFailed=Jmx registration failed. jmxUtil.jmxUnregistrationFailed=Jmx unregistration failed. +jmxUtil.jmxUnregistrationRefused=Jmx unregistration of [{0}] was refused. The MBean is still registered under that name, and its caller releases the name regardless, so nothing will ever unregister it again: it keeps a live management surface and pins the component behind it for the life of the JVM. poolProperties.invalidInterceptorProperty=Invalid interceptor property ''{0}'', missing ''='', in jdbcInterceptors value [{1}] poolProperties.credentialClearStripsStringEntry=Clearing the ''{0}'' credential also removed the entry the connectionProperties string contributes for that key. The string still names it, so every connection established from now on goes without that credential, and a connectionProperties reconfigure that re-names the key applies it again. Take the credential out of the connectionProperties string to retire it. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorRemovalBeforePoolClose.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorRemovalBeforePoolClose.java index 5dd8c029b0..17e594ee33 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorRemovalBeforePoolClose.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestInterceptorRemovalBeforePoolClose.java @@ -202,4 +202,67 @@ class TestInterceptorRemovalBeforePoolClose extends JdbcPoolBaseTest { ds.close(); } } + + @Test + void theDefinitionThatBuiltALiveChainIsToldAboutTheCloseEvenIfThePropertyLostItAgain() + throws Exception { + LifecycleRecordingInterceptor.resetCounts(); + DataSource ds = poolNamed("AddedThenRemoved-" + System.nanoTime(), ""); + try { + ds.createPool(); + ds.setJdbcInterceptors(RECORDING + "(marker=transient)"); + try (@SuppressWarnings("unused") Connection con = ds.getConnection()) { + // the chain this connection runs was built from the added + // definition - the tie that the started list and the current + // list both miss once the property drops it again + } + ds.setJdbcInterceptors(""); + ds.close(); + + Assertions.assertEquals(0, LifecycleRecordingInterceptor.starts("transient"), + "a definition the pool never started must not be credited with a start"); + Assertions.assertEquals(1, LifecycleRecordingInterceptor.closes("transient"), + "a definition the property gained after the start and lost again before the" + + " close must still be told about the close: it built the chain a" + + " borrow ran, and whatever that chain registered under the pool's" + + " name only a close takes down - the started list never held it and" + + " the current list no longer does"); + } finally { + ds.close(); + } + } + + @Test + void aDefinitionAddedAfterTheStartAndRemovedBeforeTheCloseStillReleasesThePublishedMBean() + throws Exception { + String poolName = "AddedThenRemovedJmx-" + System.nanoTime(); + DataSource ds = poolNamed(poolName, ""); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + SlowQueryReportJmx.class.getName() + + ",name=" + poolName); + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + try { + ds.createPool(); + ds.setJdbcInterceptors(SlowQueryReportJmx.class.getName() + + "(notifyPool=false,threshold=0)"); + try (Connection con = ds.getConnection(); Statement st = con.createStatement()) { + st.execute("SELECT 1 FROM SYSTEM_RANGE(1,1)"); + } + Assertions.assertTrue(server.isRegistered(oname), + "the premise: a borrow through the added definition published the MBean" + + " for the pool name"); + + ds.setJdbcInterceptors(""); + ds.close(); + + Assertions.assertFalse(server.isRegistered(oname), + "the published MBean must not outlive the pool by the life of the JVM" + + " because the one definition of it was added after the start and" + + " removed again before the close - neither list the close works" + + " through ever names it, but its chain published the registration"); + Assertions.assertFalse(SlowQueryReportJmx.mbeans.containsKey(poolName), + "and the registry of published names must not keep the name either"); + } finally { + ds.close(); + } + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxMBeanSurface.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxMBeanSurface.java index 797e1998d5..70b04af034 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxMBeanSurface.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxMBeanSurface.java @@ -17,15 +17,22 @@ package org.apache.tomcat.jdbc.pool.interceptor; import java.lang.management.ManagementFactory; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.sql.Connection; import java.sql.Statement; import java.util.Arrays; import java.util.HashSet; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Collectors; import javax.management.MBeanAttributeInfo; import javax.management.MBeanInfo; +import javax.management.MBeanNotificationInfo; import javax.management.MBeanOperationInfo; import javax.management.MBeanServer; import javax.management.ObjectName; @@ -117,6 +124,64 @@ class TestSlowQueryJmxMBeanSurface extends JdbcPoolBaseTest { } } + @Test + void mBeanInfoAdvertisesTheNotificationTypesTheMBeanSends() throws Exception { + // JMX clients discover what an MBean can send through + // MBeanInfo.getNotifications() - consoles, monitoring and bridge + // agents, tooling that builds a NotificationFilterSupport from the + // advertised types. An MBean that sends its queries as the two wire + // types of this class but advertises none of them leaves that tooling + // blind whatever delivery itself does (the pool's own MBean declares + // its types the same way, see TestJmxNotificationTypeContract). + String poolName = "JmxNotifInfo-" + System.nanoTime(); + DataSource ds = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = objectNameFor(poolName); + + try (@SuppressWarnings("unused") Connection con = ds.getConnection()) { + Assertions.assertTrue(server.isRegistered(oname), + "the premise: the interceptor MBean must be registered"); + MBeanNotificationInfo[] advertisedInfo = server.getMBeanInfo(oname).getNotifications(); + Set<String> advertised = advertisedInfo == null ? new HashSet<>() + : Arrays.stream(advertisedInfo).flatMap(i -> Arrays.stream(i.getNotifTypes())) + .collect(Collectors.toSet()); + Assertions.assertEquals(new HashSet<>(Arrays.asList( + SlowQueryReportJmx.SLOW_QUERY_NOTIFICATION, + SlowQueryReportJmx.FAILED_QUERY_NOTIFICATION)), advertised, + "the published MBean must advertise exactly the notification types it sends"); + } + } + + @Test + void theDescriptorAdvertisesTheWireTypesTheNotificationsCarry() throws Exception { + // mbeans-descriptors.xml is this MBean's legacy metadata, and like the + // published MBeanInfo it is material a tool builds its filters from: + // a notification type that differs from the wire string makes a filter + // built from the metadata match nothing. The constants are the wire + // contract (see TestJmxNotificationTypeContract), so the descriptor + // must carry exactly them, and no half-written descriptions. + Path descriptor = Paths.get("src", "main", "java", "org", "apache", "tomcat", + "jdbc", "pool", "interceptor", "mbeans-descriptors.xml"); + String xml = new String(Files.readAllBytes(descriptor), StandardCharsets.UTF_8); + + Set<String> advertised = new HashSet<>(); + Matcher types = Pattern.compile("<notification-type>([^<]+)</notification-type>") + .matcher(xml); + while (types.find()) { + advertised.add(types.group(1)); + } + Assertions.assertEquals(new HashSet<>(Arrays.asList( + SlowQueryReportJmx.SLOW_QUERY_NOTIFICATION, + SlowQueryReportJmx.FAILED_QUERY_NOTIFICATION)), advertised, + "the descriptor must name the notification types by the exact strings the" + + " notifications carry on the wire - a client building its filter" + + " from the metadata must match what the MBean actually sends"); + + Assertions.assertFalse(Pattern.compile("description=\"[^\"]*\\s\"").matcher(xml).find(), + "no description may dangle half-written on a trailing space"); + } + @Test void poolNameAndPoolNamesAreReadableThroughTheServer() throws Exception { String poolName = "JmxPoolName-" + System.nanoTime(); 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 7476786461..f03511bd67 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 @@ -25,15 +25,20 @@ import java.lang.reflect.Proxy; import java.sql.Connection; import java.sql.SQLException; import java.sql.Statement; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import javax.management.InstanceNotFoundException; import javax.management.ListenerNotFoundException; import javax.management.MBeanServer; +import javax.management.MalformedObjectNameException; import javax.management.Notification; import javax.management.NotificationListener; import javax.management.ObjectName; @@ -44,6 +49,8 @@ import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; import org.apache.tomcat.jdbc.pool.DataSource; import org.apache.tomcat.jdbc.pool.JdbcInterceptor; +import org.apache.tomcat.jdbc.pool.PoolProperties; +import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorProperty; /** * JMX notifications of the {@code SlowQueryReportJmx} interceptor in @@ -77,7 +84,11 @@ import org.apache.tomcat.jdbc.pool.JdbcInterceptor; * 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> + * releasing one under the name leaves the whole name with that pool. The + * publication itself runs as a later step of the slot-taking borrow, so a + * publish can also land behind a release of its own slot - and must then + * undo itself, leaving the name publishable by the next pool instead of + * orphaned forever.</p> * <p> * The notifications themselves must also be deliverable beyond this * JVM: {@code Notification} marshals its source, so the source has to be @@ -255,6 +266,57 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { + " registered under, not by the default shape or the instance"); } + @Test + void everyDefinitionOfANameSendsTheNameTheRegistrationWasPublishedUnder() throws Exception { + String poolName = "JmxSourceTwoDefinitions-" + System.nanoTime(); + String customName = "tomcat.jdbc:type=SlowQueryOverride_" + System.nanoTime(); + DataSource ds = newDataSource(); + ds.setName(poolName); + // Two reporting definitions in one chain - the shape the two-thresholds + // configuration documents: the first definition in the chain takes the + // registration slot and publishes under its configured name, and the + // second definition's instances never publish - yet every instance of + // the name reports its slow queries through the name's broadcaster, + // which the published MBean answers on. + ds.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=0,objectName=" + customName + ");" + + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=0)"); + ds.getPoolProperties().setInitialSize(0); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName customOname = new ObjectName(customName); + ObjectName defaultOname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + List<Notification> received = new CopyOnWriteArrayList<>(); + NotificationListener listener = (notification, handback) -> received.add(notification); + + try (Connection con = ds.getConnection()) { + Assertions.assertTrue(server.isRegistered(customOname), + "the first definition of the chain wins the name and publishes under" + + " its configured ObjectName"); + Assertions.assertFalse(server.isRegistered(defaultOname), + "the second definition must not publish a second MBean under the name"); + interceptorOf(con, SlowQueryReportJmx.class) + .addNotificationListener(listener, null, null); + try (Statement st = con.createStatement()) { + st.execute(slowSql(1)); + } + } + + Assertions.assertEquals(2, received.size(), + "both reporting definitions of the chain see the query and notify"); + for (Notification notification : received) { + Assertions.assertEquals(customOname, notification.getSource(), + "every notification of a pool name must carry the ObjectName the" + + " registration was published under: a non-publishing instance" + + " recomputing the source from its own properties stamps the" + + " notification with a name nothing is registered under, so a" + + " client filtering by source silently misses it, and a client" + + " grouping by source sees a phantom MBean"); + } + } + @Test void notificationSourceFallsBackToAStringWhenTheObjectNameCannotBeFormed() throws Exception { // a pool name containing a colon cannot be quoted into the @@ -645,6 +707,67 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } } + /* + * A reporting interceptor that counts the ObjectNames its borrows form. + * Once a pool name holds its registration, a borrow cannot publish a + * second MBean under it - the slot race is already lost to the pool's own + * registration - so forming the name and building a Registration for that + * losing race is work a healthy pool pays per connection, per borrow. + */ + public static class CountingFormationReport extends SlowQueryReportJmx { + @Override + public ObjectName getObjectName(Class<?> clazz, String poolName) + throws MalformedObjectNameException { + NAME_FORMATIONS.computeIfAbsent(String.valueOf(poolName), k -> new AtomicInteger()) + .incrementAndGet(); + return super.getObjectName(clazz, poolName); + } + } + + private static final Map<String, AtomicInteger> NAME_FORMATIONS = new ConcurrentHashMap<>(); + + private static int nameFormations(String poolName) { + AtomicInteger count = NAME_FORMATIONS.get(poolName); + return count == null ? 0 : count.get(); + } + + @Test + void aRegisteredNameCostsNoFurtherNameFormationPerBorrow() throws Exception { + String poolName = "JmxFormation-" + System.nanoTime(); + DataSource ds = reportingPoolNamed(poolName, + CountingFormationReport.class.getName() + "(notifyPool=false,threshold=0)"); + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + CountingFormationReport.class.getName() + ",name=" + poolName); + try { + try (@SuppressWarnings("unused") Connection first = ds.getConnection()) { + Assertions.assertTrue(server.isRegistered(oname), + "the premise: the first borrow published the MBean under the name"); + int formationsAtRegistration = nameFormations(poolName); + Assertions.assertTrue(formationsAtRegistration >= 1, + "the borrow that publishes the name must have formed it"); + + try (@SuppressWarnings("unused") Connection second = ds.getConnection()) { + // nothing to do: the borrow itself is the measured step + } + try (@SuppressWarnings("unused") Connection third = ds.getConnection()) { + // a reused connection exercises the same path again + } + + Assertions.assertEquals(formationsAtRegistration, nameFormations(poolName), + "a name that already holds its registration must be noticed before" + + " the name is formed and a Registration built to lose the" + + " slot race: parsing a full ObjectName and allocating a" + + " publication that cannot happen on every borrow of a" + + " perfectly registered pool is cost with no possible use -" + + " and a refused registration still leaves the slot free, so" + + " the retry semantics of a failed publish are untouched"); + } + } finally { + ds.close(); + } + } + @Test void failedRegistrationFreesTheSlotSoALaterBorrowCanStillPublish() throws Exception { String poolName = "JmxRegFail-" + System.nanoTime(); @@ -793,6 +916,40 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } } + @Test + void aNotificationSentAfterTheNameWasReleasedDoesNotBringTheNameBack() throws Exception { + String poolName = "JmxLateNotify-" + System.nanoTime(); + DataSource ds = reportingPoolNamed(poolName); + SlowQueryReportJmx reporter; + try (Connection con = ds.getConnection()) { + reporter = interceptorOf(con, SlowQueryReportJmx.class); + try (Statement st = con.createStatement()) { + st.execute(slowSql(1)); + } + Assertions.assertNotNull(SlowQueryReportJmx.notifiers.get(poolName), + "the premise: reporting a query of a live pool reaches the name's" + + " broadcaster, which is created for it"); + } + ds.close(); + Assertions.assertNull(SlowQueryReportJmx.notifiers.get(poolName), + "the premise: closing the last pool of the name released that broadcaster"); + + // A query that was already running while the pool came down reports + // when it finishes, and the interceptor instance that served it outlives + // the close holding the name it reported under. + reporter.notifyJmx("SELECT 1", SlowQueryReportJmx.SLOW_QUERY_NOTIFICATION); + + Assertions.assertNull(SlowQueryReportJmx.notifiers.get(poolName), + "a notification sent for a name that is released must not put that name back" + + " in the static registry: nothing that runs for the dead name will" + + " take it down again, so the entry - and any listener a client still" + + " holds on the broadcaster it comes with, and the class loader behind" + + " it - outlive the pool by the life of the JVM, until some later pool" + + " happens to use the same name"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "and the send must not leave anything else registered under the released name"); + } + @Test void concurrentClosesOfSameNamedPoolsReleaseTheNameExactlyOnce() throws Exception { String poolName = "JmxRace-" + System.nanoTime(); @@ -1026,4 +1183,345 @@ class TestSlowQueryJmxNotifications extends JdbcPoolBaseTest { } } } + + @Test + void aRegistrationCompletedBehindItsOwnReleaseLeavesNothingPublished() throws Exception { + String poolName = "JmxLatePublish-" + System.nanoTime(); + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + + // registerJmx takes the per-name registry slot and publishes to the + // MBeanServer as two separate steps. The race this test pins is a + // release running between them: it takes the slot out and unregisters + // the name - which nothing has published under yet, so the + // unregistration finds nothing - and the publish then completes + // behind it, with no live state left that still knows about it. The + // window inside one borrow is a thread race no test can force + // deterministically, so - as in the late-release test above - the test + // drives the two decided steps apart in the order the race leaves them. + + // The state after registerJmx's first step: the slot holds the + // registration the borrow is about to publish. + SlowQueryReportJmx.Registration inFlight = + new SlowQueryReportJmx.Registration(poolName, oname); + Assertions.assertNull(SlowQueryReportJmx.mbeans.putIfAbsent(poolName, inFlight), + "the test's premise: the per-name slot must start free"); + + // The release that lands inside the window: it takes the slot it read + // out of the registry and unregisters the ObjectName that registration + // records - a name the MBeanServer has never seen. + SlowQueryReportJmx releaser = new SlowQueryReportJmx(); + releaser.poolName = poolName; + releaser.deregisterJmx(); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "the release must have let go of the slot the in-flight publish held"); + + // The stalled publish now completes. The MBeanServer takes it: the + // name was never published, so nothing collides with it. A registration + // that does not check its own publication against the registry is + // published from here on with nothing tracking it: no pool, no + // registry entry, no live bookkeeping - an orphan until the JVM exits, + // holding the name every later pool under it would register under. + server.registerMBean(inFlight, oname); + Assertions.assertFalse(server.isRegistered(oname), + "a publish that lands behind the release of its own slot must undo" + + " itself - the registration is the only object that can" + + " still tell its publication apart from a taken one"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "the undone publish must not leave a registry entry behind"); + + // The name must stay usable: a pool started after the undone publish + // must be able to register it cleanly, and release it again on close. + DataSource revived = reportingPoolNamed(poolName); + try (@SuppressWarnings("unused") Connection borrowed = revived.getConnection()) { + Assertions.assertTrue(server.isRegistered(oname), + "the first borrow of a pool started after the undone publish must" + + " publish the name - the undone orphan must not poison it"); + } + revived.close(); + Assertions.assertFalse(server.isRegistered(oname), + "closing the pool that took the name after the undone publish must release it"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "and release the per-name registry slot with it"); + } + + @Test + void aRegistrationCompletedBehindAReleaseAndATakeoverLeavesTheSuccessorAlone() + throws Exception { + String poolName = "JmxLatePostRegister-" + System.nanoTime(); + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + + // The undo in postRegister is aimed at the name of its own publish - + // which is exactly the name a takeover under the same pool name + // registers under. The race this test pins has three steps inside the + // window between the server taking a registration and calling its + // postRegister: the release of the name lands after the commit (so it + // takes this publication down), and a new pool then finds the slot and + // the name both free and publishes itself. When the stalled + // postRegister finally runs, the slot holds the successor - and a + // registration that reads "not my slot" as "no slot" takes the + // successor's live MBean down under it. As in the test above, the + // decided steps are driven apart in the order the race leaves them. + + // The borrow's registration: slot taken, publication committed. + SlowQueryReportJmx.Registration inFlight = + new SlowQueryReportJmx.Registration(poolName, oname); + Assertions.assertNull(SlowQueryReportJmx.mbeans.putIfAbsent(poolName, inFlight)); + server.registerMBean(inFlight, oname); + + // Step one inside the window: the release, which takes the slot it + // read out of the registry and unregisters the name this publication + // occupies - the release runs against the recorded name, so it does + // take this publication down. + SlowQueryReportJmx releaser = new SlowQueryReportJmx(); + releaser.poolName = poolName; + releaser.deregisterJmx(); + Assertions.assertFalse(server.isRegistered(oname), + "the premise of the takeover below: this release ran after the commit, so its" + + " unregistration took this publication down - which is also what made" + + " the name free again"); + + // Step two: a new pool under the same name finds the slot and the name + // free - freed by that very release - and publishes cleanly. + SlowQueryReportJmx.Registration successor = + new SlowQueryReportJmx.Registration(poolName, oname); + Assertions.assertNull(SlowQueryReportJmx.mbeans.putIfAbsent(poolName, successor), + "the release must have left the slot free for the takeover"); + server.registerMBean(successor, oname); + Assertions.assertTrue(server.isRegistered(oname), + "the successor's publication must have been accepted"); + + // The stalled postRegister of the first registration now runs - what + // the server would call at the end of the interrupted registerMBean. + inFlight.postRegister(Boolean.TRUE); + + try { + Assertions.assertTrue(server.isRegistered(oname), + "a late postRegister must leave the successor's MBean alone: the slot" + + " holding another registration means that release already took" + + " this publication down - there is nothing left of it to undo -" + + " and unregistering the name now destroys the live MBean of the" + + " pool that took the name over, silently and with the registry" + + " still claiming it is published"); + Assertions.assertSame(successor, SlowQueryReportJmx.mbeans.get(poolName), + "the successor must still hold the registry slot it took"); + } finally { + SlowQueryReportJmx janitor = new SlowQueryReportJmx(); + janitor.poolName = poolName; + janitor.deregisterJmx(); + if (server.isRegistered(oname)) { + server.unregisterMBean(oname); + } + } + } + + @Test + void aLatePublishUnderANameTheRegistryNoLongerRecordsUndoesItself() throws Exception { + String poolName = "JmxLatePublishOtherName-" + System.nanoTime(); + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + // Two definitions of one pool name may compute different ObjectNames - + // the configured override of one and the default shape of the other is + // enough (see registrationObjectName), and that is what separates this + // race from the one above. + ObjectName successorName = new ObjectName( + "tomcat.jdbc:type=LatePublishSuccessor_" + System.nanoTime()); + + // Driven in the order the race leaves the steps: the release lands + // while this publish is in flight, so its unregistration finds nothing + // to take down; a second pool under the same name, configured with a + // name of its own, takes the free slot and publishes there; and only + // then does the stalled publish commit. + SlowQueryReportJmx.Registration inFlight = + new SlowQueryReportJmx.Registration(poolName, oname); + Assertions.assertNull(SlowQueryReportJmx.mbeans.putIfAbsent(poolName, inFlight)); + + SlowQueryReportJmx releaser = new SlowQueryReportJmx(); + releaser.poolName = poolName; + releaser.deregisterJmx(); + + SlowQueryReportJmx.Registration successor = + new SlowQueryReportJmx.Registration(poolName, successorName); + Assertions.assertNull(SlowQueryReportJmx.mbeans.putIfAbsent(poolName, successor), + "the release must have left the slot free for the takeover"); + server.registerMBean(successor, successorName); + + try { + // The stalled publish commits last. The server calls its + // postRegister at the end of this call, and that is where the undo + // has to decide: the slot is held, but by a registration published + // under another name, so nothing accounts for this one. + server.registerMBean(inFlight, oname); + + Assertions.assertFalse(server.isRegistered(oname), + "a publish that lands behind the release of its slot must undo itself even" + + " when the slot is meanwhile held: the successor publishes under" + + " its own name, so the registry records no name this publication" + + " could ever be given up by - reading the occupied slot as" + + " accounting for it leaves this MBean published for the life of" + + " the JVM, under a name no release of the pool can reach"); + Assertions.assertTrue(server.isRegistered(successorName), + "and the undo must leave the successor's own publication alone"); + Assertions.assertSame(successor, SlowQueryReportJmx.mbeans.get(poolName), + "the successor must still hold the registry slot it took"); + } finally { + SlowQueryReportJmx janitor = new SlowQueryReportJmx(); + janitor.poolName = poolName; + janitor.deregisterJmx(); + for (ObjectName name : new ObjectName[] { oname, successorName }) { + if (server.isRegistered(name)) { + server.unregisterMBean(name); + } + } + } + } + + @Test + void aMalformedObjectNameOverrideFailsOnlyTheRegistration() throws Exception { + // An override value that is not an ObjectName at all (no domain + // separator) fails the name formation on every borrow, before the + // registry slot and its refusal throttle are reachable - the failure + // is reported each time (see the acceptability comment in + // registerJmx). What must stay true while it is: the failure touches + // nothing but the monitoring, never latches the slot, and leaves + // nothing under the name behind. + String poolName = "JmxMalformedOname-" + System.nanoTime(); + DataSource ds = reportingPoolNamed(poolName, + "org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=-1,objectName=value-without-a-domain)"); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + Set<ObjectName> registeredBefore = server.queryNames(new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",*"), null); + try (@SuppressWarnings("unused") Connection first = ds.getConnection()) { + // the borrow itself served: a failed name formation cannot fail + // the pool's operation, only its monitoring + Assertions.assertEquals(registeredBefore, server.queryNames(new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",*"), null), + "no registration may appear while the name cannot be formed"); + } + try (@SuppressWarnings("unused") Connection second = ds.getConnection()) { + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "a name that cannot be formed must never latch the registry slot"); + Assertions.assertNull(SlowQueryReportJmx.registrationFailuresLogged.get(poolName), + "and nothing may join the per-name bookkeeping through a failure" + + " that cannot form its ObjectName"); + } + + ds.close(); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "and closing the pool leaves no registry entry under the name"); + Assertions.assertEquals(registeredBefore, server.queryNames(new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",*"), null), + "and nothing under the type for this pool"); + } + + @Test + void aNullObjectNamePropertyValueBehavesAsIfAbsent() throws Exception { + // The property grammar cannot produce a value-less property, but the + // public programmatic API can: InterceptorProperty carries a null + // value, and a definition handed through it reaches the chain with + // one. A null value names nothing to override - the same reading + // every sibling property of this family gives a null value - so the + // default name applies and everything else must hold as if the + // property were absent. Before the guard the null reached the + // ObjectName constructor as an NPE that registerJmx does not catch, + // escaping through reset() to fail every borrow of the pool and + // discard the physical connection each attempt had just opened. + String poolName = "JmxNullOname-" + System.nanoTime(); + DataSource ds = reportingPoolNamed(poolName); + for (PoolProperties.InterceptorDefinition definition + : ds.getPoolProperties().getJdbcInterceptorsAsArray()) { + if (definition.getClassName().endsWith("SlowQueryReportJmx")) { + definition.addProperty("objectName", null); + } + } + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",name=" + poolName); + try (@SuppressWarnings("unused") Connection first = ds.getConnection()) { + Assertions.assertTrue(server.isRegistered(oname), + "a value-less objectName property must cost no more than any other" + + " unformable override - the borrow itself must be served, and" + + " the registration fall back to the default name of the pool"); + } + ds.close(); + Assertions.assertFalse(server.isRegistered(oname), + "and closing the pool releases the name again"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "with no registry entry left behind"); + } + + @Test + void aReleaseThatCannotDeriveTheNameKeepsTheRegistrationReleasable() throws Exception { + // The release unregisters the ObjectName that registrationObjectName() + // derives for the registry value, and that derivation is the one step + // of the release that can throw - for a registration a subclass put + // into the open registry, whose own properties no longer form the name + // the MBeanServer holds. A release that cannot work out which name to + // take down must leave the registry slot held: a registry that has + // forgotten the registration while the MBeanServer still publishes it + // is the one state no later release can repair. As in the late-publish + // test, the test drives the registry state directly and runs the + // failing release and the later repairing one apart. + String poolName = "JmxBadRelease-" + System.nanoTime(); + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + + // The registry value and its published MBean, as registration would + // have left them: derived name, live registration, live bookkeeping. + SlowQueryReportJmx owner = new SlowQueryReportJmx(); + owner.poolName = poolName; + ObjectName oname = owner.getObjectName(SlowQueryReportJmx.class, poolName); + server.registerMBean(owner, oname); + SlowQueryReportJmx.mbeans.put(poolName, owner); + owner.getNotifier(); + SlowQueryReportJmx.registrationFailuresLogged.put(poolName, oname); + + // Reconfigure the owner so the release's recomputation of its name + // fails, as it does whenever the owner's own properties no longer form + // the name the MBeanServer holds. + Map<String, InterceptorProperty> malformed = new HashMap<>(); + malformed.put(SlowQueryReportJmx.objectNameAttribute, new InterceptorProperty( + SlowQueryReportJmx.objectNameAttribute, "value-without-a-domain")); + owner.setProperties(malformed); + + SlowQueryReportJmx releaser = new SlowQueryReportJmx(); + releaser.poolName = poolName; + releaser.deregisterJmx(); + + // The release could not name the MBean to take down - the MBean is + // still published, so the registry must still be holding the value + // that names it. Dropping the slot on the way to the failed + // derivation strands the MBean behind a registry that no longer knows + // about it: no later release can find the registration to unregister, + // and every later pool under the name is refused its own. + Assertions.assertSame(owner, SlowQueryReportJmx.mbeans.get(poolName), + "a release whose derivation fails must leave the registry slot held"); + Assertions.assertTrue(server.isRegistered(oname), + "the premise: the MBean is still published while the release failed"); + + // The broadcaster and the throttle marker belong to the name as much + // as the MBean does (see deregisterJmx's javadoc): the release drops + // them even when its own unregistration step failed. + Assertions.assertNull(SlowQueryReportJmx.notifiers.get(poolName), + "the name's broadcaster must be released with the name"); + Assertions.assertNull(SlowQueryReportJmx.registrationFailuresLogged.get(poolName), + "and the registration-failure throttle with it"); + + // The name must stay releasable: once the derivation works again - + // here, the owner's properties no longer carry the malformed override + // - a later release of the name must still find the slot and take the + // MBean down with it. + owner.setProperties(new HashMap<>()); + releaser.deregisterJmx(); + Assertions.assertFalse(server.isRegistered(oname), + "a later release with a derivable name must still be able to unregister the MBean"); + Assertions.assertNull(SlowQueryReportJmx.mbeans.get(poolName), + "and take the registry slot with it"); + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportNamelessPool.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportNamelessPool.java index 3ce7c4a6ec..933eb49569 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportNamelessPool.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportNamelessPool.java @@ -16,14 +16,24 @@ */ package org.apache.tomcat.jdbc.pool.interceptor; +import java.io.Serializable; +import java.lang.management.ManagementFactory; +import java.sql.Connection; import java.sql.SQLException; +import java.sql.Statement; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; 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.Notification; +import javax.management.NotificationListener; +import javax.management.ObjectName; + import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -46,11 +56,21 @@ import org.apache.tomcat.jdbc.pool.DataSource; * {@code getPoolStats(null)} threw rather than answering the same * {@code null} it answers for any other name that holds nothing. * <p> - * The tests pin the two halves of the corrected contract: a lookup of a name - * that can never be keyed answers {@code null} like any other absent name - - * which is also the answer the borrow-time reference in {@code reset} resolves, - * since it comes through this lookup - and a pool that starts without a name is - * refused by a failure that says which setting is missing. + * The tests pin the halves of the corrected contract: a lookup of a name that + * can never be keyed answers {@code null} like any other absent name - which is + * also the answer the borrow-time reference in {@code reset} resolves, since it + * comes through this lookup - and a pool that starts without a name is refused + * by a failure that says which setting is missing. + * </p> + * <p> + * The refusal only covers a definition the pool was started with, because a + * reporting definition is refused at its start. The interceptor string is + * mutable while the pool runs, so a definition the pool gains after its start + * never meets that check, and the nameless pool and its unformable name arrive + * at the borrow-time registration side by side. A borrow is not the place to + * learn that nothing can be registered under no name: what the missing key can + * cost there is the monitoring, never the query - so the last tests pin that a + * late definition costs a nameless pool neither its borrows nor its statements. * </p> */ class TestSlowQueryReportNamelessPool extends JdbcPoolBaseTest { @@ -110,6 +130,93 @@ class TestSlowQueryReportNamelessPool extends JdbcPoolBaseTest { } } + @Test + void aJmxReportingDefinitionAddedToANamelessPoolAfterTheStartStillServesItsBorrows() + throws Exception { + // A definition the pool gains after its start never runs poolStarted, + // so the refusal above cannot reach it: this pairing exists, and its + // borrow-time registration is where the missing key turns up. Nothing + // can be registered under no name, and a borrow is not the place where + // that is decided - the cost of the missing key is the monitoring. + DataSource ds = namelessPoolWith(""); + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + try { + ds.createPool(); + ds.setJdbcInterceptors(SlowQueryReportJmx.class.getName() + + "(notifyPool=false,threshold=0)"); + Assertions.assertDoesNotThrow(() -> { + try (@SuppressWarnings("unused") Connection con = ds.getConnection()) { + // the borrow itself is the measured step + } + }, "a reporting definition the pool took on after its start must not cost a" + + " nameless pool its borrows: the NPE out of the name-keyed registry" + + " escapes the interceptor reset and fails every connection the pool" + + " hands out, discarding the physical connection each attempt opened"); + + try (@SuppressWarnings("unused") Connection con = ds.getConnection()) { + // borrowed - now make sure the quietly declined registration + // declined rather than published under a made-up name + Assertions.assertTrue(server.queryNames(new ObjectName("tomcat.jdbc:type=" + + SlowQueryReportJmx.class.getName() + ",*"), null).stream() + .noneMatch(name -> "null".equals(name.getKeyProperty("name"))), + "a pool with no name has no name to publish under: declining the" + + " registration must not become a MBean under the literal" + + " text of a null reference"); + } + } finally { + ds.close(); + } + } + + @Test + void aSlowQueryOfANamelessPoolWithALateReportingDefinitionIsServedAndStillNotified() + throws Exception { + // Same pairing, met one step later: the slow-query notification names + // its source by the registration the pool's name holds, and a name + // that does not exist is not a key the registry can be asked about. + // The proxy keeps a report from disturbing the application, so the + // statement does come back - what the missing key silently takes is + // the notification itself, and a monitoring channel that is configured + // and quietly sends nothing is the outcome the class refuses everywhere + // else for a name it cannot form. + DataSource ds = namelessPoolWith(""); + try { + ds.createPool(); + ds.setJdbcInterceptors(SlowQueryReportJmx.class.getName() + + "(notifyPool=false,threshold=0)"); + try (Connection admin = adminConnection(); + Statement setup = admin.createStatement()) { + setup.execute("CREATE ALIAS IF NOT EXISTS SLOW_SLEEP FOR " + + "\"java.lang.Thread.sleep(long)\""); + } + String slow = "SELECT 1 FROM SYSTEM_RANGE(1,1) WHERE SLOW_SLEEP(25) IS NULL"; + try (Connection con = ds.getConnection()) { + List<Notification> received = new CopyOnWriteArrayList<>(); + NotificationListener listener = (notification, handback) -> received.add(notification); + interceptorOf(con, SlowQueryReportJmx.class).addNotificationListener(listener, null, null); + Assertions.assertDoesNotThrow(() -> { + try (Statement st = con.createStatement()) { + st.execute(slow); + } + }, "a slow query of a pool that carries no name must still come back as a" + + " executed statement"); + Assertions.assertEquals(1, received.size(), + "the slow query must still be reported to the listeners the" + + " interceptor has: the source of a notification may lose the" + + " registered name it would rather carry, but a source it" + + " cannot look up must not cost the notification - the" + + " failure rides through as a swallowed warning, so the" + + " monitoring of a nameless pool is simply gone"); + Assertions.assertInstanceOf(Serializable.class, received.get(0).getSource(), + "the source of a notification is marshalled to listeners outside this" + + " JVM, so even the fallback for a pool that carries no name" + + " must be something that survives that delivery"); + } + } finally { + ds.close(); + } + } + @Test void aPoolThatNeverSetsTheNamePropertyIsNotANamelessPool() throws Exception { try (StartLogCapture logs = new StartLogCapture()) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java index da789cb1d9..ffe85fa19b 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxRegistration.java @@ -28,6 +28,7 @@ import java.util.concurrent.atomic.AtomicReference; import javax.management.InstanceAlreadyExistsException; import javax.management.MBeanRegistration; +import javax.management.MBeanRegistrationException; import javax.management.MBeanServer; import javax.management.MalformedObjectNameException; import javax.management.Notification; @@ -60,7 +61,10 @@ import org.apache.tomcat.jdbc.pool.interceptor.StatementCache; * perfectly well.</p> * <p> * The contract the first tests pin is that the returned name is this call's own - * publish, or nothing at all. The last five drive that through a real pool: a + * publish, or nothing at all. The unregistration tests pin the mirror: what the + * helper reports when it takes a name down, since every holder drops its handle + * the moment it calls, and a refusal it cannot see leaves the MBean registered + * for the life of the JVM. The pool-level tests drive that through a real pool: a * {@code StatementCache} whose name is taken must leave the MBean that owns the * name registered when its connection is discarded, and it must publish itself * at the next borrow once the obstacle is gone; the {@code DataSource} one @@ -103,6 +107,47 @@ class TestJmxRegistration extends JdbcPoolBaseTest { } } + /* + * An MBean that refuses to be unregistered until told otherwise: the + * MBeanRegistrationException a throwing preDeregister() turns into a + * refused unregisterMBean() call, which is how a deregistration fails + * besides the name simply being gone. + */ + public interface StubbornMBean { + String getMarker(); + } + + public static class Stubborn implements StubbornMBean, MBeanRegistration { + private volatile boolean allowRelease; + + @Override + public String getMarker() { + return "stubborn"; + } + + @Override + public ObjectName preRegister(MBeanServer server, ObjectName name) { + return name; + } + + @Override + public void postRegister(Boolean registrationDone) { + // nothing to do once published + } + + @Override + public void preDeregister() throws Exception { + if (!allowRelease) { + throw new MBeanRegistrationException(null, "this MBean gives up its name only once"); + } + } + + @Override + public void postDeregister() { + // nothing to undo + } + } + /* * A modeler-style wrapper of the kind a container registers in place of a * DataSource. The DataSource is not itself a JMX compliant MBean, so the @@ -357,6 +402,62 @@ class TestJmxRegistration extends JdbcPoolBaseTest { "a malformed name must publish nothing"); } + @Test + void aSuccessfulUnregistrationComesBackAsReleased() throws Exception { + MBeanServer server = server(); + ObjectName name = uniqueName("released"); + server.registerMBean(new Probe("released"), name); + + Assertions.assertTrue(JmxUtil.unregisterJmx(name), + "an unregistration the server carried out must be reported as done: every" + + " caller of this helper drops its handle the moment it calls, so a" + + " release that silently did not happen is undetectable from the" + + " calling side"); + Assertions.assertFalse(server.isRegistered(name)); + } + + @Test + void aRefusedUnregistrationComesBackAsNotReleased() throws Exception { + MBeanServer server = server(); + ObjectName name = uniqueName("stubborn"); + Stubborn mbean = new Stubborn(); + server.registerMBean(mbean, name); + try { + Assertions.assertFalse(JmxUtil.unregisterJmx(name), + "an unregistration the MBean refused - a preDeregister() that throws, as a" + + " modeler wrapper's failing postDeregister() also produces - must" + + " come back as not done: the MBean is still registered under a" + + " name its caller is about to forget, which is exactly the" + + " failure the caller cannot see any other way, and it leaves a" + + " live management surface for a dead component holding its" + + " credentials and class loader until the JVM exits"); + Assertions.assertTrue(server.isRegistered(name), + "a refused unregistration must have left the MBean published - that is" + + " the state the caller has to be warned about"); + } finally { + mbean.allowRelease = true; + server.unregisterMBean(name); + } + } + + @Test + void anUnregistrationOfNothingCountsAsDone() throws Exception { + MBeanServer server = server(); + ObjectName gone = uniqueName("already-gone"); + server.registerMBean(new Probe("gone"), gone); + server.unregisterMBean(gone); + + // the MBean is already down - the desired end state, reached without + // this call - and no caller has anything left to do differently + Assertions.assertTrue(JmxUtil.unregisterJmx(gone), + "unregistering a name that is already gone must be reported as done, not as a" + + " failure: an InstanceNotFoundException means the world is already" + + " in the state the caller asked for, and grading it as a refusal" + + " would hide the refusals that are real"); + Assertions.assertTrue(JmxUtil.unregisterJmx(null), + "releasing nothing is the same end state and the same answer"); + } + @Test void aCacheThatWasRefusedItsNameDoesNotUnregisterTheMBeanThatOwnsIt() throws Exception { MBeanServer server = server(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
