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 2efc5bf56b4589dbed7c5bec0ff046df8859d262 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 17:09:02 2026 +0100 Keep a statement whose borrow-time retry close fails StatementFinalizer.reset() cleared the pending list, tried each entry once and dropped the failures after logging them. A statement whose retry close failed could therefore still be open on the session being served to the new borrower, while the return-time drain keeps its failures for another attempt. Collect the failures in reset() and re-add them under statementsLock, matching closeInvoked(). Update the permanent-refusal test: one leaked statement keeps one entry across borrows rather than being dropped after the borrow retry. --- .../jdbc/pool/interceptor/StatementFinalizer.java | 21 ++++++++++- .../pool/interceptor/TestStatementFinalizer.java | 44 ++++++++++++++++++---- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java index fe7f670586..00b8989e2e 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java @@ -46,8 +46,12 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * <p> * A close that reaches the statement by another route cannot be observed by that removal, so its entry is reclaimed at * the connection return: there the renewed close is a no-op on a statement the application had already closed, and the - * close it still owed on one it had not. A creation still in flight while the connection is returned is the one case - * that return cannot reach - it registers after the return drained the list - and is closed where it arrives instead of + * close it still owed on one it had not. That assumes the swallowed close did not itself recycle the statement into a + * cache: a recycling interceptor below this one would have parked it open, and the return-time close would then close + * a statement a live cache entry still holds. {@link StatementCache} is the module's recycling interceptor and refuses + * to park while it sits below this finalizer, so the ordering is safe for it; a third-party recycling interceptor is + * not covered by that guarantee. A creation still in flight while the connection is returned is the one case that + * return cannot reach - it registers after the return drained the list - and is closed where it arrives instead of * being tracked. * <p> * Tracked statements are handed to the application as proxies implementing the standard JDBC statement interfaces, as @@ -213,12 +217,25 @@ public class StatementFinalizer extends AbstractCreateStatementInterceptor { // again, so what a failed return-time close left behind gets its // last attempt here, before the new borrower runs. if (left != null && con != null) { + List<StatementEntry> failed = null; for (StatementEntry entry : left) { if (!entry.close()) { + if (failed == null) { + failed = new ArrayList<>(); + } + failed.add(entry); log.warn(MessageFactory.get("statementFinalizer.unableToCloseStatement"), entry.getAllocationStack()); } } + if (failed != null) { + // The statement may still be open on the session now being + // served to the new borrower: keep it for the next attempt, + // as the return-time drain does. + synchronized (statementsLock) { + statements.addAll(failed); + } + } } super.reset(parent, con); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java index e713de96dc..0035b15065 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementFinalizer.java @@ -592,6 +592,31 @@ class TestStatementFinalizer extends JdbcPoolBaseTest { }); } + @Test + void aRetryCloseFailureDuringTheNextBorrowStaysOnTheList() throws Exception { + DataSource ds = finalizerPool(); + ds.createPool(); + try (Connection pooled = ds.getConnection()) { + StatementFinalizer finalizer = interceptorOf(pooled, StatementFinalizer.class); + PooledConnection pcon = (PooledConnection) pooled.unwrap(PooledConnection.class); + Method create = Connection.class.getMethod("createStatement"); + + finalizer.reset(ds.getPool(), pcon); + Statement refusing = fakedRefusingStatement(); + finalizer.createStatement(null, create, null, refusing, 0L); + finalizer.closeInvoked(); + Assertions.assertEquals(1, tracked(finalizer), + "precondition: the return-time close failed and kept the entry"); + + // The next borrow retries, and the driver refuses again. The entry + // must survive for the attempt after that; the statement may still + // be open on the session the new borrower is using. + finalizer.reset(ds.getPool(), pcon); + Assertions.assertEquals(1, tracked(finalizer), + "a failed retry close must stay on the list instead of being dropped"); + } + } + @Test void traceReportsOnlyWhatTheDriverCouldAnswer() throws Exception { // With trace on, the interceptor tells the operator which statements @@ -741,10 +766,12 @@ class TestStatementFinalizer extends JdbcPoolBaseTest { } @Test - void aCloseThatNeverSucceedsIsGivenItsLastAttemptAtTheNextBorrow() throws Exception { - // A driver that cannot close at all must not grow the list across - // borrows: the reset at the next borrow is the last attempt, the - // pool says so at warn level, and the pool keeps serving. + void aCloseThatNeverSucceedsKeepsItsEntryForALaterAttempt() throws Exception { + // A driver that cannot close at all must not lose the entry: the + // statement may still be open on the session the next borrower is + // using, so the failed retry at the borrow keeps it for the return + // path, which retries on every return as well. One leaked statement + // produces one entry, not one per borrow. DataSource ds = finalizerPoolGated(); ds.createPool(); @@ -764,14 +791,15 @@ class TestStatementFinalizer extends JdbcPoolBaseTest { Connection second = ds.getConnection(); try { - Assertions.assertEquals(0, tracked(finalizer), - "the retry at the borrow is final - a permanently refusing driver must not " - + "accumulate entries across borrows"); + Assertions.assertEquals(1, tracked(finalizer), + "a failed retry at the borrow must keep the entry: the statement may still be " + + "open on the session the new borrower is using"); Assertions.assertFalse(rawLeaked.isClosed(), "precondition: the driver never took the close"); } finally { second.close(); } - Assertions.assertEquals(0, tracked(finalizer), "the next return of an empty list stays empty"); + Assertions.assertEquals(1, tracked(finalizer), + "the return-time retry also fails, so the entry stays for the next attempt"); } @Test --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
