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 b8627e47291f98eb25e9b5f1bc7e938f03c56f1e Author: opencode <[email protected]> AuthorDate: Thu Nov 5 21:48:37 2026 +0100 Do not register connection MBeans on the borrow path JMX is a best-effort management channel. A connection registers its MBean when its physical session is established, and the pool publishes the connections it already holds when its own MBean is registered. The borrow path nevertheless called createMBean() again as a retry, and a failure there failed the borrow through abandonSetup(). Drop that call and its failure handling: a registration neither path made is left for a later reconnect, and a borrow no longer does extra JMX work or fails because the management server is unavailable. Remove the obsolete setup-failure test and keep the test for skipping a registration when the pool has no JMX object. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 50 ++-------- .../apache/tomcat/jdbc/pool/PooledConnection.java | 8 +- .../jdbc/pool/TestSetupConnectionMBeanFailure.java | 108 +-------------------- 3 files changed, 19 insertions(+), 147 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 c54d5c822a..36b2ff2b05 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 @@ -447,44 +447,13 @@ public class ConnectionPool { // Fetch previously cached interceptor proxy - one per connection JdbcInterceptor handler = con.getHandler(); if (handler == null) { - // The registration reads the pool's own JMX object itself and does - // nothing once close() has given it up; it is not gated on jmxPool - // here because checking the field and then calling into the - // connection would let close() take it away between the two, and - // 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 { - // Under the connection's own lock, as every connection MBean - // registration is (see registerConnectionMBeans). The check - // and the publish inside run as one synchronized step, and - // the release of the same connection unregisters under that - // same monitor - the only lock every configuration shares, - // the connection lock being a deliberate no-op unless the - // sweeper or useLock is on - so a registration can never slip - // in beside a release that had already found no name to take - // back and leave an MBean of a dead connection, and the class - // loader it pins, registered for the life of the JVM. - con.lock(); - try { - con.createMBean(); - } finally { - con.unlock(); - } - } catch (Throwable x) { - ExceptionUtils.handleThrowable(x); - SQLException sx = new SQLException(MessageFactory.get("connectionPool.unableToRegisterMBean")); - sx.initCause(x); - // Nothing has been done to the connection, it can go back to the pool - abandonSetup(con, sx, false); - throw sx; - } + // A connection registers its MBean when its physical session is + // established, and the pool sweeps the connections it already + // holds when its own MBean is published. JMX is best effort, so a + // registration that neither path made is not retried on the borrow: + // a borrow has no business failing, or doing extra work, because a + // management channel is unavailable. + // // Build the proxy handler handler = new ProxyConnection(this, con, getPoolProperties().isUseEquals()); // Set up the interceptor chain @@ -2697,8 +2666,9 @@ public class ConnectionPool { // 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. + // may or may not be seen. JMX is best effort, so a connection this pass + // misses stays unregistered until it reconnects; it does not fail or + // delay a borrow. for (PooledConnection con : pool) { try { // Under the connection's own lock, as every operation acting 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 4cb92d0e2e..0f7f64c3c3 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 @@ -1650,10 +1650,10 @@ 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. 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. The + * live connection from the connect 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. The * release of a connection settles its unregistration and its released flag under this same monitor (see * {@link #release()}), so whichever of the two passes first settles the outcome: a registration after a release * sees the released flag and publishes nothing, and a release after a registration finds the name and takes it diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSetupConnectionMBeanFailure.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSetupConnectionMBeanFailure.java index 2875f808bb..4b51715beb 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSetupConnectionMBeanFailure.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSetupConnectionMBeanFailure.java @@ -16,80 +16,20 @@ */ package org.apache.tomcat.jdbc.pool; -import java.sql.Connection; -import java.sql.SQLException; -import java.util.concurrent.atomic.AtomicBoolean; - import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.JdbcPoolBaseTest; /** - * A borrow reads the pool's JMX object when it registers the per-connection - * MBean, but {@code close()} gives that object up without holding any pool - * lock, so a borrow racing the close can find it gone between a null check - * and its use. Read unsafely, that produced a NullPointerException from - * {@code PooledConnection.createMBean()} inside - * {@code ConnectionPool.setupConnection()}. The interesting part is where - * that lands: the connection is already in the busy queue when setup runs, - * and a failure that skips {@code abandonSetup()} strands it there - the - * physical session and a maxActive slot are lost until the pool restarts. - * <p> - * The window between check and use is a few instructions wide and cannot be - * reproduced deterministically. What these tests pin down instead is the - * contract the fix makes total: the registration is skipped when the pool has - * no JMX object to register under - rather than dereferencing it blindly - - * and it runs unconditionally from the setup path, so that whatever it fails - * with, the borrowed connection goes back to the pool. That the remaining - * JMX notifications read the volatile field into a local before using it is - * carried by review, not by a test. + * A connection registers its MBean when its physical session is established, + * not when it is borrowed: JMX is a best-effort management channel, so the + * borrow path neither retries a registration nor fails because one is + * unavailable. The registration is skipped when the pool has no JMX object to + * register under, rather than dereferencing it blindly. */ class TestSetupConnectionMBeanFailure extends JdbcPoolBaseTest { - /* - * A connection whose JMX registration fails while the flag is set, and - * performs the real registration once the test lets it through. - */ - static class FailingRegistrationConnection extends PooledConnection { - - private final AtomicBoolean failRegistration; - - FailingRegistrationConnection(PoolConfiguration prop, ConnectionPool parent, - AtomicBoolean failRegistration) { - super(prop, parent); - this.failRegistration = failRegistration; - } - - @Override - public void createMBean() { - if (failRegistration.get()) { - throw new IllegalStateException("boom from createMBean()"); - } - super.createMBean(); - } - } - - /* - * A pool handing out the connections above. The base create() is still - * invoked for its pool-size bookkeeping; the pooled connection object it - * builds is never connected and simply discarded. - */ - static class FailingRegistrationPool extends ConnectionPool { - - final AtomicBoolean failRegistration = new AtomicBoolean(true); - - FailingRegistrationPool(PoolConfiguration prop) throws SQLException { - super(prop); - } - - @Override - protected PooledConnection create(boolean incrementCounter) { - super.create(incrementCounter); - return new FailingRegistrationConnection(getPoolProperties(), this, failRegistration); - } - } - /* * The pool is built directly rather than through a DataSource: the setup * path and the failure seam are package-protected, and JMX stays disabled @@ -131,42 +71,4 @@ class TestSetupConnectionMBeanFailure extends JdbcPoolBaseTest { "an unconnected pooled connection may not own a session"); } - @Test - void failingMBeanRegistrationReturnsTheConnectionInsteadOfStrandingIt() throws Exception { - FailingRegistrationPool pool = new FailingRegistrationPool(newPoolProperties()); - try { - // The setup path must reach the registration even with JMX - // disabled, so this is the failure the borrow has to survive; a - // gate on the pool's JMX field in front of the call would let the - // check-and-use window back in. - SQLException x = Assertions.assertThrows(SQLException.class, pool::getConnection, - "a failing MBean registration must fail the borrow through the pool's own exception"); - Assertions.assertTrue(x.getCause() instanceof IllegalStateException, - "the borrow must report the registration failure, was: " + x.getCause()); - - // The stranding defect: the connection was already in busy when - // setup failed, so only the abandonSetup() path can hand it back. - Assertions.assertEquals(0, pool.getActive(), - "a setup that must not strand the connection may not leave it in busy"); - Assertions.assertEquals(1, pool.getIdle(), - "the connection must go back to the idle queue, not be discarded"); - Assertions.assertEquals(1, pool.getBorrowedCount(), "the failed borrow is counted"); - Assertions.assertEquals(1, pool.getReturnedCount(), - "the hand-back after the failed setup is counted as a return"); - Assertions.assertEquals(1, poolSessions(), - "the physical connection must still be owned by the pool"); - - // The slot the borrow needed is intact: with maxActive=1, a - // stranded connection would exhaust the pool right here. - pool.failRegistration.set(false); - Connection borrowed = pool.getConnection(); - Assertions.assertNotNull(borrowed, "the pool must still be able to hand out its connection"); - borrowed.close(); - Assertions.assertEquals(0, pool.getActive()); - Assertions.assertEquals(1, pool.getIdle()); - } finally { - pool.close(true); - } - awaitSessions(0); - } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
