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 f6c0000da283ae0eff666a4267620093f0300a91 Author: opencode <[email protected]> AuthorDate: Fri Oct 30 22:59:30 2026 +0100 Close a cached statement quietly when its cache no longer exists reset(null,null), delivered on every connection disposal - the abandon path, pool close, connection finalization - nulls the cache counter the interceptor's cached statement handles park themselves against, but only the other entry points were guarded for the cache being gone. A statement left open on an abandoned session still reaches the application handle after disposal (an abandoned borrow is deliberately never sealed, so its handles fail only at the driver), and closing such a statement dereferenced the null counter first thing: the application got a raw NullPointerException from close() instead of the contracted outcome - a quiet close, or a driver SQLException which the decorator proxy already swallows. Snapshot the counter the way getCacheSize() already does and treat a missing cache as "cannot park": skip the parking decision and close through the normal path, which closes the delegate, swallows a driver SQLException and marks the handle closed. Behavior while the cache exists is unchanged; the guard is the same shape the sibling entry points (cacheStatement, removeStatement, getCache, forceClose) already follow, so closeInvoked() is simply no longer the one unguarded dereference. Testing: TestStatementCacheCloseAfterDisposal abandons a borrow synchronously (setTimestamp(0) plus checkAbandoned on the test thread), then closes the still-open prepared statement from the application: no exception, the driver statement reports closed, the handle reports closed and a repeat close is a no-op. Against the previous code the close fails with the null-counter NPE rethrown through the facade. --- .../jdbc/pool/interceptor/StatementCache.java | 11 ++- .../TestStatementCacheCloseAfterDisposal.java | 104 +++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) 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 73fea5da3d..a6542dfd86 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 @@ -462,9 +462,16 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta @Override public void closeInvoked() { - // Should we cache it + // Should we cache it. Read the counter once, as getCacheSize() + // does: a disposal (reset(null, null) from a finalize or from an + // abandoned or closed session) nulls the field, and a handle the + // application kept through that window must still close through + // the normal path - the sibling entry points answer a disposed + // cache quietly; this first read has to do the same rather than + // dereference the field. + AtomicInteger size = cacheSize; boolean shouldClose = true; - if (cacheSize.get() < maxCacheSize) { + if (size != null && size.get() < maxCacheSize) { // Cache a proxy so that we don't reuse the facade CachedStatement proxy = new CachedStatement(getDelegate(), getSql()); proxy.setCacheKey(getCacheKey()); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheCloseAfterDisposal.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheCloseAfterDisposal.java new file mode 100644 index 0000000000..6d7e1fb8a4 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheCloseAfterDisposal.java @@ -0,0 +1,104 @@ +/* + * 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.PreparedStatement; +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.DataSource; +import org.apache.tomcat.jdbc.pool.PoolConfiguration; +import org.apache.tomcat.jdbc.pool.PooledConnection; +import org.apache.tomcat.jdbc.pool.ProxyConnection; + +/** + * The statement facade seals its handles when a borrow is returned, but a + * borrow that is abandoned instead never seals them - the pool destroys an + * abandoned session rather than lending it again, so the javadoc of + * {@link org.apache.tomcat.jdbc.pool.StatementFacade#closeInvoked()} + * documents that those handles "only fail at the driver". The application + * handle reaching that driver is the contract this class pins: when the + * disposal resets the interceptor chain with {@code reset(null, null)}, the + * statement cache drops its binding to the pool-wide entry counter, and a + * still-open statement closed from that window must skip the recycling + * attempt and close through the normal path, delivering the driver's + * outcome - with H2 that outcome is a quiet success, because the driver + * closes an open statement of a closed connection without complaint, and an + * even later close is the contract's repeated no-op - rather than the + * pool's own {@code NullPointerException} from the unguarded counter read + * that pre-fix {@code CachedStatement.closeInvoked()} performed. + */ +class TestStatementCacheCloseAfterDisposal extends JdbcPoolBaseTest { + + @Test + void aStatementClosedAfterItsSessionWasAbandonedClosesThroughTheDriver() throws Exception { + DataSource ds = newDataSource(); + PoolConfiguration properties = ds.getPoolProperties(); + properties.setJdbcInterceptors( + StatementCache.class.getName() + "(prepared=true,max=5)"); + properties.setRemoveAbandoned(true); + properties.setRemoveAbandonedTimeout(1); // seconds + properties.setInitialSize(0); + String table = uniqueTable(); + ds.createPool(); + try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + } + + Connection abandoned = ds.getConnection(); + PreparedStatement statement = abandoned.prepareStatement( + "INSERT INTO " + table + " VALUES (?)"); + Statement physical = driverStatement(statement); + statement.setInt(1, 1); + Assertions.assertEquals(1, statement.executeUpdate()); + + PooledConnection pooled = + interceptorOf(abandoned, ProxyConnection.class).getDelegateConnection(); + StatementCache cache = interceptorOf(abandoned, StatementCache.class); + + // Backdate the borrow past the abandon timeout and run the check the + // pool's cleaner thread runs: the abandon, the disconnect purge (which + // still sees the counter bound) and the final reset of the chain all + // complete synchronously on this thread. + pooled.setTimestamp(0L); + ds.getPool().checkAbandoned(); + + Assertions.assertTrue(pooled.isReleased(), + "precondition: the pool must have released the abandoned session"); + awaitSessions(0); + Assertions.assertEquals(-1, cache.getCacheSize(), + "precondition: the disposal reset must have unbound the interceptor" + + " from the pool-wide counter"); + + // The documented window: an application handle that outlives the + // abandoned session. Pre-fix the close dies on the nulled counter + // field with a NullPointerException before ever reaching the driver. + statement.close(); + + Assertions.assertTrue(physical.isClosed(), + "the close must have reached the driver and closed its statement"); + Assertions.assertTrue(statement.isClosed(), + "the pooled handle must report itself closed afterwards"); + + // A repeated close stays the quiet no-op the JDBC contract requires. + statement.close(); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
