This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 26228e3f32553c9acf03e518ab33f7f5cca3812d Author: opencode <[email protected]> AuthorDate: Tue Nov 3 15:14:33 2026 +0100 Seal the statement facade's handles when a borrow is aborted Connection.abort() ends a borrow the same way Connection.close() does: the connection facade disposes itself and delegates the call down the chain. The base interceptor, though, only forwards close to closeInvoked(), so an abort flowed through the statement facade without setting the borrow-ended flag, and every statement and result set handle created during that borrow kept its live delegate: isClosed() forwarded to the driver and answered false, unwrap() could still hand out the driver's own statement, and use() was arbitrated by the driver rather than refused with the pool's closed-handle contract. The session behind an aborted borrow never goes back to the idle queue - it is marked discarded and taken out of service - so this was not a cross-borrower hazard, but the handles of an aborted borrow were still reading and driving a session their owner had lost. The facade now seals its handles on abort as well, ahead of the call flowing down, mirroring what it already does on close, and the closeInvoked() javadoc is corrected to say it runs on both. A new test drives an abort through the facade with a driver whose abort is a no-op (H2) and pins that the statement and result set handles report themselves closed, refuse use and unwrap, and stay harmless on a second close. The statement facade's own documentation went with it. Two javadoc claims copied in from the query-report interceptors described a purpose the class does not have - it measures nothing, the elapsed- time parameter of createStatement() simply is not read - and now say what the hook and the statement proxy actually do. The statement proxy's closed field was deleted: never written and never read, it advertised a closed state that was always the initial false while the close contract lives in the null delegate and the borrow-ended flag. Inline comments lost the labels that only restated the adjacent code. The catch in createStatement() that answers a failed statement-proxy wrap with the raw object from beneath cannot be reached from application behavior - apart from the test seam that simulates a failed class initializer, the constructor cache is written only by that initializer, and a proxy constructor cannot refuse its handler argument - so the branch stays, upstream's posture of handing the caller a working statement, but it is now documented at the catch, including the one cost it carries if it ever did run: the object handed back reads no borrow-ended flag. Its warn, the last literal message in the class, now reads statementFacade.unableToCreateStatementProxy from the message bundle like every other info-and-above message of the pool. --- .../apache/tomcat/jdbc/pool/StatementFacade.java | 57 ++++++++++++++++------ .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../tomcat/jdbc/pool/TestAbortEndsTheBorrow.java | 37 ++++++++++++++ 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/StatementFacade.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/StatementFacade.java index d67e5d6d4e..97bb416f4d 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/StatementFacade.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/StatementFacade.java @@ -84,11 +84,30 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { setNext(interceptor); } + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + // The facade above ends the borrow on close and on abort alike, but + // the base class forwards only close to closeInvoked(), so seal the + // handles here for abort as well: an aborted borrow retires its + // statements and result sets exactly as a closed one does, rather + // than leaving them to answer from the driver on a session this + // handle can no longer claim. The seal runs ahead of the forward, as + // in the base class's close path, so no handle can observe the flag + // as live after the session has left service. Of the dispose calls + // the facade above lets exactly one reach this chain, and setting the + // flag is idempotent in any case. + if (compare(ABORT_VAL, method)) { + closeInvoked(); + } + return super.invoke(proxy, method, args); + } + /** * Marks the borrow as ended, which closes every statement and result set handle this facade handed out. Called - * before the connection is handed back, so a handle cannot outlive the borrow and end up driving the session of the - * next borrower. A borrow that is abandoned instead never runs this method, but the pool destroys an abandoned - * session rather than lending it again, so those handles only fail at the driver. + * before the connection leaves the application - on close and, through {@link #invoke}, on abort - so a handle + * cannot outlive the borrow and end up driving the session of the next borrower. A borrow that is abandoned + * instead never runs this method, but the pool destroys an abandoned session rather than lending it again, so + * those handles only fail at the driver. */ @Override public void closeInvoked() { @@ -96,7 +115,7 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { } /** - * Creates a statement interceptor to monitor query response times + * Wraps the statement produced underneath in a handle that retires with the borrow. */ @Override public Object createStatement(Object proxy, Method method, Object[] args, Object statement, long time) { @@ -105,35 +124,40 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { Constructor<?> constructor = null; String sql = null; if (compare(CREATE_STATEMENT, name)) { - // createStatement constructor = getConstructor(CREATE_STATEMENT_IDX, Statement.class); } else if (compare(PREPARE_STATEMENT, name)) { - // prepareStatement constructor = getConstructor(PREPARE_STATEMENT_IDX, PreparedStatement.class); sql = (String) args[0]; } else if (compare(PREPARE_CALL, name)) { - // prepareCall constructor = getConstructor(PREPARE_CALL_IDX, CallableStatement.class); sql = (String) args[0]; } else { - // Do nothing + // Not a statement creation - pass the object from beneath through return statement; } return constructor.newInstance(new Object[] { new StatementProxy(statement, sql, proxy) }); } catch (Exception x) { - logger.warn("Unable to create statement proxy.", x); + // Defensive branch: reaching it requires the proxy machinery + // to fail for the statement interfaces after it already built + // this borrow's connection handle - the constructor cache the + // wrap consults is written only by the class initializer and + // by a test seam that simulates a failed one, and a proxy + // constructor cannot refuse its single handler argument - so + // no application behavior gets here. Should it ever, the + // caller keeps a working statement, the posture upstream + // took, at the one cost that this facade's seal does not + // cover the object handed back; the warn below is that cost's + // only report. + logger.warn(MessageFactory.get("statementFacade.unableToCreateStatementProxy"), x); } return statement; } /** - * Proxy for a SQL statement used to measure query execute time. + * Proxy for a SQL statement. Refuses calls once the borrow that created it has ended, and seals every result set + * the statement hands back. */ protected class StatementProxy implements InvocationHandler { - /** - * Indicates whether this statement has been closed. - */ - protected boolean closed = false; /** * The delegated statement, nulled once a close has been forwarded. A handle can be closed from a thread other * than the one invoking on it, so the field is read by more threads than the one that nulls it and, like the @@ -253,7 +277,8 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { throw t; } } - // Perform close cleanup + // Drop the driver reference; the null delegate is what answers + // as closed from here on. if (compare(CLOSE_VAL, method)) { delegate = null; } @@ -383,7 +408,7 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { throw t; } } - // Perform close cleanup + // Drop the driver reference, as in the statement proxy. if (compare(CLOSE_VAL, method)) { delegate = null; } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index c2ff5bd634..29c076edea 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -215,6 +215,7 @@ statementFacade.resultSetClosed=ResultSet closed. statementFacade.resultSetClosedConnectionReturned=ResultSet closed. The pooled connection this result set was read from has been returned to the pool. statementFacade.statementClosed=Statement closed. statementFacade.statementClosedConnectionReturned=Statement closed. The pooled connection this statement was created from has been returned to the pool. +statementFacade.unableToCreateStatementProxy=Unable to create statement proxy. statementFinalizer.statementNotClosed=Statement created, but was not closed at: statementFinalizer.unableToCreateTrackingProxy=Unable to create statement tracking proxy. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbortEndsTheBorrow.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbortEndsTheBorrow.java index 2c475f4508..64baa83107 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbortEndsTheBorrow.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAbortEndsTheBorrow.java @@ -21,8 +21,10 @@ import java.lang.reflect.Proxy; import java.sql.Connection; import java.sql.Driver; import java.sql.DriverPropertyInfo; +import java.sql.ResultSet; import java.sql.SQLException; import java.sql.SQLFeatureNotSupportedException; +import java.sql.Statement; import java.util.Properties; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; @@ -127,6 +129,41 @@ class TestAbortEndsTheBorrow extends JdbcPoolBaseTest { } } + @Test + void anAbortSealsTheStatementAndResultSetHandlesOfTheBorrow() throws Exception { + // Plain H2 here on purpose, as in the test above: its abort is a + // no-op that leaves the session open, so this is the honest probe + // that the seal runs at the pool layer - the handles of the aborted + // borrow must retire whatever the driver's abort did, mirroring the + // close-path pins of TestStatementHandleLifetime. + DataSource ds = abortPool(org.h2.Driver.class, 2); + + Connection first = ds.getConnection(); + Statement survivor = first.createStatement(); + ResultSet cursor = survivor.executeQuery("SELECT 1"); + Assertions.assertTrue(cursor.next(), "precondition: the cursor is live during the borrow"); + Assertions.assertFalse(survivor.isClosed(), "precondition: the handle is live during the borrow"); + first.abort(DIRECT_EXECUTOR); + + Assertions.assertTrue(survivor.isClosed(), + "a statement whose borrow an abort ended must report itself closed"); + SQLException thrown = Assertions.assertThrows(SQLException.class, + () -> survivor.execute("SELECT 1"), + "a statement whose borrow an abort ended must not run on the session"); + Assertions.assertTrue(thrown.getMessage().contains("returned to the pool"), + "the failure must say what happened, but said: " + thrown.getMessage()); + Assertions.assertThrows(SQLException.class, () -> survivor.unwrap(Statement.class), + "a handle retired by an abort must not reach the driver through unwrap()"); + Assertions.assertDoesNotThrow(survivor::close, + "closing such a statement twice over stays the harmless no-op"); + + Assertions.assertTrue(cursor.isClosed(), + "a result set whose borrow an abort ended must report itself closed"); + Assertions.assertThrows(SQLException.class, cursor::next, + "a result set whose borrow an abort ended must not read the session"); + Assertions.assertDoesNotThrow(cursor::close); + } + @Test void anAbortedHandleReportsItselfClosedAndRefusesUse() throws Exception { SpecAbortDriver.ABORTS.set(0); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
