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 2fe56df201f6a40fff1fe082b272852fd6db3030 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 16:57:18 2026 +0100 Document that QueryTimeoutInterceptor must precede StatementCache The query timeout is applied by QueryTimeoutInterceptor's createStatement hook, which only runs for creation calls that actually reach that interceptor. StatementCache answers a cache hit by returning the cached proxy without invoking the rest of the chain, and a statement recycled into the cache has its query timeout reset to zero. With the cache listed first in jdbcInterceptors (first-listed is the outermost), the first execution of a prepared statement therefore carries the configured timeout but every later prepareStatement(<same sql>) is served from the cache with no timeout at all and no log entry, because the deeper interceptor never sees the reuse. Nothing documented this, and no test covered the combination. Closed as a documentation-and-test change rather than a behavioural one. The alternative considered was to wrap the statement and re-apply the timeout on each execute whenever it reads back as 0, but that cannot distinguish a cache reset from an application's deliberate setQueryTimeout(0) ("no timeout"), which JDBC allows and which today correctly wins over the pool default: it would trade one silent-timeout path for another, on every execute of every cached statement. So the requirement is now stated where each reader meets it: in the javadoc of both interceptors (cache hits bypass interceptors listed after the cache, while wrapping-only interceptors are unaffected because their wrapper rides inside the cached proxy), and in both subsections of doc/jdbc-pool.xml. The QueryTimeoutInterceptor class javadoc also now says what the hook does, which was stale since it stopped wrapping execute(). TestQueryTimeoutWithStatementCache (2) pins both orders through real sessions: the supported order keeps the timeout across a cache hit, and the wrong order is pinned as the documented limitation so a future change to either interceptor is noticed here. Both cases assert the recycle reset first, because H2 reports the session-wide timeout rather than a per-statement one; without that pin the observed failure value is the previous borrower's timeout, not 0. Full suite: 255 tests, no failures. No changelog entry, as this documents existing behaviour. --- modules/jdbc-pool/TESTING-PLAN.md | 9 ++ modules/jdbc-pool/doc/jdbc-pool.xml | 14 +++ .../pool/interceptor/QueryTimeoutInterceptor.java | 13 +++ .../jdbc/pool/interceptor/StatementCache.java | 12 +++ .../TestQueryTimeoutWithStatementCache.java | 118 +++++++++++++++++++++ 5 files changed, 166 insertions(+) diff --git a/modules/jdbc-pool/TESTING-PLAN.md b/modules/jdbc-pool/TESTING-PLAN.md index 23d4ce5fda..369ef21e5d 100644 --- a/modules/jdbc-pool/TESTING-PLAN.md +++ b/modules/jdbc-pool/TESTING-PLAN.md @@ -344,6 +344,15 @@ the working list. instead of the handle), the cache outliving a `reconnect()` whose purge hook was skipped, and the pool-wide `cacheSize` counter skewing when caching races `disconnected()`. +* `TestQueryTimeoutWithStatementCache` (2) pins the ordering dependency + between the two: with `StatementCache` listed first, a cache hit skips + the rest of the chain and the next borrower of the same SQL gets the + timeout the *first* borrower configured or `0` after a recycle reset, + not the one it configured - that case is pinned as the documented + limitation, with the reset itself pinned first because H2 reports the + session-wide value; the supported order (`StatementCache` last) is + pinned to keep the configured timeout across a cache hit. Both are + javadoc/wording fixes in the shipped code, not behaviour changes. * `build.xml` (the Ant release build of this module) still has a `build-test` target that compiles `src/test/java` against a JUnit 4 + `tomcat-dbcp` + c3p0/derby/mysql classpath it downloads, and ships diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index bd2984b03c..c53a6ead27 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -671,6 +671,13 @@ the same pool. Once the count reaches <code>max</code>, subsequent statements are not returned to the cache and are closed immediately. </p> + <p><strong>Note:</strong> a cache hit is answered by this interceptor without + invoking the rest of the interceptor chain, and recycled statements are reset to + 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. + </p> <attributes> <attribute name="prepared" required="false"> <p>(boolean as String) Enable caching of <code>PreparedStatement</code> @@ -703,6 +710,13 @@ <p>Automatically calls <code>java.sql.Statement.setQueryTimeout(seconds)</code> when a new statement is created. The pool itself doesn't timeout the query, it is still up to the JDBC driver to enforce query timeouts. </p> + <p><strong>Note:</strong> when combined with + <code>org.apache.tomcat.jdbc.pool.interceptor.StatementCache</code>, this interceptor must be listed + <em>before</em> the statement cache in the <code>jdbcInterceptors</code> attribute. The cache serves a hit + without invoking the rest of the interceptor chain and recycles statements with their query timeout reset + to zero, so an interceptor listed after the cache does not see cached statements on reuse and those + statements execute with no timeout. + </p> <attributes> <attribute name="queryTimeout" required="true"> <p>(int as String) The number of seconds to set for the query timeout. diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/QueryTimeoutInterceptor.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/QueryTimeoutInterceptor.java index 5f7c9846d4..9640d2b94a 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/QueryTimeoutInterceptor.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/QueryTimeoutInterceptor.java @@ -27,6 +27,19 @@ import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorProperty; /** * Interceptor that sets a query timeout on every created {@link Statement}. + * <p> + * The timeout is applied by the {@code createStatement} hook, which only runs + * for statement-creation calls that pass through this interceptor. When + * combined with {@link StatementCache}, this interceptor must therefore be + * listed <em>before</em> {@code StatementCache} in the + * {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#setJdbcInterceptors(String) + * jdbcInterceptors} configuration. A statement cache hit is served by the + * cache without invoking the rest of the chain, and a statement recycled into + * the cache has its query timeout reset to zero, so an interceptor listed + * after the cache never sees the reuse and the cached statement executes with + * no timeout. Listed before the cache, the hook runs for every creation + * request, including the ones the cache serves, and the timeout is re-applied + * to the reused statement. */ public class QueryTimeoutInterceptor extends AbstractCreateStatementInterceptor { private static final Log log = LogFactory.getLog(QueryTimeoutInterceptor.class); 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 19a141efa4..7e9747ef6e 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 @@ -38,6 +38,18 @@ import org.apache.tomcat.jdbc.pool.jmx.JmxUtil; /** * Interceptor that caches {@code PreparedStatement} and/or * {@code CallableStatement} instances on a connection. + * <p> + * A cache hit is answered by this interceptor alone: the rest of the + * interceptor chain is not invoked for it, and a statement recycled into + * the cache is reset to driver defaults (its query timeout goes back to + * zero). Interceptors 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. */ public class StatementCache extends StatementDecoratorInterceptor implements StatementCacheMBean { private static final Log log = LogFactory.getLog(StatementCache.class); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryTimeoutWithStatementCache.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryTimeoutWithStatementCache.java new file mode 100644 index 0000000000..4c357fcc5f --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestQueryTimeoutWithStatementCache.java @@ -0,0 +1,118 @@ +/* + * 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 org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * The query timeout is applied by the {@code createStatement} hook of + * {@link QueryTimeoutInterceptor}, which only runs for creation calls that + * pass through that interceptor. A {@link StatementCache} hit is served + * without invoking the rest of the chain, and a statement recycled into + * the cache carries query timeout 0. The ordering in + * {@code jdbcInterceptors} therefore decides whether reused statements + * keep the configured timeout - a requirement now documented on the + * interceptor and in the jdbc-pool docs, and pinned here in both orders. + */ +class TestQueryTimeoutWithStatementCache extends JdbcPoolBaseTest { + + private static final String SQL = "SELECT 42"; + + private DataSource poolWith(String... interceptors) throws SQLException { + StringBuilder chain = new StringBuilder(); + for (String interceptor : interceptors) { + if (chain.length() > 0) { + chain.append(';'); + } + chain.append(interceptor); + } + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(chain.toString()); + ds.createPool(); + return ds; + } + + @Test + void reusedStatementKeepsTheTimeoutWhenTheInterceptorIsListedFirst() + throws Exception { + DataSource ds = poolWith( + QueryTimeoutInterceptor.class.getName() + "(queryTimeout=5)", + StatementCache.class.getName() + "(prepared=true,max=5)"); + try (Connection pooled = ds.getConnection()) { + PreparedStatement first = pooled.prepareStatement(SQL); + Assertions.assertEquals(5, first.getQueryTimeout()); + Statement physical = driverStatement(first); + first.close(); + Assertions.assertEquals(0, physical.getQueryTimeout(), + "the cache must have recycled the statement with the timeout reset"); + + // the cache serves the second creation, and because this + // interceptor sits outside the cache its hook still runs - the + // timeout is re-applied to the recycled statement + PreparedStatement reused = pooled.prepareStatement(SQL); + Assertions.assertSame(physical, driverStatement(reused), + "the same driver statement must come back from the cache"); + Assertions.assertEquals(5, reused.getQueryTimeout(), + "a cache hit served through an outer QueryTimeoutInterceptor " + + "must carry the configured timeout"); + reused.close(); + } + } + + @Test + void reusedStatementLosesTheTimeoutWhenTheCacheIsListedFirst() + throws Exception { + DataSource ds = poolWith( + StatementCache.class.getName() + "(prepared=true,max=5)", + QueryTimeoutInterceptor.class.getName() + "(queryTimeout=5)"); + try (Connection pooled = ds.getConnection()) { + PreparedStatement first = pooled.prepareStatement(SQL); + Assertions.assertEquals(5, first.getQueryTimeout(), + "the creating call reaches the interceptor even in this order"); + Statement physical = driverStatement(first); + first.close(); + Assertions.assertEquals(0, physical.getQueryTimeout(), + "the cache must have recycled the statement with the timeout reset"); + + // documented limitation: the cache answers the second creation + // itself, the deeper interceptor never runs, and the recycled + // statement carries the 0 the cache reset it to - pinning this + // so a future change to either interceptor is noticed here + PreparedStatement reused = pooled.prepareStatement(SQL); + Assertions.assertSame(physical, driverStatement(reused), + "the same driver statement must come back from the cache"); + Assertions.assertEquals(0, reused.getQueryTimeout(), + "a cache hit served around an inner QueryTimeoutInterceptor " + + "carries no timeout - which is why the interceptor " + + "must be listed before the cache; if this ever reads " + + "the configured value instead, the order dependency " + + "has been removed and the notes on " + + "QueryTimeoutInterceptor and StatementCache must be " + + "updated along with this test"); + reused.close(); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
