This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 5de5f2539e9030afb082c44edd9983c8d6da8d31 Author: opencode <[email protected]> AuthorDate: Thu Oct 29 21:27:04 2026 +0100 Publish a JMX MBean for every physical JDBC pool connection Bugzilla 63705: a per-connection MBean was registered only at a connection's first borrow, so the connections the pool pre-created for its initial size, and any idle connection an application never borrowed, were counted in the pool's Size and NumIdle attributes while staying invisible to management. A connection now registers itself as part of a successful physical connect - covering pool startup, growth and reconnect, where a reconnect keeps the registration it already holds - and DataSource.registerJmx() publishes the connections a running pool already holds once the pool's own MBean has a name to publish them under, which is the order a container registers them in; a queue placeholder that has no physical connection yet is skipped and registers when it gets one. The registration check and publish became one synchronized step, with a released connection no longer registrable, because the registration now reaches a connection from the connect path, the first-borrow retry path and the publish sweep. The unregistration on release is unchanged. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 53 ++++++++++++++++++++++ .../org/apache/tomcat/jdbc/pool/DataSource.java | 8 ++++ .../apache/tomcat/jdbc/pool/PooledConnection.java | 25 ++++++++-- .../tomcat/jdbc/pool/jmx/TestJmxMonitor.java | 45 ++++++++++++++++-- .../jdbc/pool/jmx/TestJmxNotificationSource.java | 12 ++++- .../pool/jmx/TestJmxNotificationTypeContract.java | 12 ++++- webapps/docs/changelog.xml | 16 +++++++ 7 files changed, 160 insertions(+), 11 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index 1432c0808e..8b5984293b 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 @@ -356,6 +356,12 @@ public class ConnectionPool { // the resulting failure must not strand the connection - it is // already in busy, so a throw that skips abandonSetup() costs a // physical connection and a maxActive slot until the pool restarts. + // A connection that connected while the pool's MBean was already + // published registered itself as part of connect() and is a no-op + // here; this call is the retry for one that could not - because the + // pool's MBean was published only after it connected, or because the + // server refused the name, and a refused publish must be retried + // once the obstacle is gone rather than count as a registration. try { con.createMBean(); } catch (Exception x) { @@ -1790,6 +1796,53 @@ public class ConnectionPool { } } + /** + * Publishes the MBeans of all connections this pool currently holds, below the pool's own MBean. A connection + * registers itself when its physical connection is established, but that is only possible once the pool's own + * MBean has been published - and a container publishes it only after the pool, already connected to its initial + * size, has been created. Without this sweep the connections made before that publication, and in particular the + * ones the pool pre-created for its initial size, would be counted in the pool's size while staying invisible to + * JMX for as long as they are never borrowed. + */ + public void registerConnectionMBeans() { + // The connections publish themselves below the pool's own name, so + // until that name is registered there is no base to publish them under. + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; + if (jmx == null || jmx.getObjectName() == null) { + return; + } + registerConnectionMBeans(idle); + registerConnectionMBeans(busy); + } + + private void registerConnectionMBeans(BlockingQueue<PooledConnection> pool) { + // The fair queue iterates a snapshot taken under its lock, the default + // queue a view that tolerates concurrent modification: either way, a + // connection that is concurrently taken out of or added to the queue + // may or may not be seen, and both answers are correct - the one this + // pass misses registers itself on the borrow that hands it out. + for (PooledConnection con : pool) { + try { + // Under the connection's own lock, as every operation acting + // on a pooled connection is: the registration must not slip in + // beside the release of the same connection, whose + // unregistration would then find no name to take back and this + // one would leave a MBean of a dead connection behind forever. + con.lock(); + // A connection that is still without a physical connection is a + // placeholder for a slot, not a session to monitor, and it + // registers through the connect path once it gets its + // connection - publishing one now would name a session that + // does not exist. + if (con.isInitialized()) { + con.createMBean(); + } + } finally { + con.unlock(); + } + } + } + /** * The total number of connections borrowed from this pool. * diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java index 8868517198..750d666573 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java @@ -188,6 +188,14 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, // refused it, so a name a foreign MBean owns is never released as // if it were this pool's registration this.oname = JmxUtil.registerJmx(oname, null, jmx); + if (this.oname != null) { + // A connection publishes itself below this name when its physical + // connection is established; the connections the pool was already + // holding could not do that while this name did not exist yet, so + // they are published now. A publish the server refused registers + // nothing of its own and has no connections to publish either. + p.registerConnectionMBeans(); + } } /** diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java index c366f3fc54..1977f05aa1 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java @@ -275,6 +275,21 @@ public class PooledConnection implements PooledConnectionMBean { } this.discarded = false; this.lastConnected = System.currentTimeMillis(); + + // The physical session is up: this is the point at which the + // connection can publish itself below the pool's MBean. Tying that to + // the first borrow instead left a connection the pool pre-created for + // its initial size, or one that is simply never borrowed, counted in + // the pool's size yet invisible to JMX for its whole life. This also + // covers every reconnect; a connection that is already registered + // keeps its registration across a reconnect, and createMBean() leaves + // such a call alone by its own guards. With JMX disabled the pool has + // no JMX object to register under, and the borrow path still calls the + // registration unconditionally, so it is skipped here rather than + // called only to do nothing. + if (parent.getJmxPool() != null) { + createMBean(); + } } /** @@ -1096,10 +1111,14 @@ public class PooledConnection implements PooledConnectionMBean { /** * Creates and registers an MBean for this pooled connection with JMX. The pool's own MBean is the registration * base; once close() has given it up - which a borrow can race - there is no base to register under, and the - * registration is skipped. + * registration is skipped. It is skipped as well for a connection that is already registered - the pool reaches a + * live connection from the connect path, from the first-borrow path and from the sweep it runs when its own MBean + * is published - and for a released one, which would leave a registration of a dead connection behind with nothing + * left to unregister it. The check and the publish are one synchronized step because those paths can meet on the + * same connection and must never publish it twice: only one of the registrations would ever be unregistered. */ - public void createMBean() { - if (oname != null) { + public synchronized void createMBean() { + if (oname != null || released.get()) { return; } org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = parent.getJmxPool(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxMonitor.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxMonitor.java index 877de0b96d..f11f7e960b 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxMonitor.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxMonitor.java @@ -64,8 +64,11 @@ class TestJmxMonitor extends JdbcPoolBaseTest { /** * Publishes an already created pool the way a container does and - * returns the single pool MBean that appeared, or {@code null} if - * nothing was published. + * returns the pool's own MBean that appeared, or {@code null} if + * nothing was published. A container's registration publishes the + * connections the pool was already holding as well, below the pool's + * own name, so the added set can hold several MBeans and the pool's + * is the one of them that is not a connection. */ private ObjectName registerAndFindPoolMBean(MBeanServer server, DataSource ds, ObjectName dsName) throws Exception { @@ -73,7 +76,14 @@ class TestJmxMonitor extends JdbcPoolBaseTest { ds.preRegister(server, dsName); Set<ObjectName> added = new HashSet<>(poolMBeans(server)); added.removeAll(before); - return added.isEmpty() ? null : added.iterator().next(); + for (ObjectName name : added) { + // the connection MBeans repeat the pool's keys and add a + // "connections=PooledConnection[n]" key of their own + if (name.getKeyProperty("connections") == null) { + return name; + } + } + return null; } /** Counts the connection MBeans published below the given pool MBean. */ @@ -151,6 +161,9 @@ class TestJmxMonitor extends JdbcPoolBaseTest { DataSource ds = newDataSource(); ds.getPoolProperties().setJmxEnabled(true); ds.getPoolProperties().setMaxActive(2); + // two connections the pool pre-creates at startup, so the test has + // physical connections it does not have to borrow + ds.getPoolProperties().setInitialSize(2); ds.createPool(); ObjectName dsName = uniqueDataSourceName("connections"); @@ -158,15 +171,39 @@ class TestJmxMonitor extends JdbcPoolBaseTest { ObjectName poolOname = registerAndFindPoolMBean(server, ds, dsName); try { Assertions.assertNotNull(poolOname); + + // Nothing borrowed yet: the pool pre-created both connections at + // startup - before this test's registration gave the pool its MBean + // name, which is what they publish below - and publishing them is + // the container registration's own job. Pre-fix they appeared only + // at a first borrow, so the pool reported its idle size with no + // connection MBeans behind it (Bugzilla 63705). + Assertions.assertEquals(2, attr(server, poolOname, "NumIdle")); + Assertions.assertEquals(2, connectionMBeanCount(server, poolOname), + "every physical connection of an enabled pool must be published" + + " whether or not it has ever been borrowed"); + Connection first = ds.getConnection(); Connection second = ds.getConnection(); Assertions.assertEquals(2, connectionMBeanCount(server, poolOname), - "every physical connection in use must be published below the pool"); + "borrowing a published connection must not publish it a second time"); first.close(); second.close(); Assertions.assertEquals(2, connectionMBeanCount(server, poolOname), "returned connections stay published, they are still pooled"); + + // An expired max age makes the idle sweep reconnect every + // connection: the physical session is replaced, but the pooled + // connection and its published MBean survive. + ds.getPoolProperties().setMaxAge(1); + sleep(10); // now every connection is older than the max age + ds.testIdle(); + Assertions.assertEquals(2, attr(server, poolOname, "NumIdle"), + "the reconnected connections went back to the idle pool"); + Assertions.assertEquals(2, connectionMBeanCount(server, poolOname), + "a reconnect of a published connection keeps it published, with no" + + " second MBean for it"); } finally { ds.close(); // releases the physical connections ds.postDeregister(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationSource.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationSource.java index 3a0f712f10..7e076489a7 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationSource.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationSource.java @@ -97,7 +97,10 @@ class TestJmxNotificationSource extends JdbcPoolBaseTest { /* * Publishes an already created pool the way a container does and returns - * the single pool MBean that appeared. + * the pool's own MBean that appeared. A container's registration also + * publishes the connections the pool was holding, below the pool's own + * name with a "connections=PooledConnection[n]" key of their own, so the + * pool's MBean is picked as the added one that carries no such key. */ private ObjectName registerPoolMBean(MBeanServer server, DataSource ds, String role) throws Exception { @@ -107,7 +110,12 @@ class TestJmxNotificationSource extends JdbcPoolBaseTest { Set<ObjectName> added = new HashSet<>(server.queryNames(new ObjectName(POOL_DOMAIN + ":*"), null)); added.removeAll(before); - return added.isEmpty() ? null : added.iterator().next(); + for (ObjectName name : added) { + if (name.getKeyProperty("connections") == null) { + return name; + } + } + return null; } @Test diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationTypeContract.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationTypeContract.java index 1b3a92d372..14ef0d16cd 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationTypeContract.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/jmx/TestJmxNotificationTypeContract.java @@ -117,7 +117,10 @@ class TestJmxNotificationTypeContract extends JdbcPoolBaseTest { /* * Publishes an already created pool the way a container does and returns - * the single pool MBean that appeared. + * the pool's own MBean that appeared. A container's registration also + * publishes the connections the pool was holding, below the pool's own + * name with a "connections=PooledConnection[n]" key of their own, so the + * pool's MBean is picked as the added one that carries no such key. */ private ObjectName registerPoolMBean(MBeanServer server, DataSource ds, String role) throws Exception { @@ -127,7 +130,12 @@ class TestJmxNotificationTypeContract extends JdbcPoolBaseTest { Set<ObjectName> added = new HashSet<>(server.queryNames(new ObjectName(POOL_DOMAIN + ":*"), null)); added.removeAll(before); - return added.isEmpty() ? null : added.iterator().next(); + for (ObjectName name : added) { + if (name.getKeyProperty("connections") == null) { + return name; + } + } + return null; } @Test diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 4c0a7ff69f..06ae6020e2 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1072,6 +1072,22 @@ contract requires. Pull request <pr>1077</pr> provided by Shayan Davarzani. (remm) </fix> + <fix> + <bug>63705</bug>: Publish a JMX MBean for every physical connection + held by the JDBC pool, not only for the ones the application has + borrowed at least once. The connection MBeans that appear below the + pool's MBean as <code>connections=PooledConnection[n]</code> were + registered when a connection was borrowed for the first time, so the + connections the pool pre-created for its <code>initialSize</code>, and + any idle connection that was never borrowed, were counted in the pool's + <code>Size</code> and <code>NumIdle</code> attributes while remaining + invisible to management. A connection's MBean is now registered as soon + as its physical connection is established, and the connections a + running pool already holds are published when the pool's own MBean is + registered. A connection keeps its registration across a reconnect and + releases it, as before, when it leaves the pool. + (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
