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 4dcec7a16557f65c7e4ea06d187ff6241ebadc4e Author: opencode <[email protected]> AuthorDate: Tue Oct 27 17:15:55 2026 +0100 Reset the abandoned timer on failed operations in the JDBC pool The ResetAbandonedTimer interceptor reset the abandoned timer only after an intercepted call had returned normally, and it did not override the failed-query reporting hook of AbstractQueryReport at all. An application that stayed active by repeatedly executing statements that fail - a retry loop against lock timeouts or transient SQL errors, or health-check traffic that returns errors - was therefore indistinguishable from an idle connection: once removeAbandonedTimeout elapsed without a successful call, the pool cleaner abandoned the connection and closed the physical session underneath the application, surfacing as unexpected SQLExceptions in what was actually live traffic. An attempted operation is activity whether or not it completes successfully. invoke() now resets the timer in a finally block, so failed connection-level calls are counted, and the interceptor overrides reportFailedQuery() so failed statement executions reset the timer the same way the successful reportQuery() and reportSlowQuery() hooks always have. Failures of statement methods that are not executions stay outside the reporting hooks, mirroring the existing behaviour for successful calls of those methods. Regression tests drive a failing-connection-level-call storm and a failing-statement-execution storm against a one-second abandoned timeout on a live H2 session and assert the busy connection keeps its original physical session while an idle control connection is abandoned; both failure tests fail on the pre-fix code with the session lost underneath them. --- .../jdbc/pool/interceptor/ResetAbandonedTimer.java | 19 +- .../pool/interceptor/TestResetAbandonedTimer.java | 208 +++++++++++++++++++++ 2 files changed, 223 insertions(+), 4 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ResetAbandonedTimer.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ResetAbandonedTimer.java index 6f6b36d10d..4be8bdd43b 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ResetAbandonedTimer.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/ResetAbandonedTimer.java @@ -26,7 +26,8 @@ import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; /** * Class that resets the abandoned timer on any activity on the - * Connection or any successful query executions. + * Connection or any query execution, whether the operation completed + * successfully or failed. * This interceptor is useful for when you have a {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#setRemoveAbandonedTimeout(int)} * that is fairly low, and you want to reset the abandoned time each time any operation on the connection is performed * This is useful for batch processing programs that use connections for extensive amount of times. @@ -74,9 +75,13 @@ public class ResetAbandonedTimer extends AbstractQueryReport implements ResetAba @Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { - Object result = super.invoke(proxy, method, args); - resetTimer(); - return result; + try { + return super.invoke(proxy, method, args); + } finally { + //the operation was attempted on the connection, which counts + //as activity even when it failed + resetTimer(); + } } @Override @@ -95,6 +100,12 @@ public class ResetAbandonedTimer extends AbstractQueryReport implements ResetAba resetTimer(); } + @Override + protected String reportFailedQuery(String query, Object[] args, String name,long start, Throwable t) { + resetTimer(); + return super.reportFailedQuery(query, args, name, start, t); + } + @Override protected String reportQuery(String query, Object[] args, String name,long start, long delta) { resetTimer(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimer.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimer.java new file mode 100644 index 0000000000..5ed2c95483 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestResetAbandonedTimer.java @@ -0,0 +1,208 @@ +/* + * 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.interceptor; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * The {@code ResetAbandonedTimer} interceptor must treat an attempted + * operation as activity, whether the operation succeeded or failed: an + * application kept busy by executions that fail (a retry loop against + * transient SQL errors) is active and must not have its connection + * abandoned underneath it, while a borrowed connection that really is + * idle must still be reclaimed. Every test drives a busy connection and + * an idle control connection against a one-second abandoned timeout and + * asserts which of the two physical sessions the cleaner killed. + */ +class TestResetAbandonedTimer extends JdbcPoolBaseTest { + + /* + * Time the busy connection is kept making calls. It must stay well + * above removeAbandonedTimeout plus one sweep interval so that the + * idle control connection - and, before the fix, the busy one too - + * is certain to expire while the storm is still running. + */ + private static final long STORM_MILLIS = 2500; + + /* + * Lower bound on storm iterations. At a one-second abandoned + * timeout a genuine retry loop makes far more calls than this; the + * bound only catches a storm that never really ran. + */ + private static final int MIN_STORM_CALLS = 5; + + private DataSource abandonedTimerPool() throws SQLException { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + "org.apache.tomcat.jdbc.pool.interceptor.ResetAbandonedTimer"); + ds.getPoolProperties().setRemoveAbandoned(true); + ds.getPoolProperties().setRemoveAbandonedTimeout(1); // seconds + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(100); + ds.createPool(); + pool = ds; + return ds; + } + + /* The pool under test, for forced teardown. */ + private DataSource pool; + + /* + * These tests deliberately end with connections still borrowed. The + * regular data source teardown only disconnects idle connections, so + * the sessions of the borrowed ones would linger until the garbage + * collector finalizes them and leak their session counts into the + * starting point of the next test; force-close disconnects the busy + * ones as well, right away. + */ + @AfterEach + void disconnectLeakedBusyConnections() { + if (pool != null) { + pool.close(true); + } + } + + /* + * The busy connection ends with one live physical session carrying + * the marker it was given before the storm, and the idle control + * connection has lost its session to the abandoned sweep. + */ + private void assertBusySurvivedAndIdleAbandoned(Connection busy) throws SQLException { + Integer marker = sessionMarker(busy); + Assertions.assertNotNull(marker, + "the session of the busy connection must still be alive"); + Assertions.assertEquals(1, marker.intValue(), + "the busy connection must still own its original session"); + awaitSessions(1); + } + + /* + * Borrows an idle control connection - left borrowed and untouched, + * it exists only to prove the abandoned sweep is running - and a + * busy connection, each identified by a session marker, and pins + * the session count to two while both are still live. The caller + * must have waited out any sessions dying from a previous test + * first: with a one-second abandoned timeout there is no time to + * absorb stale sessions once these connections have started their + * countdown. Returns the busy connection. + */ + private Connection borrowBusyWithIdleControl(DataSource ds) throws SQLException { + Connection idle = ds.getConnection(); + setSessionMarker(idle, 2); + Connection busy = ds.getConnection(); + setSessionMarker(busy, 1); + Assertions.assertEquals(2, poolSessions(), + "both borrowed connections must start on live physical sessions"); + return busy; + } + + @Test + void successfulQueriesKeepConnectionAliveWhileIdleControlIsAbandoned() throws Exception { + awaitSessions(0); + DataSource ds = abandonedTimerPool(); + Connection busy = borrowBusyWithIdleControl(ds); + + int queries = 0; + long deadline = System.currentTimeMillis() + STORM_MILLIS; + try (Statement st = busy.createStatement()) { + do { + try (ResultSet rs = st.executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next()); + } + queries++; + } while (System.currentTimeMillis() < deadline); + } + Assertions.assertTrue(queries >= MIN_STORM_CALLS, + "the keep-alive loop must have run, only " + queries + " queries"); + + assertBusySurvivedAndIdleAbandoned(busy); + } + + /* + * An application retrying statements that all fail - against lock + * timeouts, transient SQL errors, or simply bad SQL - is active. + * The failures arrive through the failed-query reporting hook, + * which must reset the abandoned timer just like the successful + * report hooks do; otherwise the pool cleaner kills the connection + * under the app mid-retry. + */ + @Test + void failedStatementExecutionsKeepConnectionAlive() throws Exception { + awaitSessions(0); + DataSource ds = abandonedTimerPool(); + Connection busy = borrowBusyWithIdleControl(ds); + + String sql = "SELECT * FROM no_such_table_" + System.nanoTime(); + int failures = 0; + long deadline = System.currentTimeMillis() + STORM_MILLIS; + try (Statement st = busy.createStatement()) { + do { + SQLException failure = Assertions.assertThrows(SQLException.class, + () -> st.executeQuery(sql)); + // Fail on the missing table, never on a session lost mid-storm. + Assertions.assertEquals("42S04", failure.getSQLState(), + "the storm must fail on the missing table: " + failure.getMessage()); + failures++; + } while (System.currentTimeMillis() < deadline); + } + Assertions.assertTrue(failures >= MIN_STORM_CALLS, + "the failure storm must have run, only " + failures + " failures"); + + assertBusySurvivedAndIdleAbandoned(busy); + } + + /* + * A failing call made directly on the pooled connection - here a + * prepare that the driver rejects because the SQL names a missing + * table - is activity as much as a successful one. The reset was + * performed only after the intercepted call returned normally, so + * an application that stays active through failing connection-level + * calls was abandoned like an idle one. + */ + @Test + void failedConnectionLevelCallsKeepConnectionAlive() throws Exception { + awaitSessions(0); + DataSource ds = abandonedTimerPool(); + Connection busy = borrowBusyWithIdleControl(ds); + + String sql = "SELECT * FROM no_such_table_" + System.nanoTime(); + int failures = 0; + long deadline = System.currentTimeMillis() + STORM_MILLIS; + do { + // H2 compiles at prepare time, so the prepare itself throws + SQLException failure = Assertions.assertThrows(SQLException.class, + () -> busy.prepareStatement(sql)); + // Fail on the missing table, never on a session lost mid-storm. + Assertions.assertEquals("42S04", failure.getSQLState(), + "the storm must fail on the missing table: " + failure.getMessage()); + failures++; + } while (System.currentTimeMillis() < deadline); + Assertions.assertTrue(failures >= MIN_STORM_CALLS, + "the failure storm must have run, only " + failures + " failures"); + + assertBusySurvivedAndIdleAbandoned(busy); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
