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 c216f9dba1c17774f1d5fbdacf3b9483544e75f0 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 10:44:32 2026 +0100 Retire statement handles when the pooled connection goes back Statements and result sets proxied by the statement facade kept executing after the borrow that created them was returned, driving the physical session of the next borrower. The pool builds one facade per borrow, so it can mark the borrow ended when close passes through it, and every handle it issued then reports itself closed and fails with an explanatory SQLException. Closing a retired handle stays a no-op and the driver's statement is not physically closed, so the statement cache still recycles statements to the next borrower. An abandoned borrow never runs the hook, but the pool destroys abandoned sessions, so those handles only fail at the driver. --- modules/jdbc-pool/doc/jdbc-pool.xml | 3 +- .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 6 +- .../apache/tomcat/jdbc/pool/StatementFacade.java | 44 ++++- .../jdbc/pool/TestStatementHandleLifetime.java | 189 +++++++++++++++++++++ 4 files changed, 234 insertions(+), 8 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index d18e0ce4b5..6b49023fd8 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -577,7 +577,8 @@ called on the closed statements if any statement proxy is set. A wrapped statement also reports the pooled connection it was created from when <code>Statement.getConnection()</code> is called, rather than - the driver's own connection. + the driver's own connection, and stops working, along with the result + sets read from it, once that connection has been returned to the pool. Default value is <code>true</code>. </p> </attribute> diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java index 3926e13fd0..345a5b0fd9 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java @@ -926,7 +926,8 @@ public interface PoolConfiguration { * Set this to true if you wish to wrap statements in order to enable equals() and hashCode() * methods to be called on the closed statements if any statement proxy is set. A wrapped * statement also reports the pooled connection it was created from rather than the driver's - * own connection. + * own connection, and stops working, along with the result sets read from it, once that + * connection has been returned to the pool. * @param useStatementFacade set to <code>true</code> to wrap statements */ void setUseStatementFacade(boolean useStatementFacade); @@ -935,7 +936,8 @@ public interface PoolConfiguration { * Returns <code>true</code> if this connection pool is configured to wrap statements in order * to enable equals() and hashCode() methods to be called on the closed statements if any * statement proxy is set. A wrapped statement also reports the pooled connection it was - * created from rather than the driver's own connection. + * created from rather than the driver's own connection, and stops working, along with the + * result sets read from it, once that connection has been returned to the pool. * @return <code>true</code> if the statements are wrapped */ boolean getUseStatementFacade(); 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 1ff14b4dc5..148b483b1b 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 @@ -38,6 +38,14 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { private static final Log logger = LogFactory.getLog(StatementFacade.class); + /** + * Whether the borrow this facade was created for has ended. The pool + * creates one facade per borrow, so this says when every handle this + * instance handed out stopped belonging to a connection the application + * still holds. + */ + private volatile boolean borrowEnded = false; + /** * Constructs a StatementFacade. * @param interceptor the next interceptor in the chain @@ -47,9 +55,17 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { setNext(interceptor); } + /** + * 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. + */ @Override public void closeInvoked() { - // nothing to do + borrowEnded = true; } /** @@ -135,19 +151,28 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { if (compare(HASHCODE_VAL, method)) { return Integer.valueOf(this.hashCode()); } + // Read the state once: whether the borrow has ended must not + // change while the call is being answered. + boolean ended = borrowEnded; if (compare(CLOSE_VAL, method)) { - if (delegate == null) { + if (delegate == null || ended) { return null; } } if (compare(ISCLOSED_VAL, method)) { - if (delegate == null) { + if (delegate == null || ended) { return Boolean.TRUE; } } if (delegate == null) { throw new SQLException("Statement closed."); } + if (ended) { + // The delegate is still open, but it is open on a session + // that belongs to the next borrower now. + throw new SQLException("Statement closed. The pooled connection this " + + "statement was created from has been returned to the pool."); + } // The connection a statement reports is the pooled handle the // application created it from, not the driver's own connection. // Forwarding the call instead hands the application a handle on @@ -248,19 +273,28 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { if (compare(HASHCODE_VAL, method)) { return Integer.valueOf(this.hashCode()); } + // Read the state once: whether the borrow has ended must not + // change while the call is being answered. + boolean ended = borrowEnded; if (compare(CLOSE_VAL, method)) { - if (delegate == null) { + if (delegate == null || ended) { return null; } } if (compare(ISCLOSED_VAL, method)) { - if (delegate == null) { + if (delegate == null || ended) { return Boolean.TRUE; } } if (delegate == null) { throw new SQLException("ResultSet closed."); } + if (ended) { + // The cursor is still open, but it is open on a session that + // belongs to the next borrower now. + throw new SQLException("ResultSet closed. The pooled connection this " + + "result set was read from has been returned to the pool."); + } if (compare(GET_STATEMENT, method)) { return parent; diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementHandleLifetime.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementHandleLifetime.java new file mode 100644 index 0000000000..4f252ee529 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementHandleLifetime.java @@ -0,0 +1,189 @@ +/* + * 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; + +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.interceptor.StatementCache; + +/** + * How long the statement and result set handles the statement facade hands + * out stay good. + * + * <p>The pool creates one facade per borrow, so the facade knows exactly when + * its borrow ended. A handle that outlives it must not go on working: the + * physical session it runs on has gone back to the pool and is somebody + * else's by then, so continuing to execute on it means two components sharing + * one session, one transaction and any server-side cursor the first of them + * left open.</p> + */ +class TestStatementHandleLifetime extends JdbcPoolBaseTest { + + /** Borrows one connection, uses a statement and returns it. */ + private void useConnection(DataSource ds, int marker) throws SQLException { + try (Connection con = ds.getConnection(); + Statement st = con.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1")) { + setSessionMarker(con, marker); + Assertions.assertTrue(rs.next()); + } + } + + @Test + void aStatementDoesNotOutliveTheBorrowThatCreatedIt() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.createPool(); + + Connection first = ds.getConnection(); + Statement survivor = first.createStatement(); + Assertions.assertFalse(survivor.isClosed()); + first.close(); + + // The session is in use again by someone else. + Connection second = ds.getConnection(); + setSessionMarker(second, 5150); + + Assertions.assertTrue(survivor.isClosed(), + "a statement whose borrow ended must report itself closed"); + SQLException thrown = Assertions.assertThrows(SQLException.class, + () -> survivor.execute("INSERT INTO NONEXISTENT_TABLE_YET VALUES (1)"), + "a statement whose borrow ended must not run against the next borrower's session"); + Assertions.assertTrue(thrown.getMessage().contains("returned to the pool"), + "the failure must say what happened, but said: " + thrown.getMessage()); + // The driver's own statement stays out of reach even through + // unwrap(): the physical statement is still open, so only this + // guard stands between a retired handle and the next borrower's + // session. + Assertions.assertThrows(SQLException.class, + () -> survivor.unwrap(Statement.class), + "a retired handle must not reach the driver through unwrap()"); + + // Closing such a statement twice over stays the harmless no-op that a + // closed statement's close() is. + survivor.close(); + survivor.close(); + Assertions.assertEquals(Integer.valueOf(5150), sessionMarker(second), + "the second borrower's session must be untouched"); + second.close(); + } + + @Test + void aResultSetDoesNotOutliveTheBorrowItWasReadFrom() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.createPool(); + + Connection first = ds.getConnection(); + Statement st = first.createStatement(); + ResultSet survivor = st.executeQuery("SELECT 1"); + Assertions.assertTrue(survivor.next(), "the cursor is live during the borrow"); + first.close(); + + Assertions.assertTrue(survivor.isClosed(), + "a result set whose borrow ended must report itself closed"); + SQLException thrown = Assertions.assertThrows(SQLException.class, survivor::next, + "a result set whose borrow ended must not read the next borrower's session"); + Assertions.assertTrue(thrown.getMessage().contains("returned to the pool"), + "the failure must say what happened, but said: " + thrown.getMessage()); + survivor.close(); + + useConnection(ds, 5150); + } + + /** + * Invalidation belongs to the borrow, not to the physical connection: the + * next borrower must get working handles over the very same session. + */ + @Test + void theNextBorrowerGetsWorkingHandlesOverTheSameSession() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.createPool(); + + Connection first = ds.getConnection(); + Statement retired = first.createStatement(); + setSessionMarker(first, 4242); + first.close(); + + try (Connection second = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(4242), sessionMarker(second), + "the next borrower holds the same physical session"); + try (Statement st = second.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next(), "the new borrower's statement must work"); + } + Assertions.assertTrue(retired.isClosed(), + "while the previous borrow's handle stays retired"); + } + } + + /** + * The statement cache is the one case where a statement is deliberately + * handed to the next borrower, and the facade must not disturb it: the + * cache hands its own handle to each borrow and the facade wraps it + * afresh, so a cached statement arrives working. + */ + @Test + void aCachedStatementArrivesWorkingForTheNextBorrower() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.getPoolProperties().setJdbcInterceptors( + StatementCache.class.getName() + "(prepared=true,callable=false,max=10)"); + ds.createPool(); + + PreparedStatement retired; + Connection first = ds.getConnection(); + retired = first.prepareStatement("SELECT ?"); + // captured while the handle is still live, since a retired handle no + // longer reaches the driver at all + Statement cached = driverStatement(retired); + retired.setInt(1, 1); + try (ResultSet rs = retired.executeQuery()) { + Assertions.assertTrue(rs.next()); + } + retired.close(); // goes into the cache + first.close(); + + try (Connection second = ds.getConnection()) { + PreparedStatement recycled = second.prepareStatement("SELECT ?"); + Assertions.assertSame(cached, driverStatement(recycled), + "the cached statement must be the one the second borrower got"); + recycled.setInt(1, 2); + try (ResultSet rs = recycled.executeQuery()) { + Assertions.assertTrue(rs.next()); + Assertions.assertEquals(2, rs.getInt(1), + "the recycled statement must answer with this borrower's parameters"); + } + Assertions.assertTrue(retired.isClosed(), + "and the handle the first borrower closed must stay closed"); + recycled.close(); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
