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 c6824fc3394ea4ba6011fc2b4b731f6ebc451bc0 Author: opencode <[email protected]> AuthorDate: Fri Nov 6 10:44:41 2026 +0100 Do not hand out a cache entry that lost the removal race A cache hit discarded the result of removeStatement(), so it could return an entry a concurrent disconnect purge had already taken and force-closed. Reuse only an entry this call actually removed, and make the close path tolerate a delegate already cleared by a concurrent force-close. --- .../jdbc/pool/interceptor/StatementCache.java | 40 ++++--- .../TestStatementCacheLostRemovalRace.java | 132 +++++++++++++++++++++ 2 files changed, 155 insertions(+), 17 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 7e1ac210c5..b867d828db 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 @@ -380,9 +380,7 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta boolean process = process(this.types, method, false); if (process && args.length > 0 && args[0] instanceof String) { CachedStatement statement = isCached(method, args); - if (statement != null) { - // Remove it from the cache since it is used - removeStatement(statement); + if (statement != null && removeStatement(statement)) { if (statement.getAssociatedConnection() != currentPhysicalConnection()) { // The entry belongs to a connection that has been // replaced (a statement close that raced a reconnect @@ -399,9 +397,11 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta // reports a live handle instead of the previous borrower's. statement.setConnection(proxy); return statement.getActualProxy(); - } else { - return super.invoke(proxy, method, args); } + // No entry, or another thread won the removal (a disconnect purge + // closed it): prepare a fresh statement rather than hand out an + // entry this call does not own. + return super.invoke(proxy, method, args); } else { return super.invoke(proxy, method, args); } @@ -759,6 +759,12 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta // same way rather than dereference the field. AtomicInteger size = cacheSize; boolean shouldClose = true; + PreparedStatement delegate = getDelegate(); + if (delegate == null) { + // A concurrent forceClose() already retired this handle; there + // is nothing left to reset or park. + return; + } if (size != null && size.get() < maxCacheSize && !trackerBelow()) { // A tracker beneath this cache (see trackerBelow) would close a // parked statement at the connection return, so the park is @@ -770,34 +776,34 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta // that second question too, while the prepare-time binding // still names the session the statement was prepared on for // the session check in cacheStatement. - CachedStatement proxy = new CachedStatement(getDelegate(), getSql(), associatedConnection); + CachedStatement proxy = new CachedStatement(delegate, getSql(), associatedConnection); proxy.setCacheKey(getCacheKey()); try { // Only cache if statement has been marked as poolable and not close on completion - if (poolable && !getDelegate().isCloseOnCompletion()) { + if (poolable && !delegate.isCloseOnCompletion()) { // Clear Resultset - ResultSet result = getDelegate().getResultSet(); + ResultSet result = delegate.getResultSet(); if (result != null && !result.isClosed()) { result.close(); } // Clear parameter - getDelegate().clearParameters(); + delegate.clearParameters(); // Clear batch - getDelegate().clearBatch(); + delegate.clearBatch(); // Clear warnings - getDelegate().clearWarnings(); + delegate.clearWarnings(); // Reset the timeout - getDelegate().setQueryTimeout(0); + delegate.setQueryTimeout(0); // Reset fetch direction - getDelegate().setFetchDirection(ResultSet.FETCH_FORWARD); + delegate.setFetchDirection(ResultSet.FETCH_FORWARD); // Reset max field size - getDelegate().setMaxFieldSize(0); + delegate.setMaxFieldSize(0); // Reset max rows - getDelegate().setMaxRows(0); + delegate.setMaxRows(0); // Reset fetchsize - getDelegate().setFetchSize(0); + delegate.setFetchSize(0); // Reset the escape processing - getDelegate().setEscapeProcessing(true); + delegate.setEscapeProcessing(true); // Create a new facade Object actualProxy = getConstructor().newInstance(new Object[] { proxy }); proxy.setActualProxy(actualProxy); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheLostRemovalRace.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheLostRemovalRace.java new file mode 100644 index 0000000000..6c32b2a6b5 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheLostRemovalRace.java @@ -0,0 +1,132 @@ +/* + * 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.lang.reflect.Method; +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; + +/** + * A cache hit that loses the removal race to a disconnect purge must not hand + * out the entry the purge is closing, and a concurrent force-close must not + * leave the application close path dereferencing a null delegate. + */ +class TestStatementCacheLostRemovalRace extends JdbcPoolBaseTest { + + public static class StaleHitStatementCache extends StatementCache { + + private CachedStatement forcedHit; + + void forceNextHit(CachedStatement stale) { + forcedHit = stale; + } + + CachedStatement parked(Method method, Object[] args) { + return super.isCached(method, args); + } + + @Override + public CachedStatement isCached(Method method, Object[] args) { + CachedStatement stale = forcedHit; + if (stale != null) { + forcedHit = null; + return stale; + } + return super.isCached(method, args); + } + } + + private static final String SQL = "SELECT 1"; + + private static Method prepareMethod() throws Exception { + return Connection.class.getMethod("prepareStatement", String.class); + } + + private DataSource newCachingDataSource() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + StaleHitStatementCache.class.getName() + "(prepared=true,max=5)"); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(60000); + ds.createPool(); + return ds; + } + + @Test + void aHitThatLostTheRemovalRaceIsNotHandedOut() throws Exception { + DataSource ds = newCachingDataSource(); + try (Connection connection = ds.getConnection()) { + StaleHitStatementCache cache = interceptorOf(connection, StaleHitStatementCache.class); + Method prepare = prepareMethod(); + Object[] args = new Object[] { SQL }; + + PreparedStatement first = connection.prepareStatement(SQL); + first.close(); + StatementCache.CachedStatement stale = cache.parked(prepare, args); + Assertions.assertNotNull(stale, "precondition: the statement must be parked"); + Statement staleDriver = driverStatement((Statement) stale.getActualProxy()); + + // Simulate the purge winning the removal race: the entry is gone + // before the hit tries to take it. + Assertions.assertTrue(cache.removeStatement(stale), + "precondition: the test must own the removal"); + + cache.forceNextHit(stale); + PreparedStatement fresh = connection.prepareStatement(SQL); + try { + Assertions.assertNotSame(stale.getActualProxy(), fresh, + "a hit that did not win the removal must not hand out the entry"); + Assertions.assertFalse(staleDriver.isClosed(), + "the refused hit must not close an entry it did not own"); + Assertions.assertTrue(fresh.execute()); + } finally { + fresh.close(); + stale.forceClose(); + } + } finally { + ds.close(); + } + } + + @Test + void aCloseAfterAConcurrentForceCloseDoesNotDereferenceANullDelegate() throws Exception { + DataSource ds = newCachingDataSource(); + try (Connection connection = ds.getConnection()) { + StaleHitStatementCache cache = interceptorOf(connection, StaleHitStatementCache.class); + Method prepare = prepareMethod(); + Object[] args = new Object[] { SQL }; + + PreparedStatement statement = connection.prepareStatement(SQL); + statement.close(); + StatementCache.CachedStatement stale = cache.parked(prepare, args); + Assertions.assertNotNull(stale, "precondition: the statement must be parked"); + + stale.forceClose(); + Assertions.assertDoesNotThrow(stale::closeInvoked, + "closing a handle a concurrent force-close already retired must not throw"); + } finally { + ds.close(); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
