This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 18ecff2c26136f9b36922a95f7b34fea61ec0625 Author: opencode <[email protected]> AuthorDate: Mon Oct 26 17:02:47 2026 +0100 Isolate interceptor disconnected() failures from the physical close PooledConnection.disconnect() invoked the interceptor chain's disconnected() hook inside the same try block that closes the physical connection and swallows every exception at debug level. An interceptor that threw from disconnected() therefore skipped the physical close entirely: the database connection stayed open with no reference left to retry it (the connection field is nulled right after), while the pool accounting reported it as gone. In addition, ConnectionPool's disconnectEvent() walk stopped at the throwing link, so no later interceptor in the chain was ever notified of the disconnect. The hook now runs in its own guarded block that logs a warning and proceeds, and the physical close runs regardless. disconnectEvent() reads each link's successor before invoking the callback (the callback is allowed to modify the link) and continues the walk past a link that throws, naming the offending class in the log. This also makes the finalize() path reliably reachable after a throwing hook; the equivalent hazard for the reset() callback is tracked separately. Adds TestThrowingDisconnectedInterceptor, asserting database-side that all physical sessions are closed after the pool is closed even when every disconnected() call throws, and that interceptors behind a throwing link still receive exactly one disconnected() callback. Both tests fail deterministically on the previous code. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 15 ++- .../apache/tomcat/jdbc/pool/JdbcInterceptor.java | 3 + .../apache/tomcat/jdbc/pool/PooledConnection.java | 10 ++ .../pool/TestThrowingDisconnectedInterceptor.java | 111 +++++++++++++++++++++ 4 files changed, 137 insertions(+), 2 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 ef3b8e1c98..cafadb5444 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 @@ -1465,8 +1465,19 @@ public class ConnectionPool { protected void disconnectEvent(PooledConnection con, boolean finalizing) { JdbcInterceptor handler = con.getHandler(); while (handler!=null) { - handler.disconnected(this, con, finalizing); - handler=handler.getNext(); + //read the next link before the callback, as the callback + //is allowed to modify it + JdbcInterceptor next = handler.getNext(); + try { + handler.disconnected(this, con, finalizing); + } catch (Throwable t) { + //one broken interceptor must not stop the rest of the + //chain from being notified + log.warn("Interceptor [" + handler.getClass().getName() + + "] threw an exception from disconnected(); " + + "continuing with the rest of the chain.", t); + } + handler = next; } } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/JdbcInterceptor.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/JdbcInterceptor.java index 5c7e27bb47..a2b2bc4d18 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/JdbcInterceptor.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/JdbcInterceptor.java @@ -167,6 +167,9 @@ public abstract class JdbcInterceptor implements InvocationHandler { * Called when {@link java.sql.Connection#close()} is called on the underlying connection. * This is to notify the interceptors, that the physical connection has been released. * Implementation of this method should be thought through with care, as no actions should trigger an exception. + * An interceptor that nevertheless throws is isolated: the exception is logged and + * ignored, the remaining interceptors in the chain are still notified, and the + * physical connection is closed as it would have been. * @param parent - the connection pool that this connection belongs to * @param con - the pooled connection that holds this connection * @param finalizing - if this connection is finalizing. True means that the pooled connection will not reconnect the underlying connection 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 2bc0b0d848..f188b76a3a 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 @@ -428,6 +428,16 @@ public class PooledConnection implements PooledConnectionMBean { if (connection != null) { try { parent.disconnectEvent(this, finalize); + } catch (Throwable x) { + // the event itself isolates each link of the chain, so this + // catch only guards against a failure of the walk itself; + // an interceptor must not be able to keep the physical + // connection from being closed + log.warn("Interceptor threw an exception from the " + + "disconnected() event; ignoring it so that the " + + "physical connection can be closed.", x); + } + try { if (xaConnection == null) { connection.close(); } else { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDisconnectedInterceptor.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDisconnectedInterceptor.java new file mode 100644 index 0000000000..db419b1098 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDisconnectedInterceptor.java @@ -0,0 +1,111 @@ +/* + * 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.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A configured interceptor whose {@code disconnected()} hook throws must + * not be able to strand physical database connections or to keep later + * interceptors in the chain from being notified: the disconnect path + * isolates the hook from the physical close and every link of the chain + * from every other. + */ +class TestThrowingDisconnectedInterceptor extends JdbcPoolBaseTest { + + public static class BoomInterceptor extends JdbcInterceptor { + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // empty + } + + @Override + public void disconnected(ConnectionPool parent, PooledConnection con, + boolean finalizing) { + throw new IllegalStateException("boom from disconnected()"); + } + } + + public static class RecordingInterceptor extends JdbcInterceptor { + static final AtomicInteger DISCONNECTED_CALLS = new AtomicInteger(); + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // empty + } + + @Override + public void disconnected(ConnectionPool parent, PooledConnection con, + boolean finalizing) { + DISCONNECTED_CALLS.incrementAndGet(); + } + } + + @Test + void throwingDisconnectedInterceptorDoesNotLeakPhysicalConnections() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setJdbcInterceptors(BoomInterceptor.class.getName()); + Connection first = ds.getConnection(); + Connection second = ds.getConnection(); + // Hold strong references to the physical connections for the whole + // test: an abandoned, unclosed physical connection is otherwise + // reclaimed by the JDK's socket cleaner and the leaked database + // sessions would disappear with GC timing instead of staying open + java.sql.Connection physicalFirst = + ((PooledConnection) first.unwrap(PooledConnection.class)).getConnection(); + java.sql.Connection physicalSecond = + ((PooledConnection) second.unwrap(PooledConnection.class)).getConnection(); + awaitSessions(2); + first.close(); + second.close(); + // closing the pool disconnects both connections; the hook throws + // for both, the physical connections must still be closed + ds.close(); + awaitSessions(0); + Assertions.assertTrue(physicalFirst.isClosed(), + "the physical connection must be closed despite the throwing hook"); + Assertions.assertTrue(physicalSecond.isClosed(), + "the physical connection must be closed despite the throwing hook"); + } + + @Test + void throwingLinkDoesNotAbortTheDisconnectedChain() throws Exception { + RecordingInterceptor.DISCONNECTED_CALLS.set(0); + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setJdbcInterceptors( + BoomInterceptor.class.getName() + ";" + RecordingInterceptor.class.getName()); + Connection con = ds.getConnection(); + // keep the physical connection strongly reachable (see above) + java.sql.Connection physical = + ((PooledConnection) con.unwrap(PooledConnection.class)).getConnection(); + con.close(); + ds.close(); + Assertions.assertEquals(1, RecordingInterceptor.DISCONNECTED_CALLS.get(), + "interceptors after a throwing link must still receive disconnected()"); + awaitSessions(0); + Assertions.assertTrue(physical.isClosed(), + "the physical connection must be closed despite the throwing link"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
