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 f9d5df3449e41444e0971be8a8d59e7a3049e22d Author: opencode <[email protected]> AuthorDate: Tue Oct 27 20:50:23 2026 +0100 Bind a reused cached statement to the borrower it is handed to A statement closed by a borrower was recycled into the StatementCache carrying that borrower's connection handle, and the cache-hit path handed the proxy to the next borrower without rebinding it. Since the disposable facade is unconditional and is sealed on close, the stored handle is a permanently dead object: Statement.getConnection() on a reused statement answered with a connection that reports every call as made after the connection was already returned, so a framework unwrapping through the statement received a dead connection as soon as the statement had been through the cache once. With the statement facade on (the default) the facade answers getConnection() from the current borrow and masks the stale binding; the leak is user-visible with useStatementFacade=false. The cache-hit path in StatementCache.invoke() now rebinds the cached proxy to the connection handle of the borrow it is handed to before returning it. Add regression coverage in TestStatementCacheReuse: the facade-off test fails on the pre-fix code with the disposed facade of the previous borrower, and the facade-on test pins the current-borrow answer behind the default facade. --- .../jdbc/pool/interceptor/StatementCache.java | 6 ++ .../pool/interceptor/TestStatementCacheReuse.java | 98 +++++++++++++++++++++- 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementCache.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementCache.java index 7e9747ef6e..c50016c413 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementCache.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementCache.java @@ -249,6 +249,12 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta if (statement!=null) { //remove it from the cache since it is used removeStatement(statement); + // The cached proxy carries the connection handle of the + // borrower that closed the statement, which by now is a + // disposed facade. Re-bind it to the borrower the proxy is + // handed to, so that getConnection() on the reused statement + // reports a live handle instead of the previous borrower's. + statement.setConnection(proxy); return statement.getActualProxy(); } else { return super.invoke(proxy, method, args); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java index 1941f6c393..0eda0d3a2f 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java @@ -42,7 +42,9 @@ import org.apache.tomcat.jdbc.pool.DataSource; * <p>The rules asserted below are the documented ones: statements are * cached per connection, the {@code max} count is counted over the whole * pool, and once that count is reached further statements are not cached - * and are closed immediately.</p> + * and are closed immediately. A reused statement must also report the + * connection of the borrow it is handed to, not the one its previous + * borrower closed it with.</p> */ class TestStatementCacheReuse extends JdbcPoolBaseTest { @@ -106,6 +108,100 @@ class TestStatementCacheReuse extends JdbcPoolBaseTest { "both executes must have reached the database"); } + /** + * The connection a recycled statement reports is the one it is handed + * out on, not the one its previous borrower closed it with: that handle + * stops working the moment its borrow ends, and a framework unwrapping + * through {@code Statement.getConnection()} would receive a connection + * that answers every call as closed. + */ + @Test + void aRecycledStatementReportsTheBorrowerItIsHandedTo() throws Exception { + DataSource ds = cachedPool("prepared=true,max=5", 1); + // Without the statement facade the cache's own proxy is what the + // application holds; the facade answers getConnection() itself and + // so masks what the proxy behind it carries. + ds.getPoolProperties().setUseStatementFacade(false); + String table = uniqueTable(); + ds.createPool(); + try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + } + String insert = "INSERT INTO " + table + " VALUES (?)"; + + Connection first = ds.getConnection(); + setSessionMarker(first, 31); + PreparedStatement prepared = first.prepareStatement(insert); + Assertions.assertSame(first, prepared.getConnection(), + "a freshly prepared statement reports the handle that prepared it"); + prepared.setInt(1, 1); + Assertions.assertEquals(1, prepared.executeUpdate()); + prepared.close(); + Assertions.assertEquals(1, cacheOf(first).getCacheSizePerConnection(), + "the statement must be waiting in the cache"); + first.close(); + + Connection second = ds.getConnection(); + try { + Assertions.assertEquals(Integer.valueOf(31), sessionMarker(second), + "the same physical session must have been reused"); + PreparedStatement reused = second.prepareStatement(insert); + Assertions.assertEquals(0, cacheOf(second).getCacheSizePerConnection(), + "the statement must have come out of the cache"); + + Connection reported = reused.getConnection(); + Assertions.assertSame(second, reported, + "the recycled statement must report the borrower it is handed to, " + + "not the borrower that closed it"); + Assertions.assertFalse(reported.isClosed(), + "the reported connection must be the live handle of the current borrow"); + try (Statement probe = reported.createStatement()) { + Assertions.assertTrue(probe.execute("SELECT 1"), + "the reported connection must serve the current borrower"); + } + + reused.setInt(1, 2); + Assertions.assertEquals(1, reused.executeUpdate()); + reused.close(); + } finally { + second.close(); + } + Assertions.assertEquals(2, adminRowCount(table), + "both executes must have reached the database"); + } + + /** + * The same contract with the default statement facade on: the facade + * answers {@code getConnection()} from the current borrow, so the answer + * must not depend on which borrower originally prepared the statement. + */ + @Test + void aRecycledStatementBehindTheDefaultFacadeReportsTheCurrentBorrower() throws Exception { + DataSource ds = cachedPool("prepared=true,max=5", 1); + ds.createPool(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 32); + PreparedStatement prepared = first.prepareStatement("SELECT 1"); + prepared.close(); + Assertions.assertEquals(1, cacheOf(first).getCacheSizePerConnection()); + first.close(); + + Connection second = ds.getConnection(); + try { + Assertions.assertEquals(Integer.valueOf(32), sessionMarker(second), + "the same physical session must have been reused"); + try (PreparedStatement reused = second.prepareStatement("SELECT 1")) { + Assertions.assertEquals(0, cacheOf(second).getCacheSizePerConnection(), + "the statement must have come out of the cache"); + Assertions.assertSame(second, reused.getConnection(), + "the recycled statement must report the current handle"); + } + } finally { + second.close(); + } + } + @Test void eachSessionCachesItsOwnStatements() throws Exception { DataSource ds = cachedPool("prepared=true,max=5", 2); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
