This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 577f4e0f1e7123e484438f8dd6def341d372845c Author: opencode <[email protected]> AuthorDate: Fri Oct 30 09:11:22 2026 +0100 Populate the statement proxy constructor cache in the class initializer getConstructor served its shared static constructor cache from an unsynchronized check-then-act, so threads first hitting the same index concurrently each computed and stored their own proxy constructor and a reader could keep seeing a stale null with no happens-before; the outcomes were equivalent, so the cost was duplicated reflection work rather than a wrong result, but it contradicted the deliberate volatile treatment of the sibling cache in StatementDecoratorInterceptor. Rather than lock the per-statement hot path, the cache's four canonical proxy constructors - Statement, PreparedStatement, CallableStatement and ResultSet - are now computed once by the class initializer, which the JVM runs once per class loader and whose writes it publishes to every user of the class, leaving the normal path a plain, safely published array read. A slot that cannot be populated at initialization is logged and falls back to the pre-existing on-demand computation instead of failing class initialization, and a concurrent-caller test plus a class-initialization publication test pin the new behavior. --- .../AbstractCreateStatementInterceptor.java | 57 +++++++++++- .../TestAbstractCreateStatementInterceptor.java | 100 ++++++++++++++++++++- 2 files changed, 153 insertions(+), 4 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractCreateStatementInterceptor.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractCreateStatementInterceptor.java index 7fb5d33906..6bd27ccc7b 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractCreateStatementInterceptor.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractCreateStatementInterceptor.java @@ -20,10 +20,14 @@ import java.lang.reflect.Constructor; import java.lang.reflect.InvocationHandler; import java.lang.reflect.Method; import java.lang.reflect.Proxy; +import java.sql.CallableStatement; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.Statement; import java.util.Arrays; +import org.apache.juli.logging.Log; +import org.apache.juli.logging.LogFactory; import org.apache.tomcat.jdbc.pool.ConnectionPool; import org.apache.tomcat.jdbc.pool.JdbcInterceptor; import org.apache.tomcat.jdbc.pool.PooledConnection; @@ -34,6 +38,8 @@ import org.apache.tomcat.jdbc.pool.PooledConnection; * {@link #createStatement(Object, Method, Object[], Object, long)} method. */ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor { + private static final Log log = LogFactory.getLog(AbstractCreateStatementInterceptor.class); + /** * {@link java.sql.Connection#createStatement()} */ @@ -125,10 +131,50 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor * shared by all subclasses loaded through the same class loader, so every * use of an index must pass an interface that the cached proxy implements, * in practice always the same one; {@link #getConstructor(int, Class)} - * enforces this. + * enforces this. The canonical entries are populated by this class's + * initializer, which the JVM runs once and whose writes it publishes to + * every thread that uses the class, so reading a slot through + * {@link #getConstructor(int, Class)} needs no locking; writing an element + * directly from a subclass bypasses that guarantee and is the subclass's + * own responsibility. */ protected static final Constructor<?>[] constructors = new Constructor[STATEMENT_TYPE_COUNT + 1]; + static { + // Class initialization (JLS 12.4.2) runs once per class loader and its + // writes are visible to every thread once it completes, so populating + // the canonical slots here makes them safely published without any + // monitor on the hot statement path. + populateConstructor(CREATE_STATEMENT_IDX, Statement.class); + populateConstructor(PREPARE_STATEMENT_IDX, PreparedStatement.class); + populateConstructor(PREPARE_CALL_IDX, CallableStatement.class); + populateConstructor(RESULTSET_IDX, ResultSet.class); + } + + /** + * Computes the proxy constructor for one index of {@link #constructors} + * and stores it in the cache. On failure the slot is left empty and only + * logged; {@link #getConstructor(int, Class)} then computes the entry on + * demand, so a failure here degrades to the pre-existing behavior rather + * than failing class initialization. + * + * @param idx - the index of the constructor + * @param clazz - the interface that the proxy will implement + */ + @SuppressWarnings("deprecation") + private static void populateConstructor(int idx, Class<?> clazz) { + try { + Class<?> proxyClass = Proxy.getProxyClass(AbstractCreateStatementInterceptor.class.getClassLoader(), + new Class[] { clazz }); + constructors[idx] = proxyClass.getConstructor(new Class[] { InvocationHandler.class }); + } catch (ReflectiveOperationException | IllegalArgumentException | LinkageError e) { + // An exception escaping the class initializer would surface as an + // ExceptionInInitializerError at every use of every subclass. + log.warn("Unable to create the constructor cache entry for index " + idx + " (" + clazz.getName() + + ").", e); + } + } + /** * Default constructor */ @@ -156,7 +202,11 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor } /** - * Creates a constructor for a proxy class, if one doesn't already exist + * Creates a constructor for a proxy class, if one doesn't already exist. + * The canonical entries are published by this class's initializer, so on + * the normal path the method only reads a safely published slot and is + * safe for concurrent use; {@code newInstance} on the returned + * constructor runs as it always has, outside this method. * * @param idx - the index of the constructor * @param clazz - the interface that the proxy will implement @@ -180,6 +230,9 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor @SuppressWarnings("deprecation") protected Constructor<?> getConstructor(int idx, Class<?> clazz) throws NoSuchMethodException { if (constructors[idx] == null) { + // Only reachable if the class initializer failed to populate this + // slot, or a subclass nulled the element directly; compute on + // demand, as before the cache was populated eagerly. Class<?> proxyClass = Proxy.getProxyClass(AbstractCreateStatementInterceptor.class.getClassLoader(), new Class[] { clazz }); constructors[idx] = proxyClass.getConstructor(new Class[] { InvocationHandler.class }); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestAbstractCreateStatementInterceptor.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestAbstractCreateStatementInterceptor.java index d0e51e757d..d3bf9c2f88 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestAbstractCreateStatementInterceptor.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestAbstractCreateStatementInterceptor.java @@ -23,6 +23,9 @@ import java.sql.CallableStatement; import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.Statement; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -38,8 +41,8 @@ import org.junit.jupiter.api.Test; * surfaced later, as a {@code ClassCastException} against the application * or as a silently degraded, unproxied statement. * - * <p>The tests below only ever populate an index with the interface every - * in-tree call site pairs with it, and they leave the cache in that state, + * <p>The class initializer populates the cache with the canonical pairings; + * the tests below only ever use those pairings and never replace an entry, * so running them in the same JVM as the rest of the suite - and in any * method order - is safe.</p> */ @@ -123,4 +126,97 @@ class TestAbstractCreateStatementInterceptor { Assertions.assertSame(cached, interceptor.getConstructor(idx, Statement.class)); Assertions.assertTrue(newProxy(interceptor, idx, Statement.class) instanceof Statement); } + + /* + * Regression for the unsynchronized check-then-act on the cache: threads + * first hitting the same index used to race, each computing its own + * constructor and handing out a different (though equivalent) object, + * with no guarantee a reader ever saw a prior write. The canonical + * entries are now published once by the class initializer, which the JVM + * runs once and whose writes it makes visible to every thread, so every + * concurrent caller of one index must receive the very same constructor + * object. + * + * The assertion holds in any method order - a test that ran earlier can + * only have read these same published entries - and it asserts the + * outcome the published cache guarantees, never a timing-dependent count + * of computations, so it is deterministic. + */ + @Test + void concurrentCallersOfOneIndexAllReceiveTheIdenticalConstructor() throws Exception { + int rounds = 32; + int threadsPerRound = 8; + Class<?>[] canonical = { Statement.class, PreparedStatement.class, CallableStatement.class, ResultSet.class }; + + for (int round = 0; round < rounds; round++) { + final int idx = round % canonical.length; + final Class<?> iface = canonical[idx]; + + final CountDownLatch start = new CountDownLatch(1); + final CountDownLatch done = new CountDownLatch(threadsPerRound); + final Constructor<?>[] results = new Constructor<?>[threadsPerRound]; + final AtomicReference<Throwable> failure = new AtomicReference<>(); + + try { + for (int t = 0; t < threadsPerRound; t++) { + final int slot = t; + Thread thread = new Thread(() -> { + try { + start.await(); + // Distinct interceptor instances: the cache is + // shared through the class loader, not the + // instance. + results[slot] = new CacheUser().getConstructor(idx, iface); + } catch (Throwable x) { + failure.compareAndSet(null, x); + } finally { + done.countDown(); + } + }); + thread.start(); + } + } finally { + // Release the threads even if a later start() failed, so a + // failure cannot leak threads parked on the start latch. + start.countDown(); + } + Assertions.assertTrue(done.await(30, TimeUnit.SECONDS), "racing callers did not finish"); + + Assertions.assertNull(failure.get(), "racing callers must not fail: " + failure.get()); + for (int t = 0; t < threadsPerRound; t++) { + Assertions.assertSame(results[0], results[t], + "every concurrent caller of index " + idx + " must receive the same constructor"); + } + Object proxy = results[0].newInstance(new Object[] { (InvocationHandler) (p, m, a) -> null }); + Assertions.assertTrue(iface.isInstance(proxy), + "the shared constructor must produce a proxy implementing " + iface.getName()); + } + } + + /* + * The canonical entries exist before any call to getConstructor: reading + * the cache directly, without going through the method, observes what + * class initialization published. The assertions are stable in any + * method order - no test can replace a published entry, since every + * getConstructor call uses the canonical pairing, which only ever reads + * a populated slot - but the test can only distinguish eager from lazy + * population when it reads the slots before anything in this JVM has + * called getConstructor; a cache warmed by an earlier test satisfies + * the same assertions either way. + */ + @Test + void theCanonicalEntriesArePublishedByClassInitialization() throws Exception { + Class<?>[] canonical = { Statement.class, PreparedStatement.class, CallableStatement.class, ResultSet.class }; + InvocationHandler handler = (proxy, method, args) -> null; + + for (int idx = 0; idx < canonical.length; idx++) { + Constructor<?> published = AbstractCreateStatementInterceptor.constructors[idx]; + Assertions.assertNotNull(published, + "index " + idx + " must hold an entry once the class is initialized"); + Assertions.assertSame(published, new CacheUser().getConstructor(idx, canonical[idx]), + "getConstructor must hand out the entry published for index " + idx); + Assertions.assertTrue(canonical[idx].isInstance(published.newInstance(new Object[] { handler })), + "the published constructor must produce a proxy implementing " + canonical[idx].getName()); + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
