This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 42e3f3f4703604c995cc91ba060f95759c08bdc3 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 23:27:22 2026 +0100 Deliver the interceptor teardown exactly once A releasing connection ran reset(null, null) on every interceptor twice: disconnect(true) called ConnectionPool.finalize(), and the pool release then called PooledConnection.setHandler(null), which walked the chain again. Built-in interceptors tolerate the duplicate, but a third-party interceptor that releases a resource in the teardown branch can release it twice. Leave the teardown walk to the pool release, after it frees the slot, and drop the handler with clearHandler() instead of a second walking setHandler(null). Update the tests that drive release to go through ConnectionPool.release(), and assert one teardown. --- .../java/org/apache/tomcat/jdbc/pool/ConnectionPool.java | 8 ++++++-- .../org/apache/tomcat/jdbc/pool/PooledConnection.java | 15 ++++++++++++--- .../tomcat/jdbc/pool/TestCloseVsConcurrentRelease.java | 15 ++++++++++----- .../apache/tomcat/jdbc/pool/TestCloseVsTeardownWalk.java | 9 +++++---- .../jdbc/pool/TestPlaceholderVersusNewBorrower.java | 11 +++++------ .../tomcat/jdbc/pool/TestThrowingResetInterceptor.java | 9 ++++----- .../apache/tomcat/jdbc/pool/TestUnwrapVsMidGuardSeal.java | 6 +++--- .../TestSlowQueryReportDeadSessionWarning.java | 5 ++--- .../TestSlowQueryReportSlowLogAfterChainRelease.java | 6 +++--- 9 files changed, 50 insertions(+), 34 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 915a47f023..1e91f67780 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 @@ -563,7 +563,7 @@ public class ConnectionPool { // no close() will ever travel that chain - the handle was never handed out - while the return path does // not walk the chain either. The connection itself is healthy; it is the borrow state that is unusable, // and there is no chain-wide unwind that is safe for arbitrary interceptors, as each keys its disposal - // on the reset(null, null) call the release's setHandler(null) makes (StatementCache.reset is one). + // on the reset(null, null) call the release's teardown walk makes (StatementCache.reset is one). // So the half-armed chain goes out of service with the connection, the posture the two catches above // take, rather than being parked idle armed for a borrow that never started. abandonSetup(con, s, true); @@ -1328,7 +1328,11 @@ public class ConnectionPool { if (destroyed) { // Counter only decremented once size.addAndGet(-1); - con.setHandler(null); + // The one teardown reset, delivered after the slot has been + // freed and before the wake-up placeholder is offered; the + // handler is then dropped without resetting it again + finalize(con); + con.clearHandler(); // Counted with the destruction, not with the call: a // connection the abandon path already released can reach // this method once more - a return whose busy.remove lost 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 c3662653f3..5181de646b 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 @@ -798,9 +798,10 @@ public class PooledConnection implements PooledConnectionMBean { xaConnection = null; cpdsConnection = null; lastConnected = -1; - if (finalize) { - parent.finalize(this); - } + // The chain's teardown reset is delivered once, by the release that + // clears the handler after it has freed the pool slot - see + // ConnectionPool.release(). The disconnect event above is a separate + // notification and does not replace it. } /** @@ -1604,6 +1605,14 @@ public class PooledConnection implements PooledConnectionMBean { return handler; } + /** + * Drops the cached interceptor chain after the release has delivered its teardown reset, without walking it a + * second time. + */ + void clearHandler() { + this.handler = null; + } + /** * Sets the interceptor chain handler for this connection. * diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsConcurrentRelease.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsConcurrentRelease.java index 3a32ea3d15..6bcc2d9180 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsConcurrentRelease.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsConcurrentRelease.java @@ -164,7 +164,10 @@ class TestCloseVsConcurrentRelease { PooledConnection con = connectedTo(pool, rec); AtomicBoolean firstResult = new AtomicBoolean(); - Thread releaser = new Thread(() -> firstResult.set(con.release()), "first-releaser"); + Thread releaser = new Thread(() -> { + pool.release(con); + firstResult.set(con.isReleased()); + }, "first-releaser"); releaser.start(); Assertions.assertTrue(pool.insideFinalizing.await(10, TimeUnit.SECONDS), "the first release must have reached the chain's terminal notice"); @@ -173,7 +176,8 @@ class TestCloseVsConcurrentRelease { // first hand is inside the notice, the session is marked discarded // but still visibly open. This hand must claim nothing and touch // nothing. - Assertions.assertFalse(con.release(), + pool.release(con); + Assertions.assertTrue(con.isReleased(), "a release that arrives during the terminal sequence loses the claim"); Assertions.assertEquals(1, pool.disconnectEvents.get(), "the terminal notice must have gone through the chain once, not once per hand"); @@ -195,7 +199,7 @@ class TestCloseVsConcurrentRelease { CountingPool pool = countingPool(rec); PooledConnection con = connectedTo(pool, rec); - Thread releaser = new Thread(con::release, "first-releaser"); + Thread releaser = new Thread(() -> pool.release(con), "first-releaser"); releaser.start(); Assertions.assertTrue(pool.insideFinalizing.await(10, TimeUnit.SECONDS), "the first release must have reached the chain's terminal notice"); @@ -225,8 +229,9 @@ class TestCloseVsConcurrentRelease { // first one, so the notice does not need to park anyone. pool.proceed.countDown(); - Assertions.assertTrue(con.release(), "the first release wins"); - Assertions.assertFalse(con.release(), "every later release loses"); + pool.release(con); + Assertions.assertTrue(con.isReleased(), "the first release wins"); + pool.release(con); Assertions.assertEquals(1, pool.disconnectEvents.get(), "the terminal notice ran once, though two hands came"); Assertions.assertEquals(1, pool.finalizations.get()); Assertions.assertEquals(1, rec.closeCalls.get(), "the session was closed once, though two hands came"); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsTeardownWalk.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsTeardownWalk.java index bde3933eb7..275a541cea 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsTeardownWalk.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseVsTeardownWalk.java @@ -49,7 +49,8 @@ import org.apache.tomcat.jdbc.JdbcPoolBaseTest; * either. This class pins that contract at each position the close can meet * the walk - mid-walk, after it, and from inside it - and pins that the * teardown walk ends the borrow exactly once whichever position the close - * took. + * took. The pool's release path delivers that walk once, through + * {@code finalize()}. */ class TestCloseVsTeardownWalk extends JdbcPoolBaseTest { @@ -57,9 +58,9 @@ class TestCloseVsTeardownWalk extends JdbcPoolBaseTest { * A pool whose borrow the abandoned-connection check claims on demand: * the timeout is one second and the tests push the borrow past it with * setTimestamp(0) rather than sleeping, so every run drives a real - * release - disconnect(true), the ConnectionPool.finalize walk and the - * setHandler(null) walk, each resetting every cached interceptor to - * (null, null) - without depending on a timer thread. + * release - disconnect(true) and the release's finalize walk, which + * resets every cached interceptor to (null, null) - without depending on + * a timer thread. */ private DataSource abandonedCheckPool() throws SQLException { DataSource ds = newDataSource(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPlaceholderVersusNewBorrower.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPlaceholderVersusNewBorrower.java index 4df976fc4d..df2128b7f7 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPlaceholderVersusNewBorrower.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPlaceholderVersusNewBorrower.java @@ -61,12 +61,11 @@ class TestPlaceholderVersusNewBorrower extends JdbcPoolBaseTest { @Override public void reset(ConnectionPool parent, PooledConnection con) { - // A retiring connection's chain is reset with no parent twice: - // once while the session is disconnected, before the release - // frees the pool slot, and once when the release clears the - // chain, after it. The second firing sits exactly between the - // slot's release and the wake-up placeholder. - if (parent == null && ARMED.get() && PARENTLESS_RESETS.incrementAndGet() == 2) { + // A retiring connection's chain is reset with no parent once, + // when the release clears it after freeing the pool slot. That + // firing sits exactly between the slot's release and the wake-up + // placeholder. + if (parent == null && ARMED.get() && PARENTLESS_RESETS.incrementAndGet() == 1) { PARKED.countDown(); try { GO.await(5, TimeUnit.SECONDS); 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 index b35b8439af..3b7787fff4 100644 --- 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 @@ -98,11 +98,10 @@ class TestThrowingResetInterceptor extends JdbcPoolBaseTest { java.sql.Connection physical = 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"); + // the shutdown reset walks the chain once, through finalize() + Assertions.assertEquals(1, RecordingInterceptor.SHUTDOWN_RESETS.get(), + "interceptors after a throwing link must still be reset for shutdown," + + " and the teardown must be delivered exactly once"); awaitSessions(0); Assertions.assertTrue(physical.isClosed(), "the physical connection must be closed despite the throwing reset"); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestUnwrapVsMidGuardSeal.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestUnwrapVsMidGuardSeal.java index 8f37c0d59c..caab50c147 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestUnwrapVsMidGuardSeal.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestUnwrapVsMidGuardSeal.java @@ -48,9 +48,9 @@ class TestUnwrapVsMidGuardSeal extends JdbcPoolBaseTest { * A pool whose borrow the abandoned-connection check claims on demand: * the timeout is one second and the tests push the borrow past it with * setTimestamp(0) rather than sleeping, so every run drives a real - * release - disconnect(true) and the setHandler(null) walk that resets - * every cached interceptor to (null, null) - without depending on a - * timer thread. + * release - disconnect(true) and the teardown walk that resets every + * cached interceptor to (null, null) - without depending on a timer + * thread. */ private DataSource abandonedCheckPool() throws SQLException { DataSource ds = newDataSource(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDeadSessionWarning.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDeadSessionWarning.java index 0731fc2cf4..48da6563f2 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDeadSessionWarning.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportDeadSessionWarning.java @@ -49,9 +49,8 @@ import org.apache.tomcat.jdbc.pool.DataSource; * filled the log with it. * <p> * The tests drive the release the way the pool itself delivers it - - * {@code reset(null, null)} on the interceptor link, the call - * {@code PooledConnection.setHandler(null)} walks a released chain with - - * onto the live interceptor instance of a connection that is still borrowed, + * {@code reset(null, null)} on the interceptor link, which the teardown walk + * calls - onto the live interceptor instance of a connection that is still borrowed, * which is also the state a borrow racing the close of the pool's last live * same-named pool lands in. One loss of the store now earns one WARN per * interceptor instance; a booking made on a recovered store re-arms the diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportSlowLogAfterChainRelease.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportSlowLogAfterChainRelease.java index 7564854c38..2baa1597c9 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportSlowLogAfterChainRelease.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryReportSlowLogAfterChainRelease.java @@ -94,9 +94,9 @@ class TestSlowQueryReportSlowLogAfterChainRelease extends JdbcPoolBaseTest { /* * Releases the statistics reference the way the pool's own chain - * release does - reset(null, null) on the interceptor link, the call - * PooledConnection.setHandler(null) walks the chain with - while the - * borrow, the session and the application's statement all stay live, + * release does - reset(null, null) on the interceptor link, which the + * teardown walk calls - while the borrow, the session and the + * application's statement all stay live, * and pins that the window really opened: the instance can no longer * resolve any statistics entry. */ --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
