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 d8ba4c75fa3ea08a02836fdcca2d8f8b4bab79d9 Author: opencode <[email protected]> AuthorDate: Fri Oct 23 18:50:18 2026 +0200 Test the pool's own statement cache reuse Statement caching is core pool functionality, and until now the suite only proved that a cached statement could be used again inside one borrow and that a recycled statement carried no leftovers. That left the actual reuse untested: the hygiene tests would pass just as well if the cache threw every statement away. StatementCacheReuseTest pins the documented behaviour, with the driver's own statement object behind the pool proxy as the oracle - two handles onto the same instance mean the session was not asked to prepare again, and its isClosed() says what really happened to the statement: - a closed statement is handed to the next borrower of the same physical session, without a second prepare, and the rows of both borrowers reach the database; - each session caches its own statements and never lends one to another session; - the max count is counted over the whole pool, so a session that arrives after the limit is reached gets its statement closed immediately instead of cached - and gets it cached once another session takes its statement out of the cache; - a statement that did not fit is prepared again on demand and works; - the cache belongs to the physical session it was built on: after purge() the new session starts empty and prepares afresh; - only the statement types the configuration asks for are cached (prepared=false, callable=true caches callables and not prepared statements). PoolTestSupport gains interceptorOf() for reaching an interceptor in the chain of a borrowed connection and driverStatement() for the driver's object behind a statement proxy; InterceptorLifecycleTest now uses the former instead of its own copy of the chain walk. InterceptorSemanticsTest configured StatementCache with maxCacheSize=10, which the interceptor never reads - the property is called max - so that pool has been running with the default limit of 50. Corrected. What is deliberately not pinned is noted in TESTING-PLAN.md: the cached statement answering getConnection() with the previous borrower's facade, the cache outliving a reconnect whose purge hook was skipped, and the pool-wide counter skewing when caching races disconnected(); all are findings in defects-pool for the defect-processing phase. 6 new tests; the suite is green at 140 tests, 56.9% instruction / 52.8% branch coverage. --- modules/jdbc-pool/TESTING-PLAN.md | 37 ++- .../jdbc/functional/InterceptorLifecycleTest.java | 20 +- .../jdbc/functional/InterceptorSemanticsTest.java | 2 +- .../tomcat/jdbc/functional/PoolTestSupport.java | 32 +++ .../jdbc/functional/StatementCacheReuseTest.java | 288 +++++++++++++++++++++ 5 files changed, 353 insertions(+), 26 deletions(-) diff --git a/modules/jdbc-pool/TESTING-PLAN.md b/modules/jdbc-pool/TESTING-PLAN.md index 2644e21bbc..00dae6f5aa 100644 --- a/modules/jdbc-pool/TESTING-PLAN.md +++ b/modules/jdbc-pool/TESTING-PLAN.md @@ -175,10 +175,13 @@ Fixture details worth knowing before writing a test against it `setSessionMarker`/`sessionMarker` (an H2 session variable, to prove *which* physical session a borrower got) and `adminRowCount` (committed data, read over a separate admin connection). -* Fault injection goes through the real objects: +* Fault injection and white-box reach go through the real objects: `((javax.sql.PooledConnection) pooled).getConnection()` for the - physical session, `(JdbcInterceptor) Proxy.getInvocationHandler(pooled)` - to walk the interceptor chain. + physical session, `interceptorOf(pooled, type)` to find an interceptor + in the chain of a borrowed connection (its invocation handler is the + head of that chain), and `driverStatement(proxy)` — `unwrap` — for the + driver's own object behind a pooled statement, which is the honest + oracle for "this was not prepared on the session twice". ## 4. High-defect map (backlog for the later defect-processing phase — not implemented now) @@ -232,6 +235,15 @@ the working list. cancellation needs a real multi-second query (H2 has no `SLEEP()` and an in-process `CREATE ALIAS` is not cancel-safe), so the "< 60 s" budget is understood to exclude it. + Added afterwards, because the pool's own caching is core + functionality: `StatementCacheReuseTest` (6) pins the statement cache + on the driver's own statement object - a closed statement is handed + to the next borrower of the same session without a second prepare, + each session caches its own, the `max` count is counted over the whole + pool and the statement that does not fit is closed immediately and + re-prepared on demand, the cache disappears with the physical session + it was built on, and only the configured statement types are cached. + Suite totals: 140 tests, ~80 s, 56.9 % instruction / 52.8 % branch. 3. **Old suite removal**: **done**. The harvest (§1 "Harvest result") landed first: 75 tests in 12 new classes plus one added `JmxMonitorTest` case. The JUnit 4 suite then went away together @@ -241,14 +253,14 @@ the working list. `pool/` unit tests they superseded, along with the `junit`, `tomcat-dbcp` and `junit-vintage-engine` test dependencies and the `PoolCleanerTest` exclude. - The module is now 134 tests in about 80 s of test time (was ~6 min - for 319), at 56.6 % instruction / 52.2 % branch coverage. The - coverage number is *lower* than the 66.1 % measured one commit - earlier, and that is the honest reading: the legacy tests ran + At that point the module stood at 134 tests in about 80 s of test + time (was ~6 min for 319) and 56.6 % instruction / 52.2 % branch + coverage. The coverage number is *lower* than the 66.1 % measured one + commit earlier, and that is the honest reading: the legacy tests ran through `DataSourceProxy`, the JMX and naming code and the whole interceptor surface with a stub driver attached, touching code without ever deciding whether it worked. Phase 5 starts its ratchet - from this number. + from the current number (56.9 % / 52.8 %). 4. **Defect processing** (later): per defect, a reproducer test per §4 + the fix; Postgres/kill tier added when the first defect needs it (single-session kill, `idle_session_timeout`, @@ -297,6 +309,15 @@ the working list. next ordinary borrow has to reconnect. The pool stays usable (`AlternateCredentialsTest` pins that much), but the shrinkage is not something to lock in as expected behaviour. +* `StatementCacheReuseTest` pins the documented caching behaviour and + deliberately stops there. The findings in + `defects-pool/defects-...interceptor.StatementCache.md` stay unpinned: + a cached statement keeps answering `getConnection()` with the + *previous* borrower's facade (the pool rebuilds the JDBC handle on + purpose, which is why the reuse tests compare the *driver* statement + 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()`. * `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/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorLifecycleTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorLifecycleTest.java index 21d699c320..64e3cffb28 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorLifecycleTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorLifecycleTest.java @@ -60,32 +60,18 @@ class InterceptorLifecycleTest extends PoolTestSupport { return ds; } - /** Walks the interceptor chain of a borrowed connection. */ - private static JdbcInterceptor findInChain(Connection pooled, Class<?> type) { - JdbcInterceptor interceptor = - (JdbcInterceptor) Proxy.getInvocationHandler(pooled); - while (interceptor != null) { - if (type.isInstance(interceptor)) { - return interceptor; - } - interceptor = interceptor.getNext(); - } - return null; - } - @Test void aShortInterceptorNameIsResolvedAndConfigured() throws Exception { DataSource ds = reportedPool(); ds.createPool(); try (Connection pooled = ds.getConnection()) { - JdbcInterceptor interceptor = - findInChain(pooled, SlowQueryReport.class); + SlowQueryReport interceptor = + interceptorOf(pooled, SlowQueryReport.class); assertNotNull(interceptor, "the interceptor named by short name must sit in the chain of a " + "borrowed connection"); - assertInstanceOf(SlowQueryReport.class, interceptor); - assertEquals(7L, ((SlowQueryReport) interceptor).getThreshold(), + assertEquals(7L, interceptor.getThreshold(), "the properties from the configuration string must be applied"); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorSemanticsTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorSemanticsTest.java index 89bcd8cd53..9c60dae74b 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorSemanticsTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorSemanticsTest.java @@ -134,7 +134,7 @@ class InterceptorSemanticsTest extends PoolTestSupport { void statementCacheServesRepeatedPreparesWithinOneBorrow() throws Exception { DataSource ds = newDataSource(); ds.getPoolProperties().setJdbcInterceptors( - "org.apache.tomcat.jdbc.pool.interceptor.StatementCache(maxCacheSize=10)"); + "org.apache.tomcat.jdbc.pool.interceptor.StatementCache(prepared=true,max=10)"); ds.getPoolProperties().setMaxActive(1); String t = uniqueTable(); try (Connection c = ds.getConnection()) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PoolTestSupport.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PoolTestSupport.java index 5e1968b612..3200e49ec7 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PoolTestSupport.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PoolTestSupport.java @@ -16,6 +16,7 @@ */ package org.apache.tomcat.jdbc.functional; +import java.lang.reflect.Proxy; import java.sql.Connection; import java.sql.DriverManager; import java.sql.ResultSet; @@ -31,6 +32,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.JdbcInterceptor; import org.h2.tools.Server; /** @@ -141,6 +143,36 @@ public abstract class PoolTestSupport { return DriverManager.getConnection(url, "sa", ""); } + /** + * Finds an interceptor of the given type in the chain behind a + * borrowed connection. The objects the pool hands out are JDK proxies + * whose invocation handler is the first interceptor of the chain, so + * this is how a test reaches the pool's own internals without needing + * a test-only interceptor of its own. + */ + protected static <T extends JdbcInterceptor> T interceptorOf(Connection pooled, + Class<T> type) { + JdbcInterceptor interceptor = (JdbcInterceptor) Proxy.getInvocationHandler(pooled); + while (interceptor != null) { + if (type.isInstance(interceptor)) { + return type.cast(interceptor); + } + interceptor = interceptor.getNext(); + } + return null; + } + + /** + * The driver's own object behind a pooled connection's statement. For + * a prepared statement this is the honest oracle for "the session was + * not asked to prepare this SQL again": two handles onto the same + * instance mean one physical prepare, and {@code isClosed()} on it + * says what really happened to the statement. + */ + protected static Statement driverStatement(Statement pooled) throws SQLException { + return pooled.unwrap(Statement.class); + } + /** * Number of physical sessions on the database other than the admin * connection this method itself opens - i.e. the connections the diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/StatementCacheReuseTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/StatementCacheReuseTest.java new file mode 100644 index 0000000000..de567e3221 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/StatementCacheReuseTest.java @@ -0,0 +1,288 @@ +/* + * 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.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.CallableStatement; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.Statement; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.interceptor.StatementCache; + +/** + * Expected functionality of the pool's own statement cache: a prepared + * statement an application closes is kept for the next prepare of the + * same statement on the same physical session, so the session is not + * asked to prepare it a second time. That is the whole point of the + * feature, and the oracle here is the driver's own statement object + * behind the pool's proxy: two handles onto the same instance mean one + * physical prepare, and its {@code isClosed()} says what really happened + * to it. The interceptor's published counters + * ({@code getCacheSizePerConnection()}, {@code getCacheSize()}) and the + * rows that reach the database carry the rest. + * + * <p>The rules asserted below are the documented ones: statements are + * cached per connection, the {@code max} count is counted over the whole + * pool, and once that count is reached further statements are not cached + * and are closed immediately.</p> + */ +class StatementCacheReuseTest extends PoolTestSupport { + + private DataSource cachedPool(String properties, int maxActive) { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + StatementCache.class.getName() + "(" + properties + ")"); + ds.getPoolProperties().setMaxActive(maxActive); + return ds; + } + + /** The statement cache as the given borrower sees it. */ + private static StatementCache cacheOf(Connection pooled) { + return interceptorOf(pooled, StatementCache.class); + } + + @Test + void aClosedStatementServesTheNextBorrowerWithoutPreparingAgain() throws Exception { + DataSource ds = cachedPool("prepared=true,max=5", 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(); + setSessionMarker(first, 17); + PreparedStatement prepared = first.prepareStatement(insert); + Statement physical = driverStatement(prepared); + prepared.setInt(1, 1); + assertEquals(1, prepared.executeUpdate()); + prepared.close(); + assertEquals(1, cacheOf(first).getCacheSizePerConnection(), + "a closed statement must be waiting in the cache of its session"); + first.close(); + + Connection second = ds.getConnection(); + try { + assertEquals(Integer.valueOf(17), sessionMarker(second), + "the same physical session must have been reused"); + assertEquals(1, cacheOf(second).getCacheSizePerConnection(), + "the cache must survive the return to the pool"); + + PreparedStatement reused = second.prepareStatement(insert); + assertEquals(0, cacheOf(second).getCacheSizePerConnection(), + "preparing the same statement again must take it out of the cache"); + assertSame(physical, driverStatement(reused), + "the next borrower must get the same physical statement, not a " + + "second prepare on the session"); + + reused.setInt(1, 2); + assertEquals(1, reused.executeUpdate()); + reused.close(); + assertEquals(1, cacheOf(second).getCacheSizePerConnection(), + "the statement must go back into the cache when closed again"); + } finally { + second.close(); + } + assertEquals(2, adminRowCount(table), "both executes must have reached the database"); + } + + @Test + void eachSessionCachesItsOwnStatements() throws Exception { + DataSource ds = cachedPool("prepared=true,max=5", 2); + 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 (1)"; + + Connection first = ds.getConnection(); + Connection second = ds.getConnection(); + try { + assertEquals(2, poolSessions(), "two borrowers must hold two sessions"); + + PreparedStatement onFirst = first.prepareStatement(insert); + Statement physicalOnFirst = driverStatement(onFirst); + onFirst.executeUpdate(); + onFirst.close(); + + PreparedStatement onSecond = second.prepareStatement(insert); + Statement physicalOnSecond = driverStatement(onSecond); + onSecond.executeUpdate(); + onSecond.close(); + + assertEquals(1, cacheOf(first).getCacheSizePerConnection(), + "the first session caches its own statement"); + assertEquals(1, cacheOf(second).getCacheSizePerConnection(), + "the second session caches its own statement"); + assertEquals(2, cacheOf(first).getCacheSize().get(), + "the pool-wide count must see both"); + assertNotSame(physicalOnFirst, physicalOnSecond, + "a cached statement belongs to the session it was prepared on and " + + "must never be handed to another session"); + } finally { + first.close(); + second.close(); + } + assertEquals(2, adminRowCount(table)); + } + + @Test + void theCacheLimitIsCountedOverTheWholePool() throws Exception { + DataSource ds = cachedPool("prepared=true,max=1", 2); + ds.createPool(); + + Connection first = ds.getConnection(); + PreparedStatement onFirst = first.prepareStatement("SELECT 1"); + Statement physicalOnFirst = driverStatement(onFirst); + onFirst.close(); + assertEquals(1, cacheOf(first).getCacheSizePerConnection()); + assertEquals(1, cacheOf(first).getCacheSize().get()); + + // The limit is reached, so the second session's statement cannot be + // cached: the documentation says it is closed immediately. + Connection second = ds.getConnection(); + PreparedStatement onSecond = second.prepareStatement("SELECT 2"); + Statement physicalOnSecond = driverStatement(onSecond); + onSecond.close(); + assertTrue(physicalOnSecond.isClosed(), + "once the pool-wide count is at max, a further statement must not be " + + "cached but closed immediately"); + assertEquals(0, cacheOf(second).getCacheSizePerConnection()); + assertEquals(1, cacheOf(first).getCacheSize().get(), + "the count must not grow past max"); + + // Taking the cached statement out of the cache frees the slot for + // the other session. + PreparedStatement reused = first.prepareStatement("SELECT 1"); + assertSame(physicalOnFirst, driverStatement(reused), + "the first session's statement must still come from the cache"); + reused.close(); + assertEquals(1, cacheOf(first).getCacheSizePerConnection()); + assertEquals(1, cacheOf(first).getCacheSize().get(), + "putting it back must not exceed the limit either"); + + first.close(); + second.close(); + } + + @Test + void aStatementOverTheLimitIsRepreparedOnDemand() throws Exception { + DataSource ds = cachedPool("prepared=true,max=1", 1); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + StatementCache cache = cacheOf(pooled); + + PreparedStatement inCache = pooled.prepareStatement("SELECT 1"); + Statement physicalInCache = driverStatement(inCache); + inCache.close(); + assertEquals(1, cache.getCacheSizePerConnection()); + assertFalse(physicalInCache.isClosed(), + "a cached statement must stay open - that is what saves the prepare"); + + PreparedStatement overLimit = pooled.prepareStatement("SELECT 2"); + Statement physicalOverLimit = driverStatement(overLimit); + overLimit.close(); + assertTrue(physicalOverLimit.isClosed(), + "the statement that did not fit must be closed, not parked"); + assertEquals(1, cache.getCacheSizePerConnection(), + "the cache must not grow past the configured limit"); + + // and preparing that statement again works, on a fresh statement + PreparedStatement again = pooled.prepareStatement("SELECT 2"); + assertNotSame(physicalOverLimit, driverStatement(again), + "the closed statement cannot come back; a new one must be prepared"); + assertTrue(again.execute(), "the re-prepared statement must work"); + again.close(); + } + } + + @Test + void theCacheGoesAwayWithTheSessionItWasBuiltOn() throws Exception { + DataSource ds = cachedPool("prepared=true,max=5", 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 (1)"; + + Connection pooled = ds.getConnection(); + PreparedStatement prepared = pooled.prepareStatement(insert); + Statement physical = driverStatement(prepared); + prepared.executeUpdate(); + prepared.close(); + assertEquals(1, cacheOf(pooled).getCacheSizePerConnection()); + pooled.close(); + + // the physical session is retired, with everything hanging off it + ds.purge(); + try (Connection next = ds.getConnection()) { + assertEquals(0, cacheOf(next).getCacheSizePerConnection(), + "a fresh session must start with an empty statement cache"); + try (PreparedStatement fresh = next.prepareStatement(insert)) { + assertNotSame(physical, driverStatement(fresh), + "the statement of the retired session must not be reachable"); + assertEquals(1, fresh.executeUpdate(), + "the statement must simply be prepared again on the new session"); + } + } + assertEquals(2, adminRowCount(table)); + } + + @Test + void onlyTheStatementTypesTheConfigurationAsksForAreCached() throws Exception { + DataSource ds = cachedPool("prepared=false,callable=true,max=5", 1); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + StatementCache cache = cacheOf(pooled); + + PreparedStatement notCached = pooled.prepareStatement("SELECT 1"); + Statement physical = driverStatement(notCached); + notCached.close(); + assertEquals(0, cache.getCacheSizePerConnection(), + "prepared statements must not be cached when prepared=false"); + try (PreparedStatement again = pooled.prepareStatement("SELECT 1")) { + assertNotSame(physical, driverStatement(again), + "so preparing again means a real prepare"); + } + + CallableStatement cached = pooled.prepareCall("{call ROUND(2.5, 0)}"); + Statement physicalCallable = driverStatement(cached); + assertTrue(cached.execute(), "the procedure must run"); + cached.close(); + assertEquals(1, cache.getCacheSizePerConnection(), + "callable statements must be cached when callable=true"); + try (CallableStatement again = pooled.prepareCall("{call ROUND(2.5, 0)}")) { + assertSame(physicalCallable, driverStatement(again), + "and the cached one must be handed out again"); + } + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
