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 5b227c9c036660ebc1556b2e1a403329e2afc8a0 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 10:09:03 2026 +0100 Report a disconnected pooled connection instead of an NPE through its MBean PooledConnection implements PooledConnectionMBean, and its attribute methods delegated straight to the connection field. That field is transiently null while a reconnect is in flight - the reconnect triggered by expired max age or alternate credentials runs disconnect(false) before connect() - and it stays null after a failed reconnect until the connection is released, while the per-connection MBean can still be registered during that window. Polling it then escaped a NullPointerException: JMX surfaced it as an UnknownRuntimeException instead of the declared SQLException, and isClosed() and clearWarnings(), which declare no exception, threw outright. Internally the same NPE was worse than cosmetic: ConnectionPool.shouldClose() only catches SQLException around isClosed(), so it escaped there after the connection had already been removed from the busy count and before release(), skipping the release. The delegation now reports the state instead of failing on it. The six attribute getters that declare SQLException throw SQLException("Connection has been disconnected.") when there is no physical connection, isClosed() reports true, and clearWarnings() is a no-op as there is nothing to clear warnings on; a driver failure it swallows (the method declares no exception) now logs a message that no longer promises a close that does not happen. Each method snapshots the volatile field once, so a concurrent disconnect() cannot reintroduce the NPE between the check and the use. Pool-visible behavior is unchanged: disconnect() marks the connection discarded before nulling the field, and shouldClose() short-circuits on that flag, so the pool already discarded such connections; this is the JMX boundary and the defensive seal of the shouldClose shape. Tests: TestMBeanDelegationWhenDisconnected pins the never-connected stand-in (isClosed true, clearWarnings no-throw, shouldClose true via VALIDATE_RETURN, SQLException from each getter - two of these fail with NPEs before the fix) and a live path proving the methods still reach the real physical connection after borrow and return. --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 54 ++++++++-- .../pool/TestMBeanDelegationWhenDisconnected.java | 118 +++++++++++++++++++++ 2 files changed, 163 insertions(+), 9 deletions(-) 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 5f98000966..15d2ad129f 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 @@ -986,47 +986,83 @@ public class PooledConnection implements PooledConnectionMBean { return oname; } + /** + * Returns the physical connection behind this pooled connection. The + * connection field is transiently null while a reconnect is in flight + * (and after a failed one), during which the per-connection MBean stays + * registered, so the JMX-exposed delegation methods must report that + * state rather than fail with a {@link NullPointerException}. + * @return the live physical connection + * @throws SQLException if this pooled connection currently has no + * physical connection + */ + private java.sql.Connection requireConnection() throws SQLException { + java.sql.Connection c = connection; + if (c == null) { + throw new SQLException("Connection has been disconnected."); + } + return c; + } + + /** + * Clears the warnings on the physical connection. A no-op while this + * pooled connection has no physical connection, as there is nothing to + * clear warnings on. A driver failure to clear the warnings is logged + * and swallowed rather than propagated; the operation is JMX-exposed and + * therefore declares no exception. + */ @Override public void clearWarnings() { + java.sql.Connection c = connection; + if (c == null) { + return; + } try { - connection.clearWarnings(); + c.clearWarnings(); } catch (SQLException e) { - log.warn("Unable to clear Warnings, connection will be closed.", e); + log.warn("Unable to clear warnings on the connection.", e); } } + /** + * Returns true if this pooled connection is closed: either it currently + * has no physical connection or that connection is closed. + * @return true if disconnected or the physical connection is closed + * @throws SQLException if the physical connection reports a failure + */ @Override public boolean isClosed() throws SQLException { - return connection.isClosed(); + java.sql.Connection c = connection; + return c == null || c.isClosed(); } @Override public boolean getAutoCommit() throws SQLException { - return connection.getAutoCommit(); + return requireConnection().getAutoCommit(); } @Override public String getCatalog() throws SQLException { - return connection.getCatalog(); + return requireConnection().getCatalog(); } @Override public int getHoldability() throws SQLException { - return connection.getHoldability(); + return requireConnection().getHoldability(); } @Override public boolean isReadOnly() throws SQLException { - return connection.isReadOnly(); + return requireConnection().isReadOnly(); } @Override public String getSchema() throws SQLException { - return connection.getSchema(); + return requireConnection().getSchema(); } @Override public int getTransactionIsolation() throws SQLException { - return connection.getTransactionIsolation(); + return requireConnection().getTransactionIsolation(); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestMBeanDelegationWhenDisconnected.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestMBeanDelegationWhenDisconnected.java new file mode 100644 index 0000000000..7c4b9b75b4 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestMBeanDelegationWhenDisconnected.java @@ -0,0 +1,118 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool; + +import java.sql.Connection; +import java.sql.SQLException; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The physical connection field of a {@link PooledConnection} is transiently + * {@code null} while a reconnect is in flight - the reconnect triggered by + * expired max age or alternate credentials runs {@code disconnect(false)} + * before {@code connect()} - and a failed reconnect leaves it {@code null} + * until the connection is released, while the per-connection MBean + * ({@link PooledConnectionMBean}) can still be registered. The delegation + * methods exposed through that MBean must report the state - a + * {@link SQLException} or a meaningful answer - instead of escaping a raw + * {@link NullPointerException} as a JMX {@code UnknownRuntimeException}. + * JMX is disabled in these tests; the methods are exercised directly on a + * pooled connection object that never had a physical session, which is the + * deterministic stand-in for the state such a registered MBean is polled in. + */ +class TestMBeanDelegationWhenDisconnected extends JdbcPoolBaseTest { + + /* + * A pooled connection object that never had a physical session: the + * direct, deterministic stand-in for the state the registered MBean can + * be polled in during the reconnect window. + */ + private static PooledConnection disconnectedPooledConnection(ConnectionPool pool) { + return new PooledConnection(pool.getPoolProperties(), pool); + } + + @Test + void stateDelegationReportsClosedWithoutANullPointer() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + PooledConnection disconnected = disconnectedPooledConnection(pool); + + // Pre-fix: NullPointerException. + Assertions.assertTrue(disconnected.isClosed(), + "a pooled connection without a physical connection is closed"); + // Declares no exception, so it must not throw at all. + Assertions.assertDoesNotThrow(disconnected::clearWarnings); + + // The pool's own return/release decision (ConnectionPool's + // release(path) uses this same VALIDATE_RETURN call): pre-fix the + // NPE escaped shouldClose()'s catch (SQLException) after the + // connection had already been removed from the busy count, + // without release() ever running. + Assertions.assertTrue( + pool.shouldClose(disconnected, PooledConnection.VALIDATE_RETURN), + "a pooled connection without a physical connection must be " + + "closed rather than returned to the pool"); + } + + @Test + void attributeDelegationFailsWithSQLExceptionNotANullPointer() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + PooledConnection disconnected = disconnectedPooledConnection(pool); + + // Pre-fix: NullPointerException, which JMX surfaces as an + // UnknownRuntimeException instead of the declared SQLException. + Assertions.assertThrows(SQLException.class, disconnected::getAutoCommit); + Assertions.assertThrows(SQLException.class, disconnected::getCatalog); + Assertions.assertThrows(SQLException.class, disconnected::getHoldability); + Assertions.assertThrows(SQLException.class, disconnected::isReadOnly); + Assertions.assertThrows(SQLException.class, disconnected::getSchema); + Assertions.assertThrows(SQLException.class, + disconnected::getTransactionIsolation); + } + + @Test + void delegationStillReachesTheLivePhysicalConnection() throws Exception { + DataSource ds = newDataSource(); + Connection handle = ds.getConnection(); + + PooledConnection pooled = handle.unwrap(PooledConnection.class); + Assertions.assertFalse(pooled.isClosed()); + Assertions.assertTrue(pooled.getAutoCommit()); + Assertions.assertNotNull(pooled.getCatalog()); + Assertions.assertNotNull(pooled.getSchema()); + Assertions.assertFalse(pooled.isReadOnly()); + Assertions.assertTrue(pooled.getTransactionIsolation() + >= Connection.TRANSACTION_NONE); + Assertions.assertTrue(pooled.getHoldability() > 0); + pooled.clearWarnings(); // must not throw + + handle.close(); // back to the pool: still physically connected + Assertions.assertFalse(pooled.isClosed(), + "a returned connection keeps its physical session"); + Assertions.assertTrue(pooled.getAutoCommit(), + "the delegation methods still reach the idle session"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
