This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 391402c2259365d8effb0f9515347ea5cf6a392d Author: opencode <[email protected]> AuthorDate: Fri Oct 30 23:31:47 2026 +0100 Bind a cached statement to the session the prepare ran on StatementCache records the physical connection each cached statement was prepared on so a statement whose session has since been replaced is never parked nor handed out. It recorded that binding by asking the statement itself, but an interceptor listed after the cache wraps statements beneath it and answers their getConnection() with the pool's handle for the borrow instead of forwarding, so the recorded value was a handle, never the session, every park was refused on the session check, every closed statement was closed instead of cached and the next prepare prepared again -- silently, while the class javadoc promised wrapping-only interceptors after the cache were unaffected. The binding now starts from the driver's own answer only where that answer is readable as the current physical connection; when it is one of the pool's own handles it resolves through the handle to the session sitting behind it, on which this very prepare ran; anything else stays trusted, so a driver honestly naming a replaced session still refuses the park. The park at close carries the prepare-time binding down to the cached proxy rather than asking the delegate again, so a lower wrapper cannot perturb it and a session replacement between prepare and park is still refused. A park refused for a lost association is now noted at debug level at most once per connection, so a cache configured but silently never caching costs one log line. --- .../jdbc/pool/interceptor/StatementCache.java | 113 ++++++++- .../TestStatementCacheInterceptorOrder.java | 257 +++++++++++++++++++++ webapps/docs/changelog.xml | 17 ++ 3 files changed, 378 insertions(+), 9 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 57473e6e2f..0009cc8a64 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 @@ -18,6 +18,7 @@ package org.apache.tomcat.jdbc.pool.interceptor; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.lang.reflect.Proxy; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; @@ -25,6 +26,7 @@ import java.sql.SQLException; import java.util.Arrays; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import javax.management.ObjectName; @@ -32,6 +34,7 @@ import javax.management.ObjectName; import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; import org.apache.tomcat.jdbc.pool.ConnectionPool; +import org.apache.tomcat.jdbc.pool.JdbcInterceptor; import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorProperty; import org.apache.tomcat.jdbc.pool.PooledConnection; import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; @@ -44,8 +47,9 @@ import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; * listed <em>after</em> this one in {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#setJdbcInterceptors(String) * jdbcInterceptors} therefore do not see reused statements - relevant to any interceptor whose {@code createStatement} * hook performs an action rather than only wrapping the statement: {@link QueryTimeoutInterceptor} must be listed - * before this one to keep its timeout on cached statements, while wrapping-only interceptors are unaffected because - * their wrapper is carried inside the cached proxy. + * before this one to keep its timeout on cached statements, while wrapping-only interceptors are unaffected: their + * wrapper is carried inside the cached proxy, and the cache binds a cached statement to the physical session the + * prepare ran on rather than to whatever connection a wrapper beneath it reports for the statement. */ public class StatementCache extends StatementDecoratorInterceptor implements StatementCacheMBean { private static final Log log = LogFactory.getLog(StatementCache.class); @@ -308,6 +312,7 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta // disconnect/reconnect, landing after the disconnected() purge // had already run). The statement belongs to a dead connection // and must not be parked in the cache of the new one. + logLostAssociationOnce(proxy); return false; } else if (cacheSize.get() >= maxCacheSize) { return false; @@ -375,6 +380,54 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta return pCon == null ? null : pCon.getConnection(); } + /** + * Whether a reported connection is one of the handles the pool itself hands out - a proxy whose invocation + * handler is a pool interceptor - rather than a physical connection. Only the pool's own handles are read this + * way; an answer from anything else stays the statement's reported binding, so a driver that honestly names a + * replaced (dead) session keeps refusing the park. + * + * @param reported the value reported as a statement's connection + * + * @return true if the value is one of the pool's own connection handles + */ + private static boolean isPoolHandle(Connection reported) { + return reported != null && Proxy.isProxyClass(reported.getClass()) + && Proxy.getInvocationHandler(reported) instanceof JdbcInterceptor; + } + + /** + * Set once the refusal of a park for a lost association has been logged for the pooled connection this + * interceptor instance is bound to (the pool builds one chain, and so one interceptor instance, per pooled + * connection), so the note appears once per connection and not once per refused statement. The claim is a + * compare-and-set run under the debug guard, so of two refusals on concurrent closes exactly one logs, and + * a refusal met while debug is off leaves the claim unspent for a later refusal to make after debug is + * enabled. + */ + private final AtomicBoolean associationLossLogged = new AtomicBoolean(); + + /** + * Logs, at most once per connection, that a park was refused because the connection recorded for the statement + * is not the current physical one. The usual cause is benign - a close racing a session replacement - but a + * wrapper beneath this cache that answers the statement's {@code getConnection()} with a value this cache + * cannot resolve to a physical session reaches this refusal for every statement it closes, and a cache that + * never caches is worth one line of log. + * + * @param proxy the statement whose park was refused + */ + private void logLostAssociationOnce(CachedStatement proxy) { + // The debug guard runs first, so a refusal met while debug is off does + // not spend the claim, and the compare-and-set runs second, so of the + // refusals met while debug is on exactly one logs. + if (log.isDebugEnabled() && associationLossLogged.compareAndSet(false, true)) { + log.debug("Not caching statement [" + proxy.getSql() + "]: the connection recorded for it is " + + "not the current physical connection. Either the session was replaced while the " + + "statement was being closed, or an interceptor listed below " + + getClass().getSimpleName() + " answers the statement's getConnection() with a value " + + "that cannot be resolved to the physical session, which keeps every statement out " + + "of the cache."); + } + } + /** * Returns the statement cache for the current pooled connection. * @@ -424,7 +477,9 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta private boolean poolable = true; /** - * Constructs a CachedStatement. + * Constructs a CachedStatement, recording the physical connection the statement was prepared on as the + * answer of the statement itself (or, when that answer is not a physical connection the cache can read, of + * the pool's own record of the session the prepare ran on). * * @param parent the parent prepared statement * @param sql the SQL text @@ -435,9 +490,29 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta } /** - * Resolves the physical connection a statement belongs to. The connection the driver reports for the statement - * itself is authoritative; only when the driver will not answer does the pool's own record of the current - * connection serve as the binding. + * Constructs a CachedStatement with an already determined binding to the physical connection the statement + * was prepared on. {@link #closeInvoked()} parks through this constructor, carrying the binding that was + * made when the statement was prepared down to the proxy that enters the cache: asking the delegate again + * at park time would hand a wrapper beneath this cache another chance to answer with something that is not + * the physical session, while the binding made at prepare time names the session the statement was really + * prepared on and still lets a session replacement between the prepare and the park refuse it. + * + * @param parent the parent prepared statement + * @param sql the SQL text + * @param associatedConnection the physical connection the statement was prepared on + */ + CachedStatement(PreparedStatement parent, String sql, Connection associatedConnection) { + super(parent, sql); + this.associatedConnection = associatedConnection; + } + + /** + * Resolves the physical connection a statement belongs to. The connection the driver reports for the + * statement itself is authoritative; a wrapper listed beneath this cache may however intercept the call and + * answer it with a pool handle rather than forward it to the driver, and a handle names no session of its + * own - the physical session that sits behind the handle now, and behind the prepare of this very + * statement, serves as the binding for that answer. Only when the driver will not answer does the pool's + * own record of the current connection serve as the binding. * * @param statement the statement whose associated connection to determine * @@ -445,7 +520,21 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta */ private Connection associatedConnectionOf(PreparedStatement statement) { try { - return statement.getConnection(); + Connection reported = statement.getConnection(); + if (reported == currentPhysicalConnection()) { + return reported; + } + if (isPoolHandle(reported)) { + // A wrapper beneath the cache intercepted the call (the + // StatementDecoratorInterceptor answers it with the + // handle of the borrow, among others). The handle is + // this pooled connection's own, and the current borrow + // - on whose session this prepare ran - sits on its + // current physical connection: that session is the + // statement's binding. + return currentPhysicalConnection(); + } + return reported; } catch (SQLException x) { return currentPhysicalConnection(); } @@ -472,8 +561,14 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta AtomicInteger size = cacheSize; boolean shouldClose = true; if (size != null && size.get() < maxCacheSize) { - // Cache a proxy so that we don't reuse the facade - CachedStatement proxy = new CachedStatement(getDelegate(), getSql()); + // Cache a proxy so that we don't reuse the facade. The + // binding to the physical session made at the prepare is + // carried over to the parked proxy rather than the delegate + // asked again: a wrapper beneath this cache would intercept + // 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); proxy.setCacheKey(getCacheKey()); try { // Only cache if statement has been marked as poolable and not close on completion diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheInterceptorOrder.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheInterceptorOrder.java new file mode 100644 index 0000000000..62cd4868ea --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheInterceptorOrder.java @@ -0,0 +1,257 @@ +/* + * 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.PooledConnection; + +/** + * The documented order independence of the statement cache: a wrapping + * interceptor listed <em>after</em> the cache sits beneath it in the chain, + * and the cache then prepares its entry on that interceptor's proxy rather + * than on the driver's statement. The proxy answers {@code getConnection()} + * with the pooled handle of the borrow instead of forwarding the call to the + * driver, so if the cache took that answer as the statement's physical + * connection it would never match the session and every park would be + * refused - the cache silently degrades to closing every closed statement, + * exactly what {@code StatementCache}'s class documentation says does not + * happen ("wrapping-only interceptors are unaffected"). These tests pin both + * orderings: the cache must park and reuse through a wrapper beneath it, a + * wrapper above it must keep working as before, and the protection against + * parking a statement of a replaced session (which the driver's own answer + * carries) must stay intact on both routes. + */ +class TestStatementCacheInterceptorOrder extends JdbcPoolBaseTest { + + private static final String CACHE_CONFIG = + StatementCache.class.getName() + "(prepared=true,max=5)"; + private static final String WRAPPER_CONFIG = + StatementDecoratorInterceptor.class.getName(); + private static final String WRAPPER_BEFORE_CACHE = + WRAPPER_CONFIG + ";" + CACHE_CONFIG; + + /** The statement cache as the given borrower sees it. */ + private static StatementCache cacheOf(Connection pooled) { + return interceptorOf(pooled, StatementCache.class); + } + + /** + * The defect: with the wrapper configured beneath the cache, closing one + * prepared statement must still park it - the same park, the same reuse + * and the same driver statement as with the cache alone (compare + * {@link TestStatementCacheReuse}). Pre-fix the park was refused because + * the connection recorded for the statement was the decorator's pooled + * handle, never the physical session, so the cache size stayed at + * {@code 0}, the driver statement came back closed, and the next prepare + * of the same SQL was a real second prepare. + */ + @Test + void aWrappingInterceptorListedAfterTheCacheDoesNotStopTheCaching() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(CACHE_CONFIG + ";" + WRAPPER_CONFIG); + ds.getPoolProperties().setMaxActive(1); + 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(); + // The configuration order must really put the wrapper beneath the + // cache, otherwise this test pins nothing about the ordering. + StatementCache cache = cacheOf(first); + Assertions.assertTrue(cache.getNext() instanceof StatementDecoratorInterceptor, + "an interceptor listed after the cache must sit beneath it in the chain"); + setSessionMarker(first, 41); + PreparedStatement prepared = first.prepareStatement(insert); + Statement physical = driverStatement(prepared); + prepared.setInt(1, 1); + Assertions.assertEquals(1, prepared.executeUpdate()); + prepared.close(); + Assertions.assertEquals(1, cache.getCacheSizePerConnection(), + "a wrapper beneath the cache must not stop the statement from " + + "being parked (pre-fix the park was refused and the " + + "cache stayed empty)"); + Assertions.assertFalse(physical.isClosed(), + "the parked statement must stay open - that is what saves the prepare"); + first.close(); + + Connection second = ds.getConnection(); + try { + Assertions.assertEquals(Integer.valueOf(41), sessionMarker(second), + "the same physical session must have been reused"); + Assertions.assertEquals(1, cacheOf(second).getCacheSizePerConnection(), + "the cache must survive the return to the pool"); + + PreparedStatement reused = second.prepareStatement(insert); + Assertions.assertSame(physical, driverStatement(reused), + "the next borrower must get the same physical statement, not a " + + "second prepare on the session"); + Assertions.assertSame(second, reused.getConnection(), + "the wrapper's own handle must not leak out as the statement's " + + "connection on reuse"); + reused.setInt(1, 2); + Assertions.assertEquals(1, reused.executeUpdate()); + reused.close(); + Assertions.assertEquals(1, cacheOf(second).getCacheSizePerConnection(), + "the statement must go back into the cache when closed again"); + } finally { + second.close(); + } + Assertions.assertEquals(2, adminRowCount(table), + "both executes must have reached the database"); + } + + /** + * The reverse order, which worked before the defect was addressed: the + * wrapper above the cache sees every prepare (its proxy wraps even a + * statement served from the cache) and the cache prepares directly on + * the driver's statement, whose connection answer is the physical + * session. This must keep park-and-reusing, so a fix aimed at the other + * ordering cannot quietly change this one. + */ + @Test + void aWrappingInterceptorListedBeforeTheCacheKeepsWorkingAsBefore() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(WRAPPER_BEFORE_CACHE); + ds.getPoolProperties().setMaxActive(1); + 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(); + StatementCache cache = cacheOf(first); + Assertions.assertFalse(cache.getNext() instanceof StatementDecoratorInterceptor, + "an interceptor listed before the cache must not sit beneath it"); + setSessionMarker(first, 42); + PreparedStatement prepared = first.prepareStatement(insert); + Statement physical = driverStatement(prepared); + prepared.setInt(1, 1); + Assertions.assertEquals(1, prepared.executeUpdate()); + prepared.close(); + Assertions.assertEquals(1, cache.getCacheSizePerConnection(), + "the wrapper above the cache must not keep the statement out of the cache"); + Assertions.assertFalse(physical.isClosed(), + "the parked statement must stay open"); + first.close(); + + Connection second = ds.getConnection(); + try { + Assertions.assertEquals(Integer.valueOf(42), sessionMarker(second), + "the same physical session must have been reused"); + PreparedStatement reused = second.prepareStatement(insert); + Assertions.assertSame(physical, driverStatement(reused), + "the next borrower must get the same physical statement"); + reused.setInt(1, 2); + Assertions.assertEquals(1, reused.executeUpdate()); + reused.close(); + Assertions.assertEquals(1, cacheOf(second).getCacheSizePerConnection(), + "the statement must go back into the cache when closed again"); + } finally { + second.close(); + } + Assertions.assertEquals(2, adminRowCount(table), + "both executes must have reached the database"); + } + + /** + * The protection the driver's answer carries must survive the fix on the + * route where the answer is intercepted: a statement close racing a + * session replacement must still not be parked in the new session's + * cache. Same deterministic race as + * {@link TestStatementCacheRacingClose} (the reconnect runs inside + * {@code cacheStatement}), but with the wrapping interceptor beneath the + * cache, so the statement's own {@code getConnection()} can never answer + * the old session - a fix that resolved the intercepted answer from the + * session current at park time would park the dead statement here. + */ + @Test + void aStatementClosedWhileTheSessionIsReplacedIsStillNotParkedWithAWrapperBelow() + throws Exception { + DataSource ds = newDataSource(); + String racingCache = + TestStatementCacheRacingClose.RacingCloseStatementCache.class.getName(); + ds.getPoolProperties().setJdbcInterceptors( + racingCache + "(prepared=true,max=5);" + WRAPPER_CONFIG); + ds.getPoolProperties().setMaxActive(1); + // keep the background sweeper out of the way: the racing close + // performs the reconnect itself here + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(60000); + String racingTable = uniqueTable(); + ds.createPool(); + try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + racingTable + " (v INT)"); + } + String racingSql = "INSERT INTO " + racingTable + " VALUES (?)"; + + Connection first = ds.getConnection(); + PooledConnection pcon = (PooledConnection) first.unwrap(PooledConnection.class); + TestStatementCacheRacingClose.RacingCloseStatementCache cache = + interceptorOf(first, TestStatementCacheRacingClose.RacingCloseStatementCache.class); + setSessionMarker(first, 43); + + Connection oldPhysical = pcon.getConnection(); + PreparedStatement racing = first.prepareStatement(racingSql); + Statement racingDriverStatement = driverStatement(racing); + racing.setInt(1, 2); + Assertions.assertEquals(1, racing.executeUpdate()); + + cache.raceNextParkWithReconnect(pcon); + racing.close(); + + Assertions.assertTrue(oldPhysical.isClosed(), + "the racing reconnect must have replaced the physical session"); + awaitSessions(1); + Assertions.assertEquals(0, cache.getCacheSizePerConnection(), + "the statement of the replaced session must not be parked through " + + "the intercepted connection answer"); + Assertions.assertTrue(racingDriverStatement.isClosed(), + "the refused statement must have been closed by its close"); + + first.close(); // back to the pool on the new session + Connection second = ds.getConnection(); + try { + Assertions.assertNull(sessionMarker(second), + "the next borrower must sit on the new physical session"); + PreparedStatement fresh = second.prepareStatement(racingSql); + Assertions.assertNotSame(racingDriverStatement, driverStatement(fresh), + "the statement of the replaced session must not be served"); + fresh.setInt(1, 3); + Assertions.assertEquals(1, fresh.executeUpdate(), + "the next borrower must get a statement that works on its session"); + fresh.close(); + Assertions.assertEquals(1, cache.getCacheSizePerConnection(), + "the new session must be able to park its own statement"); + } finally { + second.close(); + } + Assertions.assertEquals(2, adminRowCount(racingTable), + "both executes of the racing SQL must have reached the database"); + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 9f276515ef..d927909471 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1439,6 +1439,23 @@ closed, <code>abort()</code> is the no-op the specification prescribes. Based on code submitted by Arturo Bernal. (remm) </fix> + <fix> + Correct a defect in the <code>StatementCache</code> interceptor + where an interceptor listed after the cache that wraps statements + beneath it - <code>StatementDecoratorInterceptor</code>, for + example - silently stopped the cache from caching. The cached + statement recorded as its connection whatever that lower wrapper + answered for the statement's <code>getConnection()</code> - the + pool's handle for the borrow, not the physical connection - so no + closed statement ever passed the session check on the way into the + cache, every one was closed instead, and the next prepare prepared + again, at any log level. The cache now binds a cached statement to + the physical session the prepare ran on, using the driver's own + answer where it is readable, while a statement prepared on a + session that has since been replaced still refuses both caching + and reuse, and a park refused for a lost association is now noted + once per connection at debug level. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
