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 e00e62150ef8a42b1948b03e13719c12b7638663 Author: opencode <[email protected]> AuthorDate: Mon Oct 26 17:33:44 2026 +0100 Isolate interceptor reset() failures from pool shutdown The shutdown-time reset(null,null) callback was walked unguarded in ConnectionPool.finalize() and in PooledConnection.setHandler(), which resets the old chain before replacing it and assigned the new handler only after that walk. An interceptor that threw from its shutdown reset therefore aborted the walk for the rest of the chain, could leave a released PooledConnection holding a stale interceptor chain, and - the walk being on the path of ConnectionPool.release() - propagated out of the drain loop of close(), which had no guard of its own. The pool, already marked closed, then kept every remaining idle and busy physical connection open: a dead pool holding live database connections, with only a stack trace in the log as evidence. Both walks now read each link's successor before invoking the callback (the callback may modify the link) and continue past a link that throws, naming the offending class in a warning. setHandler() assigns the new handler in a finally block so a failing reset walk cannot keep a stale chain on a destroyed connection. The drain loop of close() guards each release()/abandon() call likewise, so one failing connection cannot strand the others. The borrow-time reset(pool,con) walk in setupConnection is intentionally unchanged. Adds TestThrowingResetInterceptor, asserting database-side that closing the pool disconnects every pooled connection, and that an interceptor behind a throwing link is still reset for shutdown and no destroyed connection keeps a stale chain. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 29 ++++-- .../apache/tomcat/jdbc/pool/JdbcInterceptor.java | 2 + .../apache/tomcat/jdbc/pool/PooledConnection.java | 32 ++++-- .../jdbc/pool/TestThrowingResetInterceptor.java | 111 +++++++++++++++++++++ 4 files changed, 160 insertions(+), 14 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 cafadb5444..67a3b963f4 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 @@ -453,10 +453,16 @@ public class ConnectionPool { //close it and retrieve the next one, if one is available while (con != null) { //close the connection - if (pool==idle) { - release(con); - } else { - abandon(con); + try { + if (pool==idle) { + release(con); + } else { + abandon(con); + } + } catch (Throwable x) { + //one failing connection must not leave the + //remaining idle and busy connections undisconnected + log.warn("Unable to close pooled connection.", x); } if (!pool.isEmpty()) { con = pool.poll(1000, TimeUnit.MILLISECONDS); @@ -1452,8 +1458,19 @@ public class ConnectionPool { protected void finalize(PooledConnection con) { JdbcInterceptor handler = con.getHandler(); while (handler!=null) { - handler.reset(null, null); - handler=handler.getNext(); + //read the next link before the callback, as the callback + //is allowed to modify it + JdbcInterceptor next = handler.getNext(); + try { + handler.reset(null, null); + } catch (Throwable t) { + //one broken interceptor must not stop the rest of the + //chain from being reset + log.warn("Interceptor [" + handler.getClass().getName() + + "] threw an exception from reset(); " + + "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 a2b2bc4d18..8d6fc078ad 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 @@ -158,6 +158,8 @@ public abstract class JdbcInterceptor implements InvocationHandler { * the interceptor can be reused for another connection. * <br> * This method may be called with null as both arguments when we are closing down the connection. + * When called for a shutdown reset, an interceptor that throws is isolated: the exception + * is logged and ignored, and the remaining interceptors in the chain are reset as well. * @param parent - the connection pool owning the connection * @param con - the pooled 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 f188b76a3a..043e3cbd6b 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 @@ -865,14 +865,30 @@ public class PooledConnection implements PooledConnectionMBean { * @param handler the first interceptor in the chain */ public void setHandler(JdbcInterceptor handler) { - if (this.handler!=null && this.handler!=handler) { - JdbcInterceptor interceptor = this.handler; - while (interceptor!=null) { - interceptor.reset(null, null); - interceptor = interceptor.getNext(); - }//while - }//end if - this.handler = handler; + try { + if (this.handler!=null && this.handler!=handler) { + JdbcInterceptor interceptor = this.handler; + while (interceptor!=null) { + //read the next link before the callback, as the callback + //is allowed to modify it + JdbcInterceptor next = interceptor.getNext(); + try { + interceptor.reset(null, null); + } catch (Throwable t) { + //one broken interceptor must not stop the rest of the + //chain from being reset + log.warn("Interceptor [" + interceptor.getClass().getName() + + "] threw an exception from reset(); " + + "continuing with the rest of the chain.", t); + } + interceptor = next; + }//while + }//end if + } finally { + //a released connection must never keep a stale chain, + //even if resetting the old one failed + this.handler = handler; + } } @Override diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingResetInterceptor.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingResetInterceptor.java new file mode 100644 index 0000000000..08558a7714 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingResetInterceptor.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; + +/** + * An interceptor whose shutdown-time {@code reset(null, null)} throws must + * not abort the drain loop of {@code close()}: every other pooled + * connection must still be disconnected, and the remaining interceptors + * in the chain must still receive their reset callback. + */ +class TestThrowingResetInterceptor extends JdbcPoolBaseTest { + + public static class ThrowResetInterceptor extends JdbcInterceptor { + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + if (con == null) { + throw new IllegalStateException("boom from reset(null,null)"); + } + } + } + + public static class RecordingInterceptor extends JdbcInterceptor { + static final AtomicInteger SHUTDOWN_RESETS = new AtomicInteger(); + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + if (con == null) { + SHUTDOWN_RESETS.incrementAndGet(); + } + } + } + + @Test + void throwingShutdownResetDoesNotAbortThePoolClose() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setJdbcInterceptors(ThrowResetInterceptor.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 + PooledConnection pooledFirst = + (PooledConnection) first.unwrap(PooledConnection.class); + PooledConnection pooledSecond = + (PooledConnection) second.unwrap(PooledConnection.class); + java.sql.Connection physicalFirst = pooledFirst.getConnection(); + java.sql.Connection physicalSecond = pooledSecond.getConnection(); + awaitSessions(2); + first.close(); + second.close(); + // closing the pool resets both connections; the shutdown hook + // throws for both, every connection must still be disconnected + ds.close(); + awaitSessions(0); + Assertions.assertTrue(physicalFirst.isClosed(), + "the physical connection must be closed despite the throwing reset"); + Assertions.assertTrue(physicalSecond.isClosed(), + "the physical connection must be closed despite the throwing reset"); + Assertions.assertNull(pooledFirst.getHandler(), + "a destroyed connection must not keep a stale interceptor chain"); + Assertions.assertNull(pooledSecond.getHandler(), + "a destroyed connection must not keep a stale interceptor chain"); + } + + @Test + void throwingLinkDoesNotAbortTheShutdownResetChain() throws Exception { + RecordingInterceptor.SHUTDOWN_RESETS.set(0); + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setJdbcInterceptors( + ThrowResetInterceptor.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(); + // a shutdown reset walks the chain twice by existing design + // (once from finalize(), once from release()'s setHandler(null)), + // so only the presence of a callback past the throwing link is pinned + Assertions.assertTrue(RecordingInterceptor.SHUTDOWN_RESETS.get() >= 1, + "interceptors after a throwing link must still be reset for shutdown"); + awaitSessions(0); + Assertions.assertTrue(physical.isClosed(), + "the physical connection must be closed despite the throwing reset"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
