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 a4096f6703244b71b83d194d71abc23ae14b5de5 Author: remm <[email protected]> AuthorDate: Sat Oct 31 23:38:29 2026 +0100 Guard the statement cache against a finalizer beneath it - StatementCache now refuses to park statements at all when a StatementFinalizer sits beneath it in the built chain: in that order the finalizer tracks the driver statement while the cache swallows the close, so the return-time reclaim closed parked statements and the next borrower's cache hit threw on any use - one failing victim per cached SQL per recycle cycle. The pool additionally warns once at start when the configured interceptor classes show the misordering, including a finalizer listed on both sides of the cache (a finalizer beneath some cache is last-finalizer after first-cache, mirroring the query-timeout check). - cacheStatement() and removeStatement() read the cache link once into a local and use it throughout, so a disposal landing mid-park can no longer strand the reserved slot: the counter is pool-wide and was permanently shrinking every connection's statement budget by one per lost race. - getTypes() now returns a clone: the arrays are protected static and shared by every cache instance, so one caller mutating the result would change what every pool in the JVM caches. getCacheSizePerConnection() answers a disposed or never-set-up binding quietly with 0 instead of logging a warning per JMX poll. - The StatementFinalizer javadoc's "harmless no-op" claim is corrected, the MBean size attributes state what they count, and the jdbc-pool documentation carries the ordering rule and refuse-to-park behavior. --- modules/jdbc-pool/doc/jdbc-pool.xml | 6 +- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 25 +- .../jdbc/pool/interceptor/StatementCache.java | 123 ++++++++- .../jdbc/pool/interceptor/StatementCacheMBean.java | 10 +- .../jdbc/pool/interceptor/StatementFinalizer.java | 11 +- .../tomcat/jdbc/pool/util/LocalStrings.properties | 2 + .../TestStatementCacheCloseAfterDisposal.java | 125 +++++++++ .../TestStatementCacheStatementFinalizerOrder.java | 285 +++++++++++++++++++++ webapps/docs/changelog.xml | 16 ++ 9 files changed, 576 insertions(+), 27 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index e0b356b0c7..5ddde5ed27 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -701,7 +701,11 @@ driver defaults. Interceptors listed <em>after</em> the cache in <code>jdbcInterceptors</code> do not see reused statements – for example, <code>QueryTimeoutInterceptor</code> must be listed <em>before</em> the cache to keep - its timeout on cached statements. + its timeout on cached statements. An interceptor that tracks statements in order to + close them when the connection is returned, such as <code>StatementFinalizer</code>, + must also be listed <em>before</em> the cache: a parked statement is deliberately + left open, so a tracker beneath the cache would close entries out from under a live + cache, and the cache therefore refuses to park statements at all in that order. </p> <attributes> <attribute name="prepared" required="false"> diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index d287b7d3d7..2502df05e9 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -50,6 +50,7 @@ import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; import org.apache.tomcat.jdbc.pool.interceptor.QueryTimeoutInterceptor; import org.apache.tomcat.jdbc.pool.interceptor.StatementCache; +import org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer; import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** @@ -763,21 +764,26 @@ public class ConnectionPool { } /** - * Warns once at pool start when the configured interceptor chain lets the statement cache answer a cache hit - * before the query timeout interceptor can re-apply its timeout. A + * Warns once at pool start when the configured interceptor order defeats one of the pool's own interceptors. A * {@link org.apache.tomcat.jdbc.pool.interceptor.StatementCache} hit is served without invoking the rest of the * chain, and a statement recycled into the cache carries query timeout zero, so a * {@link org.apache.tomcat.jdbc.pool.interceptor.QueryTimeoutInterceptor} listed after the cache never sees the - * reuse and every cached statement executes with no timeout. The chain is built back-to-front in - * {@link #setupConnection(PooledConnection)}, making the first-listed interceptor the outermost one, so the - * timeout interceptor must be listed before the cache. This detection only reports the misconfiguration; no - * behaviour is changed. + * reuse and every cached statement executes with no timeout. And because a parked statement is deliberately left + * open, a {@link StatementFinalizer} listed after the cache still tracks that statement - it is reached only by the + * cache's own handle, never by the finalizer's close-aware proxy, so the close that parked it is invisible to the + * tracker - and the tracker's close on connection return then closes the parked statement out from under a live + * cache entry, leaving the next borrower with a closed statement. The chain is built back-to-front in + * {@link #setupConnection(PooledConnection)}, making the first-listed interceptor the outermost one, so both the + * timeout interceptor and the finalizer must be listed before the cache. This detection only reports the + * misconfiguration; the cache's own defence against the tracker beneath it is separate and does not depend on + * this check. * * @param proxies the parsed interceptor definitions, in configuration order */ private void checkInterceptorOrder(PoolProperties.InterceptorDefinition[] proxies) { int firstCache = -1; int lastTimeout = -1; + int lastFinalizer = -1; for (int i = 0; i < proxies.length; i++) { try { Class<? extends JdbcInterceptor> interceptorClass = proxies[i].getInterceptorClass(); @@ -787,6 +793,8 @@ public class ConnectionPool { } } else if (QueryTimeoutInterceptor.class == interceptorClass) { lastTimeout = i; + } else if (StatementFinalizer.class == interceptorClass) { + lastFinalizer = i; } } catch (ReflectiveOperationException | LinkageError x) { // The pool-start loop above has already loaded and instantiated every one of these classes, so @@ -798,6 +806,11 @@ public class ConnectionPool { if (firstCache >= 0 && lastTimeout > firstCache) { log.warn(MessageFactory.get("connectionPool.statementCacheBeforeQueryTimeout")); } + // A finalizer beneath some cache is exactly lastFinalizer > firstCache, so this catches the case where + // the finalizer is listed on both sides of the cache (firstFinalizer would sit before it and stay silent). + if (firstCache >= 0 && lastFinalizer > firstCache) { + log.warn(MessageFactory.get("connectionPool.statementCacheBeforeStatementFinalizer")); + } } /** 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 04787b3132..01ac2a3b3b 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 @@ -50,7 +50,11 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * 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: 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. + * prepare ran on rather than to whatever connection a wrapper beneath it reports for the statement. An interceptor + * that closes tracked statements at the connection return is affected too: {@link StatementFinalizer} must be listed + * before this one, because a parked statement is deliberately left open and a tracker beneath the cache would close + * it out from under a live cache entry; listed after this one, it makes this cache refuse to park at all, and + * statements are closed normally instead of cached. */ public class StatementCache extends StatementDecoratorInterceptor implements StatementCacheMBean { private static final Log log = LogFactory.getLog(StatementCache.class); @@ -78,6 +82,19 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta private static final String SET_POOLABLE = "setPoolable"; + /** + * State of the one look for a tracker beneath this interceptor: the chain has not been looked at yet. + */ + private static final int TRACKER_UNCHECKED = 0; + /** + * State of the look: no tracker that closes parked statements sits beneath this interceptor. + */ + private static final int TRACKER_NONE_BELOW = 1; + /** + * State of the look: such a tracker sits beneath this interceptor and statements must not be recycled. + */ + private static final int TRACKER_BELOW = 2; + /** * Default constructor. */ @@ -109,13 +126,27 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta /** * Returns the statement types being cached. + * <p> + * A copy is returned because the arrays a cache matches against are shared + * by every instance in the class loader; mutating the array behind the + * accessor would change what every pool in the JVM caches, not just this + * one. * * @return array of statement type strings */ public String[] getTypes() { - return types; + String[] t = this.types; + return t == null ? null : t.clone(); } + /** + * The pool-wide count of statements parked in the caches of all + * connections. Parked statements are physically open until the cache is + * disposed, so this is not the number of statements ready to run. + * + * @return the count, or {@code -1} once this interceptor has been disposed + * and the pool-wide counter is gone + */ @Override public int getCacheSize() { AtomicInteger size = this.cacheSize; @@ -153,6 +184,47 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta private AtomicInteger cacheSize; + /** + * Whether a tracker that closes its tracked statements at the connection return sits beneath this interceptor in + * the chain. A {@link StatementFinalizer} below the cache tracks the driver's statement - its close-aware proxy is + * reached only by a close that descends through the cache, and the close that parks a statement stops here - so a + * parked statement, deliberately left open, would be closed by that tracker at the connection return out from + * under a live cache entry, and the next hit would hand the borrower a closed statement. A park is refused when + * this answers true, which leaves the close to the normal path; the safe order, in which the tracker sits above + * the cache and tracks the cache handle that retires itself at the park, answers false and keeps caching. The + * chain of an interceptor is fixed once built, so the first look settles the answer for the life of this + * connection's chain; the state is settled by compare-and-set, so of two concurrent first parks exactly one + * settles and reports, and the other re-reads the settled value. + */ + private final AtomicInteger trackerBelow = new AtomicInteger(TRACKER_UNCHECKED); + + /** + * Looks once down the chain below this interceptor for a statement tracker that would close a parked statement at + * the connection return. + * + * @return true if such a tracker sits beneath this cache, in which case statements are not recycled + */ + private boolean trackerBelow() { + int state = trackerBelow.get(); + if (state == TRACKER_UNCHECKED) { + boolean below = false; + for (JdbcInterceptor i = getNext(); i != null; i = i.getNext()) { + if (i instanceof StatementFinalizer) { + below = true; + break; + } + } + if (trackerBelow.compareAndSet(TRACKER_UNCHECKED, below ? TRACKER_BELOW : TRACKER_NONE_BELOW)) { + if (below) { + log.warn(MessageFactory.get("statementCache.trackerBeneathCache")); + } + return below; + } + state = trackerBelow.get(); + } + return state == TRACKER_BELOW; + } + @Override public void poolStarted(ConnectionPool pool) { cacheSizeMap.putIfAbsent(pool, new AtomicInteger(0)); @@ -302,7 +374,15 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta */ public boolean cacheStatement(CachedStatement proxy) { ConcurrentHashMap<CacheKey, CachedStatement> cache = getCache(); - if (cache == null) { + // Read the counter once, like getCacheSize() and closeInvoked() do: + // a disposal (the reset(null, null) walks) nulls the field, and every + // read of the field itself - here at the entry check, and again in the + // withdrawal below - could meet a null in the middle of this call, so + // that the withdrawal throws and the increment this call made is never + // returned to the counter the whole pool shares. The counter object + // outlives the field's nulling, so a local keeps it reachable. + AtomicInteger size = cacheSize; + if (cache == null || size == null) { return false; } if (proxy.getCacheKey() == null) { @@ -315,10 +395,10 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta // and must not be parked in the cache of the new one. logLostAssociationOnce(proxy); return false; - } else if (cacheSize.get() >= maxCacheSize) { + } else if (size.get() >= maxCacheSize) { return false; - } else if (cacheSize.incrementAndGet() > maxCacheSize) { - cacheSize.decrementAndGet(); + } else if (size.incrementAndGet() > maxCacheSize) { + size.decrementAndGet(); return false; } // Reserve the slot first and let the map itself arbitrate the @@ -342,7 +422,7 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta // statement is not in the map and never was. Undo exactly // the one increment this call made and let the caller // close it - nothing here can leak a slot or a statement. - cacheSize.decrementAndGet(); + size.decrementAndGet(); } } return parked; @@ -359,11 +439,18 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta */ public boolean removeStatement(CachedStatement proxy) { ConcurrentHashMap<CacheKey, CachedStatement> cache = getCache(); - if (cache == null) { + // The counter is read once for the same disposal window as in + // cacheStatement: a disposal is answered like at the other entry + // points, with false, because its disconnect purge has already walked + // the entries and withdrew their slots while the fields were still + // set. The counter object outlives the field's nulling, so the local + // keeps it reachable for the decrement below. + AtomicInteger size = cacheSize; + if (cache == null || size == null) { return false; } if (cache.remove(proxy.getCacheKey(), proxy)) { - cacheSize.decrementAndGet(); + size.decrementAndGet(); return true; } else { return false; @@ -450,6 +537,12 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta @Override public int getCacheSizePerConnection() { + // Answer a disposed or never set up binding quietly, as getCacheSize() + // does: a JMX read can land in the disposal window, and a benign + // monitoring call should not log. + if (pcon == null) { + return 0; + } ConcurrentHashMap<CacheKey, CachedStatement> cache = getCache(); if (cache == null) { return 0; @@ -556,12 +649,16 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta // 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. + // the normal path. The sibling entry points tolerate a disposed + // cache and answer false or null - some of them through the + // warning in getCache(); this first read has to tolerate it the + // same way rather than dereference the field. AtomicInteger size = cacheSize; boolean shouldClose = true; - if (size != null && size.get() < maxCacheSize) { + 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 + // refused here and the close falls to the normal path below. // 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 diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementCacheMBean.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementCacheMBean.java index cf265b605c..a8bc117a53 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementCacheMBean.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementCacheMBean.java @@ -53,9 +53,15 @@ public interface StatementCacheMBean { int getCacheSize(); /** - * Returns the cache size for the current connection. + * Returns the cache size for the current connection: the statements parked + * in this connection's cache. Parked statements are physically open until + * the cache is disposed, so this is not the number of statements ready to + * run. A read against an interceptor that is not bound to a pooled + * connection, for instance one made while a disposal is in progress, is + * answered quietly rather than logged. * - * @return number of cached statements for this connection + * @return number of cached statements for this connection, or {@code 0} if + * this interceptor instance is not bound to a pooled connection */ int getCacheSizePerConnection(); } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java index 4223050e9e..303d132291 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementFinalizer.java @@ -43,11 +43,12 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; * when the connection is returned. Statements that the application does close are forgotten by the returned proxy at * the moment <code>close()</code> completes, keeping the tracked list bounded by the statements that are genuinely * still open and closed through that handle. (The removal cannot observe a close that reaches the statement by - * another route: an interceptor placed outside this one may swallow the statement's <code>close()</code>, e.g. a - * statement cache recycling instead of closing, and a statement the application closes through the driver handle - * that <code>unwrap</code> hands back never passes through this proxy; such entries are simply reclaimed at the - * connection return, where the renewed close meets a statement already closed as a harmless no-op, as they were - * before.) + * another route: a statement the application closes through the driver handle that <code>unwrap</code> hands back + * never passes through this proxy; such entries are simply reclaimed at the connection return, where the renewed + * close meets a statement already closed as a harmless no-op, as they were before. An interceptor placed outside + * this one could swallow a close in the same way, but the pool's own {@link StatementCache} no longer does: listed + * above this interceptor it refuses to recycle, because a recycled statement is left open rather than closed and + * would be reclaimed here alive, out from under the cache.) * <p> * Tracked statements are handed to the application as proxies implementing the standard JDBC statement interfaces, as * is the statement facade the pool always installs in front of this interceptor and as are the proxies of the other diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 1b6e1c4e8d..766d9628e8 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -49,6 +49,7 @@ connectionPool.poolMisconfigured=Connection pool is misconfigured, doesn''t supp connectionPool.poolWaitInterrupted=Pool wait interrupted. connectionPool.requestHintRefused=The request boundary hint [{0}] was refused by the driver connectionPool.statementCacheBeforeQueryTimeout=The [StatementCache] interceptor is configured before [QueryTimeoutInterceptor]. A statement cache hit is answered by the cache without invoking the timeout interceptor, and a statement recycled into the cache carries no timeout, so cached statements run without a query timeout. List QueryTimeoutInterceptor before StatementCache in the jdbcInterceptors configuration. +connectionPool.statementCacheBeforeStatementFinalizer=The [StatementCache] interceptor is configured before [StatementFinalizer]. The finalizer tracks the statements prepared beneath the cache and closes the still open ones when the connection is returned, while a statement parked in the cache is deliberately left open, so the cache does not park statements in this order and nothing is cached. List StatementFinalizer before StatementCache in the jdbcInterceptors configuration. connectionPool.suspectConnectionReturned=Connection({0}) that has been marked suspect was returned. The processing time is {1} ms. connectionPool.testAllIdleFailed=testAllIdle failed, it will be retried. connectionPool.timeBetweenEvictionRunsTooLarge=timeBetweenEvictionRunsMillis is larger than maxAge, setting timeBetweenEvictionRunsMillis to: {0} @@ -181,6 +182,7 @@ slowQueryReportJmx.unableToInitCompositeType=Unable to initialize composite data statementCache.connectionClosedOrAbandoned=Connection has already been closed or abandoned statementCache.nullKey=Null key +statementCache.trackerBeneathCache=Not recycling statements: a [StatementFinalizer] is listed below this interceptor, where the close at the connection return would reach a parked statement out from under the cache. List StatementFinalizer before StatementCache in the jdbcInterceptors configuration to use the statement cache. statementCache.unableToCloseDiscardedStatement=Unable to close statement [{0}] that the statement cache is discarding. statementDecoratorInterceptor.statementClosed=Statement closed. 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 index 6d7e1fb8a4..5e71a45fdb 100644 --- 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 @@ -19,6 +19,8 @@ package org.apache.tomcat.jdbc.pool.interceptor; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.Statement; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -45,6 +47,15 @@ import org.apache.tomcat.jdbc.pool.ProxyConnection; * 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. + * + * <p>The second test pins the narrower window inside a park that is still + * running: the disposal lands after the close has reserved its slot on the + * pool-wide entry counter but before the withdrawal of a refused insert. The + * counter object outlives the field's nulling (the pool owns it), so the + * withdrawal must run through the counter read at the start of the call - + * pre-fix it re-read the nulled field, threw, and left the reserved slot + * counted forever, shrinking the {@code max} budget of every connection in + * the pool by one per such race.</p> */ class TestStatementCacheCloseAfterDisposal extends JdbcPoolBaseTest { @@ -101,4 +112,118 @@ class TestStatementCacheCloseAfterDisposal extends JdbcPoolBaseTest { // A repeated close stays the quiet no-op the JDBC contract requires. statement.close(); } + + /** + * The disposal meeting a park mid-flight: the hook takes the key so the + * insert is refused, and disposes the interceptor while the park still + * holds its reservation. The close must finish as a plain physical close, + * and the refused statement's reservation must come back off the pool-wide + * counter - so a later, live session still finds the full {@code max=2} + * budget and parks both of its statements. Pre-fix the withdrawal hit the + * nulled field, the reservation leaked into the counter, and the second + * statement of the later session was refused with a full-looking cache. + */ + @Test + void aDisposalLandingInsideAnUnsuccessfulParkDoesNotEatAPoolWideSlot() throws Exception { + DataSource ds = newDataSource(); + PoolConfiguration properties = ds.getPoolProperties(); + properties.setJdbcInterceptors( + StatementCache.class.getName() + "(prepared=true,max=2)"); + properties.setMaxActive(2); + properties.setInitialSize(0); + // keep the background sweeper out of the way during the race + properties.setTimeBetweenEvictionRunsMillis(60000); + String table = uniqueTable(); + ds.createPool(); + try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + } + + Connection first = ds.getConnection(); + StatementCache cache = interceptorOf(first, StatementCache.class); + PooledConnection pooled = (PooledConnection) first.unwrap(PooledConnection.class); + DisposalAtParkMap parkPoint = new DisposalAtParkMap(); + parkPoint.disposal = () -> cache.reset(null, null); + pooled.getAttributes().put(StatementCache.STATEMENT_CACHE_ATTR, parkPoint); + + setSessionMarker(first, 61); + String sqlOne = "INSERT INTO " + table + " VALUES (?)"; + PreparedStatement racing = first.prepareStatement(sqlOne); + Statement racingPhysical = driverStatement(racing); + racing.setInt(1, 1); + Assertions.assertEquals(1, racing.executeUpdate()); + racing.close(); // the hook fires inside this close's park + Assertions.assertTrue(parkPoint.raced, + "precondition: the disposal must have landed inside the park"); + Assertions.assertTrue(racingPhysical.isClosed(), + "the refused park must have closed the statement through the normal path"); + // The raced session's chain is disposed; take it out of service so the + // next borrow must come from a fresh chain (on the shared counter). + pooled.setDiscarded(true); + first.close(); + awaitSessions(0); + + Connection second = ds.getConnection(); + try { + Assertions.assertNull(sessionMarker(second), + "the next borrower must sit on a fresh session and chain"); + StatementCache fresh = interceptorOf(second, StatementCache.class); + String sqlTwo = "INSERT INTO " + table + " VALUES (? + 1)"; + PreparedStatement a = second.prepareStatement(sqlOne); + Statement aPhysical = driverStatement(a); + a.close(); + PreparedStatement b = second.prepareStatement(sqlTwo); + Statement bPhysical = driverStatement(b); + b.close(); + Assertions.assertFalse(aPhysical.isClosed(), + "the first statement of the fresh session must park"); + Assertions.assertFalse(bPhysical.isClosed(), + "the second must park too - the raced reservation must have " + + "come back off the pool-wide counter"); + Assertions.assertEquals(2, fresh.getCacheSizePerConnection()); + Assertions.assertEquals(2, fresh.getCacheSize(), + "the pool-wide counter must equal the parked count"); + Assertions.assertSame(aPhysical, driverStatement(second.prepareStatement(sqlOne)), + "both cached statements must serve their next prepare"); + } finally { + second.close(); + } + } + + /** + * The cache map, swapped in like in {@link TestStatementCacheRacingPark}, + * on whose park point the disposal runs: the first {@code putIfAbsent} + * takes the key with a sentinel, disposes the interceptor, and then lets + * the real insert see the taken key, so the park is refused while its + * reservation is still in flight. The sentinel is withdrawn again - the + * entry the disconnect purge later walks is never a foreign object. + */ + private static final class DisposalAtParkMap extends ConcurrentHashMap<Object, Object> { + + private static final long serialVersionUID = 1L; + + private final AtomicBoolean racedOnce = new AtomicBoolean(); + + /** Run by the first intercepted insert, inside the park window. */ + volatile Runnable disposal = () -> { + }; + + /** Whether the disposal really ran inside a park. */ + volatile boolean raced; + + @Override + public Object putIfAbsent(Object key, Object value) { + if (racedOnce.compareAndSet(false, true)) { + super.putIfAbsent(key, this); + disposal.run(); + Object refused = super.putIfAbsent(key, value); + if (refused == this) { + super.remove(key, this); + } + raced = true; + return refused; + } + return super.putIfAbsent(key, value); + } + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheStatementFinalizerOrder.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheStatementFinalizerOrder.java new file mode 100644 index 0000000000..b20f5cb7fc --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheStatementFinalizerOrder.java @@ -0,0 +1,285 @@ +/* + * 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.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.List; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import java.util.stream.Collectors; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.juli.logging.LogFactory; +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.ConnectionPool; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * The interaction between the statement cache and the statement finalizer, + * which the pool builds into one chain in configuration order. The finalizer + * tracks the statement object its hook receives - the one the interceptor + * below the cache produces - and closes every tracked statement that is still + * open when the connection is returned. A parked statement is deliberately + * open, so with the cache listed first (outermost) the return-time close + * reaches the statement out from under a live cache entry, and the next + * borrower's cache hit hands out a statement that fails on any use. The cache + * therefore must refuse to park whenever a tracker sits beneath it, closing + * the statement through the normal path instead, while the safe order (the + * finalizer first, tracking the cache's own handle, which retires itself when + * the statement parks) must keep park-and-reusing. The pool warns once at + * start when it can see the misordering in the configured classes. + */ +class TestStatementCacheStatementFinalizerOrder extends JdbcPoolBaseTest { + + private static final String CACHE = StatementCache.class.getName() + "(prepared=true,max=5)"; + private static final String FINALIZER = StatementFinalizer.class.getName(); + + /** Distinguishes the new warning from the analogous QueryTimeout one. */ + private static final String FINALIZER_ORDER_WARNING = "before [StatementFinalizer]"; + + private DataSource poolWith(String interceptors) throws SQLException { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(interceptors); + ds.getPoolProperties().setMaxActive(1); + ds.createPool(); + return ds; + } + + /** + * The defect: the cache listed first parks on close, the finalizer beneath + * it still tracks the parked statement, and returning the connection + * closes it out from under the cache. The next borrower of that session + * then received a cache hit whose statement throws on any use (pre-fix + * this failed at {@code setInt} with "The object is already closed"). + * With the tracker beneath it the cache must not park at all: the close + * has to reach the statement, and the next borrower must get a fresh + * statement that works. + */ + @Test + void aClosedStatementIsNotParkedBeneathATrackerAndTheNextBorrowerGetsAWorkingStatement() + throws Exception { + DataSource ds = poolWith(CACHE + ";" + FINALIZER); + String table = uniqueTable(); + 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 = interceptorOf(first, StatementCache.class); + Assertions.assertTrue(cache.getNext() instanceof StatementFinalizer, + "the configuration order must really put the finalizer beneath the cache"); + setSessionMarker(first, 51); + PreparedStatement prepared = first.prepareStatement(insert); + Statement physical = driverStatement(prepared); + prepared.setInt(1, 1); + Assertions.assertEquals(1, prepared.executeUpdate()); + prepared.close(); + Assertions.assertTrue(physical.isClosed(), + "with a tracker beneath the cache the close must reach the statement " + + "instead of parking it open above the tracker"); + Assertions.assertEquals(0, cache.getCacheSizePerConnection(), + "nothing may be parked while a tracker beneath the cache would " + + "close the parked statement at connection return"); + first.close(); + + Connection second = ds.getConnection(); + try { + Assertions.assertEquals(Integer.valueOf(51), sessionMarker(second), + "the same physical session must have been reused"); + PreparedStatement reused = second.prepareStatement(insert); + Assertions.assertNotSame(physical, driverStatement(reused), + "the next borrower must get a fresh prepare, not the closed " + + "statement of the previous cache entry"); + reused.setInt(1, 2); + Assertions.assertEquals(1, reused.executeUpdate(), + "the statement the next borrower receives must work"); + reused.close(); + } finally { + second.close(); + } + Assertions.assertEquals(2, adminRowCount(table), + "both executes must have reached the database"); + } + + /** + * The safe order, which worked before and must keep working: the finalizer + * above the cache tracks the cache's own handle, whose retirement at the + * park makes the return-time reclaim a no-op, so the statement stays open + * in the cache and the next borrower of the session gets the very same + * driver statement back. A fix aimed at the other order must not quietly + * switch the caching off here. + */ + @Test + void theSafeOrderStillParksAndHandsTheSameStatementToTheNextBorrower() throws Exception { + DataSource ds = poolWith(FINALIZER + ";" + CACHE); + String table = uniqueTable(); + 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 = interceptorOf(first, StatementCache.class); + Assertions.assertFalse(cache.getNext() instanceof StatementFinalizer, + "an interceptor listed before the cache must not sit beneath it"); + setSessionMarker(first, 52); + PreparedStatement prepared = first.prepareStatement(insert); + Statement physical = driverStatement(prepared); + prepared.setInt(1, 1); + Assertions.assertEquals(1, prepared.executeUpdate()); + prepared.close(); + Assertions.assertFalse(physical.isClosed(), + "the tracker above the cache must let the statement park open"); + Assertions.assertEquals(1, cache.getCacheSizePerConnection(), + "the safe order must still park"); + first.close(); + + Connection second = ds.getConnection(); + try { + Assertions.assertEquals(Integer.valueOf(52), 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 - the tracker " + + "above must not close the parked statement"); + 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 cache as the given borrower sees it. */ + private static StatementCache cacheOf(Connection pooled) { + return interceptorOf(pooled, StatementCache.class); + } + + /** + * The pool-side announcement: {@code checkInterceptorOrder} compares the + * configured classes and must warn once at start when the cache is listed + * before the finalizer - the same way it already warns for the analogous + * query-timeout misordering - and must stay silent for the safe order. A + * finalizer listed on both sides of the cache still has one beneath it, + * which defeats the cache just the same, so that order must be reported + * too (pre-fix only the first occurrence was looked at and it was not). + */ + @Test + void thePoolWarnsAtStartOnlyForTheOrderThatDefeatsTheCache() throws Exception { + try (PoolLogCapture logs = new PoolLogCapture()) { + DataSource wrong = newDataSource(); + wrong.getPoolProperties().setJdbcInterceptors(CACHE + ";" + FINALIZER); + wrong.createPool(); + logs.assertFinalizerOrderWarnings(1, + "a cache configured above the finalizer defeats the cache and must be reported"); + } + try (PoolLogCapture logs = new PoolLogCapture()) { + DataSource wrongToo = newDataSource(); + wrongToo.getPoolProperties().setJdbcInterceptors( + FINALIZER + ";" + CACHE + ";" + FINALIZER); + wrongToo.createPool(); + logs.assertFinalizerOrderWarnings(1, + "a finalizer on both sides still leaves one beneath the cache " + + "and must be reported (pre-fix it was silent)"); + } + try (PoolLogCapture logs = new PoolLogCapture()) { + DataSource right = newDataSource(); + right.getPoolProperties().setJdbcInterceptors(FINALIZER + ";" + CACHE); + right.createPool(); + logs.assertFinalizerOrderWarnings(0, + "the safe order must not be reported"); + } + } + + /** + * Captures what {@link ConnectionPool} logs at pool start, following the + * handler idiom of the pool's other diagnostic tests. The canary - written + * through the JULI logger the pool logs through - proves the wiring + * observes those records before the pool under test gets its chance to log. + */ + private static final class PoolLogCapture implements AutoCloseable { + private static final String CANARY = "finalizer-order-handler-wiring-canary"; + + private final Logger logger = Logger.getLogger(ConnectionPool.class.getName()); + private final Level originalLevel = logger.getLevel(); + private final List<LogRecord> records = new ArrayList<>(); + private final Handler handler = new Handler() { + @Override + public void publish(LogRecord record) { + synchronized (records) { + records.add(record); + } + } + + @Override + public void flush() { + // no buffering + } + + @Override + public void close() { + // nothing to release + } + }; + + PoolLogCapture() { + logger.addHandler(handler); + logger.setLevel(Level.ALL); + LogFactory.getLog(ConnectionPool.class).error(CANARY); + synchronized (records) { + if (records.stream().noneMatch(r -> CANARY.equals(r.getMessage()))) { + close(); + throw new AssertionError("the test handler must observe what this class logs"); + } + } + } + + /* Records carrying the finalizer-order warning, counted. */ + void assertFinalizerOrderWarnings(int expected, String why) { + List<LogRecord> warnings; + synchronized (records) { + warnings = records.stream() + .filter(r -> r.getMessage() != null && r.getMessage().contains(FINALIZER_ORDER_WARNING)) + .collect(Collectors.toList()); + } + Assertions.assertEquals(expected, warnings.size(), + why + ", messages seen: " + records.stream().map(LogRecord::getMessage) + .collect(Collectors.toList())); + } + + @Override + public void close() { + logger.setLevel(originalLevel); + logger.removeHandler(handler); + } + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 99559d184d..bfe3f8adf7 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1216,6 +1216,22 @@ and reuse, and a park refused for a lost association is now noted once per connection at debug level. (remm) </fix> + <fix> + Correct a defect where listing the <code>StatementCache</code> + interceptor before the <code>StatementFinalizer</code> interceptor in + <code>jdbcInterceptors</code> handed the next borrower of a session a + cached statement that failed on any use. The finalizer tracked the + driver statement beneath the cache and closed every statement still + open when the connection was returned, while a statement parked in + the cache is deliberately left open, so that return-time close killed + the cached statement out from under a live cache entry and the next + cache hit served a closed statement. The cache now refuses to park + statements when such a tracker is configured beneath it - the close + then reaches the statement through the normal path - and the pool + warns once at start when the configuration lists the cache before the + finalizer, as it already does for the analogous query timeout + ordering. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
