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 f3ca5ddaffcdde6e47a4c41ce2a4aaf7a50841f5 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 22:02:22 2026 +0100 Report the statement cache size as a value, not a shared counter The CacheSize attribute of the StatementCache interceptor MBean was declared as an AtomicInteger and the interceptor returned the very field the whole pool shares to enforce its max cache size. A client reading the attribute from inside the JVM received that object itself and could change the accounting it had only come to observe: inflating it stopped the pool from caching any further statement, deflating it let the cache grow beyond max for the lifetime of the pool. A remote client instead received a serialized snapshot, so the attribute meant two different things depending on the transport, and AtomicInteger is not a JMX open type for tooling that only handles those. The attribute is now a plain int, a snapshot of the pool's accounting at the moment of the read, and the pool-wide scope the count covers is stated in the interface. An interceptor instance that is not bound to a pooled connection reports -1 instead of the previous no value at all. The existing tests read the accessor directly and were updated to the new signature. TestStatementCacheJmxCacheSize drives the published interceptor MBean through the platform MBeanServer and pins that the attribute is described as a number, that a read is not a write path into the pool's accounting, and the value reported when unbound; all three fail on the previous implementation. --- .../jdbc/pool/interceptor/StatementCache.java | 5 +- .../jdbc/pool/interceptor/StatementCacheMBean.java | 14 +- .../TestStatementCacheJmxCacheSize.java | 201 +++++++++++++++++++++ .../pool/interceptor/TestStatementCacheReuse.java | 8 +- webapps/docs/changelog.xml | 16 ++ 5 files changed, 233 insertions(+), 11 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 c40226810b..e935916eb9 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 @@ -118,8 +118,9 @@ public class StatementCache extends StatementDecoratorInterceptor implements Sta } @Override - public AtomicInteger getCacheSize() { - return cacheSize; + public int getCacheSize() { + AtomicInteger size = this.cacheSize; + return size == null ? -1 : size.get(); } @Override 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 6627f41c50..a087f6db11 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 @@ -16,8 +16,6 @@ */ package org.apache.tomcat.jdbc.pool.interceptor; -import java.util.concurrent.atomic.AtomicInteger; - /** * MBean interface for monitoring the statement cache. */ @@ -38,10 +36,16 @@ public interface StatementCacheMBean { */ int getMaxCacheSize(); /** - * Returns the current global cache size across all connections. - * @return current cache size counter + * Returns the number of statements currently held in the statement + * cache, counted across all connections that belong to the same + * connection pool - the value the {@code max} limit is compared + * against. The number is a snapshot of the pool's accounting at the + * moment of the read; it is not a handle that can be used to change + * that accounting. + * @return the number of cached statements, or {@code -1} if this + * interceptor instance is not bound to a pooled connection */ - AtomicInteger getCacheSize(); + int getCacheSize(); /** * Returns the cache size for the current connection. * @return number of cached statements for this connection diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheJmxCacheSize.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheJmxCacheSize.java new file mode 100644 index 0000000000..2fefd43c9b --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheJmxCacheSize.java @@ -0,0 +1,201 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool.interceptor; + +import java.lang.management.ManagementFactory; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.Statement; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; + +import javax.management.MBeanAttributeInfo; +import javax.management.MBeanInfo; +import javax.management.MBeanServer; +import javax.management.ObjectName; + +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 {@code CacheSize} attribute of the {@code StatementCache} + * interceptor MBean as a JMX client sees it. The count it reports comes + * from the counter the whole pool shares to enforce the {@code max} limit, + * and the attribute is the only place where that internal state crosses + * the management boundary. A monitoring read must therefore yield a + * number: a client that reads it from inside the JVM and ends up holding + * the counter itself can write through it and permanently skew the + * accounting of the pool it only came to look at. A number is also what a + * client limited to JMX open types needs in order to represent the value. + * <p> + * The interceptor publishes one MBean per pooled connection, below the + * pool's own MBean, so these tests drive the registration the way a + * container does: publish the DataSource, then let the pool hand out a + * connection. + */ +class TestStatementCacheJmxCacheSize extends JdbcPoolBaseTest { + + private static ObjectName uniqueDataSourceName() throws Exception { + return new ObjectName("tomcat:type=StatementCacheTestDataSource,uid=" + + System.nanoTime()); + } + + private static Set<ObjectName> poolMBeans(MBeanServer server) throws Exception { + return server.queryNames(new ObjectName("tomcat.jdbc:*"), null); + } + + /* A pool that caches prepared statements and publishes its MBeans. */ + private DataSource cachingPool(String properties) throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJmxEnabled(true); + ds.setJdbcInterceptors(StatementCache.class.getName() + properties); + ds.getPoolProperties().setInitialSize(0); + ds.createPool(); + return ds; + } + + /* + * The interceptor MBeans that appeared since the given snapshot - one + * for each pooled connection that has been handed out meanwhile. + */ + private static Set<ObjectName> interceptorMBeans(MBeanServer server, + Set<ObjectName> before) throws Exception { + Set<ObjectName> added = new HashSet<>(poolMBeans(server)); + added.removeAll(before); + added.removeIf(name -> name.getKeyProperty("JdbcInterceptor") == null); + return added; + } + + @Test + void theCacheSizeAttributeIsADescribedNumber() throws Exception { + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + DataSource ds = cachingPool("(prepared=true,max=5)"); + Set<ObjectName> before = poolMBeans(server); + ds.preRegister(server, uniqueDataSourceName()); + + try (Connection con = ds.getConnection()) { + Set<ObjectName> interceptors = interceptorMBeans(server, before); + Assertions.assertEquals(1, interceptors.size(), + "the pooled connection must have published one interceptor MBean, found: " + + interceptors); + ObjectName oname = interceptors.iterator().next(); + + MBeanInfo info = server.getMBeanInfo(oname); + MBeanAttributeInfo cacheSize = null; + for (MBeanAttributeInfo attribute : info.getAttributes()) { + if ("CacheSize".equals(attribute.getName())) { + cacheSize = attribute; + } + } + Assertions.assertNotNull(cacheSize, "CacheSize must stay an attribute of the " + + "interceptor MBean, which exposes " + + info.getAttributes().length + " attributes"); + Assertions.assertEquals(int.class.getName(), cacheSize.getType(), + "the attribute must be described as a number, so that a client limited to " + + "JMX open types can represent it, but its type is: " + + cacheSize.getType()); + Assertions.assertFalse(cacheSize.isWritable(), + "the pool's cache accounting is not something a management client configures"); + + PreparedStatement cached = con.prepareStatement("SELECT 1"); + cached.close(); + + Object read = server.getAttribute(oname, "CacheSize"); + Assertions.assertInstanceOf(Integer.class, read, + "a monitoring read must yield the number of cached statements rather than " + + "an object the client can change"); + Assertions.assertEquals(1, read, + "the statement that has just been closed is cached and must be counted"); + } finally { + ds.close(); + ds.postDeregister(); + } + } + + @Test + void readingCacheSizeCannotChangeWhatThePoolCaches() throws Exception { + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + DataSource ds = cachingPool("(prepared=true,max=2)"); + Set<ObjectName> before = poolMBeans(server); + ds.preRegister(server, uniqueDataSourceName()); + + try (Connection con = ds.getConnection()) { + StatementCache cache = interceptorOf(con, StatementCache.class); + ObjectName oname = interceptorMBeans(server, before).iterator().next(); + + con.prepareStatement("SELECT 1").close(); + Assertions.assertEquals(1, cache.getCacheSizePerConnection(), + "the first statement must have been cached"); + + // Inflate: a client that came for a number, but which ends up + // holding the pool's own counter, writes to what it read. + Object inflated = server.getAttribute(oname, "CacheSize"); + if (inflated instanceof AtomicInteger) { + ((AtomicInteger) inflated).set(1000); + } + PreparedStatement second = con.prepareStatement("SELECT 2"); + Statement physicalSecond = driverStatement(second); + second.close(); + Assertions.assertFalse(physicalSecond.isClosed(), + "reading the attribute must not stop the pool caching: a count inflated past " + + "max closes every further statement instead of recycling it"); + Assertions.assertEquals(2, cache.getCacheSizePerConnection(), + "the second statement must still have reached the cache"); + + // Deflate: the same client writing downwards must not lift the + // limit either - that would let the cache grow past max. + Object deflated = server.getAttribute(oname, "CacheSize"); + if (deflated instanceof AtomicInteger) { + ((AtomicInteger) deflated).set(-1000); + } + con.prepareStatement("SELECT 3").close(); + con.prepareStatement("SELECT 4").close(); + Assertions.assertEquals(2, cache.getCacheSizePerConnection(), + "reading the attribute must not let the cache grow past max=2, it holds: " + + cache.getCacheSizePerConnection()); + Assertions.assertEquals(2, + ((Number) server.getAttribute(oname, "CacheSize")).intValue(), + "the count the attribute reports must stay the one the limit is enforced with"); + } finally { + ds.close(); + ds.postDeregister(); + } + } + + @Test + void anInterceptorBoundToNoConnectionReportsMinusOne() throws Exception { + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = new ObjectName("tomcat:type=UnboundStatementCache,uid=" + + System.nanoTime()); + StatementCache unbound = new StatementCache(); + server.registerMBean(unbound, oname); + try { + // This instance has never been given a pooled connection, so + // there is no pool whose accounting it could report; 0 would be + // indistinguishable from an empty cache. + Assertions.assertEquals(-1, server.getAttribute(oname, "CacheSize"), + "an interceptor that is not bound to a pooled connection must report the " + + "documented sentinel instead of a value that looks like a count"); + } finally { + server.unregisterMBean(oname); + } + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java index 0eda0d3a2f..98d66a512b 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementCacheReuse.java @@ -231,7 +231,7 @@ class TestStatementCacheReuse extends JdbcPoolBaseTest { "the first session caches its own statement"); Assertions.assertEquals(1, cacheOf(second).getCacheSizePerConnection(), "the second session caches its own statement"); - Assertions.assertEquals(2, cacheOf(first).getCacheSize().get(), + Assertions.assertEquals(2, cacheOf(first).getCacheSize(), "the pool-wide count must see both"); Assertions.assertNotSame(physicalOnFirst, physicalOnSecond, "a cached statement belongs to the session it was prepared on and " @@ -253,7 +253,7 @@ class TestStatementCacheReuse extends JdbcPoolBaseTest { Statement physicalOnFirst = driverStatement(onFirst); onFirst.close(); Assertions.assertEquals(1, cacheOf(first).getCacheSizePerConnection()); - Assertions.assertEquals(1, cacheOf(first).getCacheSize().get()); + Assertions.assertEquals(1, cacheOf(first).getCacheSize()); // The limit is reached, so the second session's statement cannot be // cached: the documentation says it is closed immediately. @@ -265,7 +265,7 @@ class TestStatementCacheReuse extends JdbcPoolBaseTest { "once the pool-wide count is at max, a further statement must not be " + "cached but closed immediately"); Assertions.assertEquals(0, cacheOf(second).getCacheSizePerConnection()); - Assertions.assertEquals(1, cacheOf(first).getCacheSize().get(), + Assertions.assertEquals(1, cacheOf(first).getCacheSize(), "the count must not grow past max"); // Taking the cached statement out of the cache frees the slot for @@ -275,7 +275,7 @@ class TestStatementCacheReuse extends JdbcPoolBaseTest { "the first session's statement must still come from the cache"); reused.close(); Assertions.assertEquals(1, cacheOf(first).getCacheSizePerConnection()); - Assertions.assertEquals(1, cacheOf(first).getCacheSize().get(), + Assertions.assertEquals(1, cacheOf(first).getCacheSize(), "putting it back must not exceed the limit either"); first.close(); diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index c1dc9bbc6e..c0c63f64b4 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1182,6 +1182,22 @@ <code>IllegalStateException</code> whenever statistics were present. (remm) </fix> + <fix> + The <code>CacheSize</code> attribute of the + <code>StatementCache</code> interceptor MBean is now a plain + <code>int</code>. It used to be a + <code>java.util.concurrent.atomic.AtomicInteger</code>, which + meant that a client reading the attribute from inside the JVM + received the very object the pool uses to enforce its + <code>max</code> cache size, and could change the accounting it + had only come to observe: inflating it stopped the pool from + caching any further statement, deflating it let the cache grow + beyond <code>max</code>. A remote client, on the other hand, + received a snapshot that its writes could not affect. The value + reported by an interceptor instance that is not bound to a pooled + connection is now <code>-1</code> instead of no value at all. + (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
