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 8d5fb742e5e870dfce9707b50e58398845c824fd Author: opencode <[email protected]> AuthorDate: Tue Nov 3 12:38:40 2026 +0100 Harden statement-proxy creation in the interceptor base Four defects in AbstractCreateStatementInterceptor, the shared base of every statement-wrapping interceptor, found by a from-scratch review and fixed together; a subsequent blank-slate review of the resulting class found nothing further to fix. None of the four has an in-tree trigger in the default configuration; all sit on the public extension surface or on exceptional paths. A throwing createStatement() hook used to propagate its throwable while leaving the statement the base had just created unreachable and open on the pooled connection, an orphaned server-side cursor or prepared statement for the life of the physical session, because nothing below guaranteed its close: the always-installed StatementFacade rethrows Errors rather than closing, and the StatementFinalizer that would track and reclaim it is not configured by default. The base now closes the statement best-effort when the hook fails, before propagating the original throwable unchanged, attaching a failure of that close as suppressed rather than masking the hook's own failure. The lazy fallback in getConstructor() filled a slot of the shared static constructor cache from whichever borrower thread reached it first. The class-initializer writes to that cache are safely published per JLS 12.4.2, but this single write had no happens-before edge to the readers of the other slots, a data race that also let racing callers hand out different Constructor objects. The initializer is now the only writer of the cache at all: an empty slot is served by computing a constructor and returning it uncached, leaving the safe-publication argument unconditional. The cache itself, previously a protected array every subclass could write from any thread, is now private, with getConstructor its sole accessor. closeInvoked() ran before the close was delegated, so a hook that threw short-circuited the close before it reached the chain terminal: the session was never returned to the pool and stayed busy - recoverable only through the abandoned-connection reaper, which is off by default - while the application handle was already disposed, leaving no way even to retry the close. The base now isolates the hook the way the pool isolates the reset and disconnected fan-outs: fatal errors propagate through ExceptionUtils.handleThrowable(), anything else is logged and the close travels on, so the hooks below a thrower still run and the borrow always ends. The elapsed time handed to the createStatement() hook was bracketed by two System.currentTimeMillis() readings but is booked as a duration, so an NTP step or a VM suspend could make it negative or absurdly large in the prepare-time statistics. It is now measured on the monotonic clock via the elapsedMillis() helper, lifted from AbstractQueryReport where the same hazard had already been fixed for execute timing, so the tree shares one convention for measuring a duration. Closing the review, the RESULTSET_IDX javadoc corrected (it indexes the cache's extra ResultSet slot, not STATEMENT_TYPES), the class javadoc given the missing close and reset halves of the subclass contract, the bogus "used for recursion" accumulator wording removed, and the stale deprecation comment on the cached proxy-class lookup replaced with the real tradeoff against Proxy.newProxyInstance. The touched classes build javadoc clean. --- .../AbstractCreateStatementInterceptor.java | 210 ++++++++++--- .../jdbc/pool/interceptor/AbstractQueryReport.java | 19 -- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../TestAbstractCreateStatementInterceptor.java | 333 ++++++++++++++++++++- .../TestCloseInvokedFailureIsolated.java | 110 +++++++ 5 files changed, 602 insertions(+), 71 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 df48876908..224209a125 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 @@ -31,12 +31,28 @@ 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; +import org.apache.tomcat.jdbc.pool.util.ExceptionUtils; import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** * Abstraction interceptor. This component intercepts all calls to create some type of SQL statement. By extending this * class, one can intercept queries and update statements by overriding the * {@link #createStatement(Object, Method, Object[], Object, long)} method. + * <p> + * Subclass contract. The base intercepts three things on the connection proxy, and a subclass participates in each of + * them:</p> + * <ul> + * <li>the statement creation path, through {@link #createStatement}, which receives the created statement and the + * monotonic elapsed time and returns what the application will hold;</li> + * <li>the close path, through {@link #closeInvoked()}, fired on every {@link java.sql.Connection#close()} before the + * close reaches the pool, while the connection is still alive;</li> + * <li>the borrow-start path: this base holds no per-borrow state and its {@link #reset} is a no-op, so any per-borrow + * state a subclass keeps must be cleared by the subclass's own reset override - the pool fans out reset, not + * anything in this class.</li> + * </ul> + * <p> + * Proxy classes come from the constructor cache described at {@link #getConstructor(int, Class)}; each index there is + * bound to one interface and a subclass must keep it that way.</p> */ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor { private static final Log log = LogFactory.getLog(AbstractCreateStatementInterceptor.class); @@ -82,7 +98,11 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor protected static final String GET_STATEMENT = "getStatement"; /** - * Index for {@link #GET_STATEMENT} in {@link #STATEMENT_TYPES} + * Index in {@link #constructors} of the {@link ResultSet} proxy + * constructor. This is not an index into {@link #STATEMENT_TYPES}: the + * cache carries one slot beyond the statement types, for the result set + * handles of subclasses that wrap statement-level calls such as + * {@link Statement#getResultSet()} and {@link ResultSet#getStatement()}. */ protected static final int RESULTSET_IDX = 3; @@ -128,18 +148,49 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor EXECUTE_LARGE_UPDATE, EXECUTE_LARGE_BATCH }; /** - * the constructors that are used to create statement proxies. The cache is - * 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. 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]; + * The constructors used to create statement proxies, one slot per index: + * {@link #CREATE_STATEMENT_IDX}, {@link #PREPARE_STATEMENT_IDX}, + * {@link #PREPARE_CALL_IDX} and {@link #RESULTSET_IDX}. The cache is + * shared by all subclasses loaded through the same class loader. Its only + * writer is 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 needs no locking. Subclasses reach the entries only through + * {@link #getConstructor(int, Class)}, which enforces that every use of an + * index passes an interface the cached proxy implements. + */ + private static final Constructor<?>[] constructors = new Constructor[STATEMENT_TYPE_COUNT + 1]; + + /** + * Reads one slot of the constructor cache. Package-private rather than + * protected: getConstructor is the accessor for subclasses, in-tree or + * third party, and this exists only so the tests in this package can + * observe what the initializer published. + * + * @param idx the index of the slot + * + * @return the constructor cached for the index, or null if the initializer + * failed to populate it + */ + static Constructor<?> cachedConstructor(int idx) { + return constructors[idx]; + } + + /** + * Replaces one slot of the constructor cache, returning what was there. + * The write counterpart of {@link #cachedConstructor(int)}: package-private + * and used only by the tests in this package, to simulate a slot the class + * initializer failed to populate. + * + * @param idx the index of the slot + * @param replacement what to store, possibly null + * + * @return the constructor that was in the slot + */ + static Constructor<?> replaceCachedConstructor(int idx, Constructor<?> replacement) { + Constructor<?> previous = constructors[idx]; + constructors[idx] = replacement; + return previous; + } static { // Class initialization (JLS 12.4.2) runs once per class loader and its @@ -155,9 +206,9 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor /** * 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. + * logged; {@link #getConstructor(int, Class)} then computes a constructor + * on demand without caching it, 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 @@ -186,16 +237,45 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor @Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { if (compare(CLOSE_VAL, method)) { - closeInvoked(); + try { + closeInvoked(); + } catch (Throwable t) { + // Propagate the fatal errors the module never swallows, then + // isolate the rest: the borrow must end whatever a hook makes + // of it, because the application handle is already disposed at + // this point, so a throw here would strand the session as busy + // with no way to retry the close. + ExceptionUtils.handleThrowable(t); + log.warn(MessageFactory.get("abstractCreateStatementInterceptor.closeInvokedFailed", + getClass().getName()), t); + } return super.invoke(proxy, method, args); } else { - boolean process = false; - process = isStatement(method, process); - if (process) { - long start = System.currentTimeMillis(); + if (isStatement(method, false)) { + long start = System.nanoTime(); Object statement = super.invoke(proxy, method, args); - long delta = System.currentTimeMillis() - start; - return createStatement(proxy, method, args, statement, delta); + long delta = elapsedMillis(start); + try { + return createStatement(proxy, method, args, statement, delta); + } catch (Throwable t) { + // The application never receives the statement when the + // hook fails, so do not orphan it on the pooled + // connection. See the createStatement() contract below. + if (statement instanceof Statement) { + try { + ((Statement) statement).close(); + } catch (Throwable closeFailure) { + // Best effort: never mask the hook's failure, but + // propagate the fatal errors the module never + // swallows. + ExceptionUtils.handleThrowable(closeFailure); + if (closeFailure != t) { + t.addSuppressed(closeFailure); + } + } + } + throw t; + } } else { return super.invoke(proxy, method, args); } @@ -203,11 +283,34 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor } /** - * 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. + * Measures the milliseconds elapsed since the {@link System#nanoTime()} + * reading taken at {@code startNanos}, rounded to the nearest millisecond. + * <p> + * An interval is measured with the monotonic clock. Had it been measured + * with the wall clock instead, an NTP correction, a manual set, or + * resuming a suspended VM between the two readings could have made the + * difference negative, and a negative duration is never over the slow + * query threshold while the statistics book it as if it were real. + * </p> + * + * @param startNanos the {@link System#nanoTime()} reading the measurement began with + * + * @return the elapsed time in milliseconds + */ + protected static long elapsedMillis(long startNanos) { + return (System.nanoTime() - startNanos + 500000) / 1000000; + } + + /** + * Hands out the proxy constructor cached for this index. The method is + * safe for concurrent use: the only writer of the cache is this class's + * initializer, whose writes the JVM publishes to every thread that uses + * the class, so reading a slot needs no locking; {@code newInstance} on + * the returned constructor runs as it always has, outside this method. If + * the initializer failed to populate the index, a constructor for the + * requested interface is computed on demand and returned without being + * cached, so such calls return equivalent, but not necessarily identical, + * objects. * * @param idx - the index of the constructor * @param clazz - the interface that the proxy will implement @@ -220,29 +323,32 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor * an interface that the cached proxy implements. */ /* - * The cache, this method and the class itself are all reachable from subclasses, in-tree or third party. The - * cache is shared by every subclass loaded through the same class loader, so each index must always be - * paired with an interface that the cached proxy implements, in practice always the same one; a cache hit - * whose cached proxy does not implement the requested interface is rejected below rather than silently - * returning a proxy that does not implement what the caller asked for. Given the comments in the jdbc-pool - * code regarding caching for performance, continue to use Proxy.getProxyClass(). This will need to be - * revisited if that method is marked for removal. + * The pool's comments argue for caching the proxy class, so continue to use Proxy.getProxyClass(): the drop-in + * alternative, Proxy.newProxyInstance, would rebuild the proxy class lookup on every statement creation instead + * of reading a cache slot. getProxyClass has been deprecated since Java 9 without being marked for removal, so + * the deprecation suppression is sufficient; should that ever flip, the removal lint and this comment will say + * so and the caching argument must be re-weighed against the replacement. */ @SuppressWarnings("deprecation") protected Constructor<?> getConstructor(int idx, Class<?> clazz) throws NoSuchMethodException { - if (constructors[idx] == null) { + Constructor<?> cached = constructors[idx]; + if (cached == 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. + // slot. Compute the constructor but deliberately do not store it: + // a write from an arbitrary thread would be the only write outside + // the class initializer and would defeat the safe publication of + // the other entries, while the JDK's own proxy class cache keeps + // the repeated computation to a lookup. Class<?> proxyClass = Proxy.getProxyClass(AbstractCreateStatementInterceptor.class.getClassLoader(), new Class[] { clazz }); - constructors[idx] = proxyClass.getConstructor(new Class[] { InvocationHandler.class }); - } else if (!clazz.isAssignableFrom(constructors[idx].getDeclaringClass())) { + return proxyClass.getConstructor(new Class[] { InvocationHandler.class }); + } + if (!clazz.isAssignableFrom(cached.getDeclaringClass())) { throw new IllegalStateException(MessageFactory.get( "abstractCreateStatementInterceptor.constructorCacheIndexPopulated", - idx, Arrays.toString(constructors[idx].getDeclaringClass().getInterfaces()), clazz.getName())); + idx, Arrays.toString(cached.getDeclaringClass().getInterfaces()), clazz.getName())); } - return constructors[idx]; + return cached; } /** @@ -250,19 +356,29 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor * around the statement or return the statement itself. If this method returns a wrapper then it should return a * wrapper object that implements one of the following interfaces. {@link java.sql.Statement}, * {@link java.sql.PreparedStatement} or {@link java.sql.CallableStatement} + * <p> + * An implementation must not make the statement reachable from any other code before it returns, because if this + * method throws then the statement is closed on a best-effort basis by the interceptor base class before the + * throwable is propagated to the application. * * @param proxy the actual proxy object * @param method the method that was called. It will be one of the methods defined in {@link #STATEMENT_TYPES} * @param args the arguments to the method * @param statement the statement that the underlying connection created - * @param time Elapsed time + * @param time elapsed time in milliseconds, measured with the monotonic clock * * @return a {@link java.sql.Statement} object */ public abstract Object createStatement(Object proxy, Method method, Object[] args, Object statement, long time); /** - * Method invoked when the operation {@link java.sql.Connection#close()} is invoked. + * Method invoked when the operation {@link java.sql.Connection#close()} is invoked. It runs before the close is + * passed down the chain, so that a hook can still reach the connection while it is alive - statements must be + * closed before the session is handed to the next borrower. Implementation of this method should be thought + * through with care, as no actions should trigger an exception. An interceptor that nevertheless throws is + * isolated: the exception is logged and ignored, the remaining interceptors in the chain still receive the close, + * and the connection is returned to the pool as it would have been. The fatal virtual machine errors the pool + * never swallows are the exception: those propagate. */ public abstract void closeInvoked(); @@ -270,7 +386,7 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor * Returns true if the method that is being invoked matches one of the statement types. * * @param method the method being invoked on the proxy - * @param process boolean result used for recursion + * @param process initial accumulator value, pass <code>false</code> * * @return returns true if the method name matched */ @@ -282,7 +398,7 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor * Returns true if the method that is being invoked matches one of the execute types. * * @param method the method being invoked on the proxy - * @param process boolean result used for recursion + * @param process initial accumulator value, pass <code>false</code> * * @return returns true if the method name matched */ @@ -295,7 +411,7 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor * * @param names list of method names that we want to intercept * @param method the method being invoked on the proxy - * @param process boolean result used for recursion + * @param process initial accumulator value, pass <code>false</code> * * @return returns true if the method name matched */ diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java index f9d56c7a4e..30de75c849 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractQueryReport.java @@ -217,25 +217,6 @@ public abstract class AbstractQueryReport extends AbstractCreateStatementInterce return statement; } - /** - * Measures the milliseconds elapsed since the {@link System#nanoTime()} - * reading taken at {@code startNanos}, rounded to the nearest millisecond. - * <p> - * An interval is measured with the monotonic clock. Had it been measured - * with the wall clock instead, an NTP correction, a manual set, or - * resuming a suspended VM between the two readings could have made the - * difference negative, and a negative duration is never over the slow - * query threshold while the statistics book it as if it were real. - * </p> - * - * @param startNanos the {@link System#nanoTime()} reading the measurement began with - * - * @return the elapsed time in milliseconds - */ - private static long elapsedMillis(long startNanos) { - return (System.nanoTime() - startNanos + 500000) / 1000000; - } - /** * Class to measure query execute time */ 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 fdc34456d3..299de84097 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 @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +abstractCreateStatementInterceptor.closeInvokedFailed=Interceptor [{0}] threw an exception from closeInvoked(); the exception is logged and ignored, the rest of the chain still receives the close, and the connection is returned to the pool as it would have been. abstractCreateStatementInterceptor.constructorCacheIndexPopulated=Constructor cache index {0} is already populated for {1}, but {2} was requested. Every use of an index must pass an interface the cached proxy implements. abstractCreateStatementInterceptor.unableToCreateConstructorCacheEntry=Unable to create the constructor cache entry for index {0} ({1}). 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 d3bf9c2f88..522f9ed830 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 @@ -19,17 +19,22 @@ package org.apache.tomcat.jdbc.pool.interceptor; 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.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.apache.tomcat.jdbc.pool.JdbcInterceptor; + /** * The contract of the constructor cache that every subclass of * {@code AbstractCreateStatementInterceptor} shares in the JVM: an index @@ -42,9 +47,10 @@ import org.junit.jupiter.api.Test; * or as a silently degraded, unproxied statement. * * <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> + * the tests below use only those pairings, and the one test that simulates a + * slot the initializer failed to populate restores the published entry in a + * finally block, so running them in the same JVM as the rest of the suite - + * and in any method order - is safe.</p> */ class TestAbstractCreateStatementInterceptor { @@ -108,7 +114,7 @@ class TestAbstractCreateStatementInterceptor { // The rejection must leave the cached entry untouched, so a caller // with the correct pairing is unaffected. - Assertions.assertSame(cached, AbstractCreateStatementInterceptor.constructors[idx]); + Assertions.assertSame(cached, AbstractCreateStatementInterceptor.cachedConstructor(idx)); Assertions.assertSame(cached, interceptor.getConstructor(idx, Statement.class)); } @@ -210,7 +216,7 @@ class TestAbstractCreateStatementInterceptor { InvocationHandler handler = (proxy, method, args) -> null; for (int idx = 0; idx < canonical.length; idx++) { - Constructor<?> published = AbstractCreateStatementInterceptor.constructors[idx]; + Constructor<?> published = AbstractCreateStatementInterceptor.cachedConstructor(idx); Assertions.assertNotNull(published, "index " + idx + " must hold an entry once the class is initialized"); Assertions.assertSame(published, new CacheUser().getConstructor(idx, canonical[idx]), @@ -219,4 +225,321 @@ class TestAbstractCreateStatementInterceptor { "the published constructor must produce a proxy implementing " + canonical[idx].getName()); } } + + /* + * A slot the class initializer failed to populate must still yield a + * working constructor, and the method must not write the shared slot from + * the calling thread: the class initializer is the only writer of the + * cache, which is what makes every read safely published without a lock. + * Before the fix, a degraded slot was lazily written without any + * happens-before edge - a data race against the concurrent readers of the + * other entries. + * + * The test replaces a canonical slot, so it restores the exact published + * reference in a finally block; the suite runs methods sequentially in a + * single thread (no Jupiter parallel execution is configured), which is + * what makes the temporary nulling safe. + */ + @Test + void anEmptySlotIsServedWithoutWritingTheCache() throws Exception { + CacheUser interceptor = new CacheUser(); + int idx = AbstractCreateStatementInterceptor.CREATE_STATEMENT_IDX; + Constructor<?> published = AbstractCreateStatementInterceptor.cachedConstructor(idx); + + AbstractCreateStatementInterceptor.replaceCachedConstructor(idx, null); + try { + Constructor<?> first = interceptor.getConstructor(idx, Statement.class); + Constructor<?> second = interceptor.getConstructor(idx, Statement.class); + + Assertions.assertNull(AbstractCreateStatementInterceptor.cachedConstructor(idx), + "a degraded slot must not be written by the thread that served it"); + Assertions.assertTrue(first.getDeclaringClass().isInstance( + first.newInstance(new Object[] { (InvocationHandler) (p, m, a) -> null })), + "a constructor computed on demand must still produce the proxy"); + Assertions.assertEquals(first, second, + "the constructor computed on demand must be equivalent for the same request"); + } finally { + AbstractCreateStatementInterceptor.replaceCachedConstructor(idx, published); + } + + // The published entry is untouched by the degraded calls. + Assertions.assertSame(published, interceptor.getConstructor(idx, Statement.class)); + } + + /* A hook that fails, as a third-party subclass or an Error can. */ + private static class ThrowingHook extends AbstractCreateStatementInterceptor { + + private final RuntimeException failure; + + ThrowingHook(RuntimeException failure) { + this.failure = failure; + } + + @Override + public Object createStatement(Object proxy, Method method, Object[] args, Object statement, long time) { + throw failure; + } + + @Override + public void closeInvoked() { + // NOOP + } + } + + /* A hook whose closeInvoked() does whatever the test handed it. */ + private static class CloseHook extends AbstractCreateStatementInterceptor { + + private final Runnable behaviour; + final AtomicBoolean ran = new AtomicBoolean(); + + CloseHook(Runnable behaviour) { + this.behaviour = behaviour; + } + + @Override + public Object createStatement(Object proxy, Method method, Object[] args, Object statement, long time) { + return statement; + } + + @Override + public void closeInvoked() { + ran.set(true); + behaviour.run(); + } + } + + /* The link a hand-built chain ends in, recording whether the call arrived. */ + private static JdbcInterceptor terminal(AtomicBoolean reached) { + return new JdbcInterceptor() { + @Override + public Object invoke(Object proxy, Method method, Object[] args) { + reached.set(true); + return null; + } + + @Override + public void reset(org.apache.tomcat.jdbc.pool.ConnectionPool parent, + org.apache.tomcat.jdbc.pool.PooledConnection con) { + // NOOP + } + }; + } + + /* A hook that records the elapsed time handed to it. */ + private static class TimingHook extends AbstractCreateStatementInterceptor { + + final AtomicLong timeSeen = new AtomicLong(Long.MIN_VALUE); + + @Override + public Object createStatement(Object proxy, Method method, Object[] args, Object statement, long time) { + timeSeen.set(time); + return statement; + } + + @Override + public void closeInvoked() { + // NOOP + } + } + + /* A statement stand-in that records whether it was closed. */ + private static Statement recordingStatement(AtomicBoolean closed, RuntimeException closeFailure) { + return (Statement) Proxy.newProxyInstance( + TestAbstractCreateStatementInterceptor.class.getClassLoader(), + new Class<?>[] { Statement.class }, + (proxy, method, args) -> { + if ("close".equals(method.getName())) { + closed.set(true); + if (closeFailure != null) { + throw closeFailure; + } + } + return null; + }); + } + + /* A chain of one interceptor over a terminal that hands out the statement. */ + private static void chainOver(AbstractCreateStatementInterceptor interceptor, Statement real) { + interceptor.setNext(new JdbcInterceptor() { + @Override + public Object invoke(Object proxy, Method method, Object[] args) { + return real; + } + + @Override + public void reset(org.apache.tomcat.jdbc.pool.ConnectionPool parent, + org.apache.tomcat.jdbc.pool.PooledConnection con) { + // NOOP + } + }); + } + + /* + * Regression for the orphaned statement: when the createStatement hook + * throws, the application never receives the statement, so the base must + * not leave it open and unreachable on the pooled connection. Before the + * fix the throwable propagated and the just-created statement stayed open + * for the life of the physical connection. + */ + @Test + void aFailingCreateHookClosesTheStatementTheApplicationNeverReceives() throws Throwable { + AtomicBoolean closed = new AtomicBoolean(); + Statement real = recordingStatement(closed, null); + RuntimeException hookFailure = new RuntimeException("hook-failed"); + + ThrowingHook interceptor = new ThrowingHook(hookFailure); + chainOver(interceptor, real); + + Method create = java.sql.Connection.class.getMethod("createStatement"); + RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, + () -> interceptor.invoke(null, create, null), + "the hook failure must still reach the application"); + + Assertions.assertSame(hookFailure, thrown, "the original throwable must propagate unchanged"); + Assertions.assertTrue(closed.get(), + "the statement the application never received must not be orphaned"); + } + + /* + * A failing close of the orphaned statement must not mask the hook's + * failure: the original throwable is still the one the application sees, + * with the close failure suppressed. + */ + @Test + void aCloseFailureOfTheOrphanedStatementIsSuppressedNotThrown() throws Throwable { + AtomicBoolean closed = new AtomicBoolean(); + RuntimeException closeFailure = new RuntimeException("close-failed"); + Statement real = recordingStatement(closed, closeFailure); + RuntimeException hookFailure = new RuntimeException("hook-failed"); + + ThrowingHook interceptor = new ThrowingHook(hookFailure); + chainOver(interceptor, real); + + Method create = java.sql.Connection.class.getMethod("createStatement"); + RuntimeException thrown = Assertions.assertThrows(RuntimeException.class, + () -> interceptor.invoke(null, create, null)); + + Assertions.assertSame(hookFailure, thrown); + Assertions.assertTrue(closed.get()); + Assertions.assertArrayEquals(new Throwable[] { closeFailure }, thrown.getSuppressed(), + "the close failure must be attached to the exception the application receives"); + } + + /* + * Regression for the unguarded close fan-out: a throwing closeInvoked() + * used to short-circuit the dispatch before the close reached the chain + * terminal, so the session was never returned to the pool - and, with the + * handle already disposed, the application could not even retry the + * close. A hook failure must instead be isolated: logged, the close still + * travels the rest of the chain, and the borrow ends. + */ + @Test + void aFailingCloseHookStillEndsTheBorrow() throws Throwable { + AtomicBoolean terminalReached = new AtomicBoolean(); + CloseHook throwing = new CloseHook(() -> { + throw new IllegalStateException("close-hook-failed"); + }); + throwing.setNext(terminal(terminalReached)); + + Method close = java.sql.Connection.class.getMethod("close"); + throwing.invoke(null, close, null); + + Assertions.assertTrue(throwing.ran.get(), "the hook must have been attempted"); + Assertions.assertTrue(terminalReached.get(), + "the close must still reach the terminal so the borrow ends"); + } + + /* + * Isolation is per link: a throwing hook may not stop the hooks below it + * from receiving the close, just as the reset and disconnected fan-outs + * isolate each interceptor individually. + */ + @Test + void aFailingCloseHookDoesNotStopTheHooksBelowIt() throws Throwable { + AtomicBoolean terminalReached = new AtomicBoolean(); + CloseHook throwing = new CloseHook(() -> { + throw new IllegalStateException("close-hook-failed"); + }); + CloseHook inner = new CloseHook(() -> { + }); + throwing.setNext(inner); + inner.setNext(terminal(terminalReached)); + + Method close = java.sql.Connection.class.getMethod("close"); + throwing.invoke(null, close, null); + + Assertions.assertTrue(throwing.ran.get(), "the throwing hook ran and failed"); + Assertions.assertTrue(inner.ran.get(), "the hook beneath the thrower must still receive the close"); + Assertions.assertTrue(terminalReached.get(), "the close must still reach the terminal"); + } + + /* + * A StackOverflowError is the recoverable Error, and TrapException does + * not convert Errors, so the isolation must catch it; a + * VirtualMachineError must instead propagate, per the policy + * ExceptionUtils.handleThrowable enforces across the module. + */ + @Test + void anErrorThrownByTheCloseHookIsIsolatedToo() throws Throwable { + AtomicBoolean terminalReached = new AtomicBoolean(); + CloseHook throwing = new CloseHook(() -> { + throw new StackOverflowError("close-hook-failed"); + }); + throwing.setNext(terminal(terminalReached)); + + Method close = java.sql.Connection.class.getMethod("close"); + throwing.invoke(null, close, null); + + Assertions.assertTrue(terminalReached.get(), + "even a recoverable Error from the hook must not strand the session as busy"); + } + + @Test + void aFatalErrorThrownByTheCloseHookPropagates() throws Exception { + AtomicBoolean terminalReached = new AtomicBoolean(); + InternalError fatal = new InternalError("close-hook-fatal"); + CloseHook throwing = new CloseHook(() -> { + throw fatal; + }); + throwing.setNext(terminal(terminalReached)); + + Method close = java.sql.Connection.class.getMethod("close"); + InternalError thrown = Assertions.assertThrows(InternalError.class, + () -> throwing.invoke(null, close, null), + "the fatal errors the pool never swallows must propagate from the close dispatch"); + Assertions.assertSame(fatal, thrown); + } + + /* + * The time handed to the hook is booked as a duration (prepare totals and + * the averages derived from them), and measuring it on the wall clock let + * a clock step between the two readings book a negative or wildly large + * duration. Pin the contract: milliseconds, and from the monotonic clock + * the never-negative guarantee comes from. + */ + @Test + void theElapsedTimeBookedByTheHookIsAMonotonicMillisecondDuration() throws Throwable { + TimingHook hook = new TimingHook(); + hook.setNext(new JdbcInterceptor() { + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws InterruptedException { + Thread.sleep(25); + return null; + } + + @Override + public void reset(org.apache.tomcat.jdbc.pool.ConnectionPool parent, + org.apache.tomcat.jdbc.pool.PooledConnection con) { + // NOOP + } + }); + + Method prepare = java.sql.Connection.class.getMethod("prepareStatement", String.class); + hook.invoke(null, prepare, new Object[] { "SELECT 1" }); + + Assertions.assertTrue(hook.timeSeen.get() >= 20, + "the hook must observe the creation duration in milliseconds, saw " + hook.timeSeen.get()); + Assertions.assertTrue(AbstractCreateStatementInterceptor.elapsedMillis(System.nanoTime()) >= 0, + "a duration measured with the monotonic clock cannot come out negative"); + } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestCloseInvokedFailureIsolated.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestCloseInvokedFailureIsolated.java new file mode 100644 index 0000000000..e8955d6aa9 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestCloseInvokedFailureIsolated.java @@ -0,0 +1,110 @@ +/* + * 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.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.ConnectionPool; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * A throwing {@code closeInvoked()} hook must not strand the borrow: the + * close dispatch runs the hook before handing the call down the chain, so an + * unguarded throw used to short-circuit the chain terminal, the close threw + * at the application over an already-disposed handle and the session stayed + * busy - permanently so without the abandoned-connection reaper, which is not + * on by default. The base class now isolates the hook the way it isolates the + * reset and disconnected fan-outs, and the close travels on. + */ +class TestCloseInvokedFailureIsolated extends JdbcPoolBaseTest { + + /** + * Interceptor that always throws from its borrow-end hook, simulating a + * third-party subclass that fails unexpectedly. Configured through the + * jdbcInterceptors class-name list, so it must stay a public static class + * with a no-arg constructor. + */ + public static class ThrowingCloseHook extends SlowQueryReport { + @Override + public void closeInvoked() { + throw new IllegalStateException("close hook intentionally fails"); + } + } + + /* maxWait stays short so a stranded borrow fails the test in seconds. */ + private DataSource throwingHookPool(String interceptors) throws SQLException { + DataSource ds = newDataSource(); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxWait(2000); + ds.getPoolProperties().setJdbcInterceptors(interceptors); + ds.createPool(); + return ds; + } + + @Test + void throwingCloseHookStillReturnsTheSessionToThePool() throws Exception { + DataSource ds = throwingHookPool(ThrowingCloseHook.class.getName()); + ConnectionPool pool = ds.getPool(); + + Connection c = ds.getConnection(); + try (Statement st = c.createStatement()) { + st.execute("SELECT 1"); + } + Assertions.assertDoesNotThrow(c::close, + "a failing close hook must not fail the application's close()"); + + Assertions.assertEquals(0, pool.getActive(), + "the borrow must end even though the hook threw"); + Assertions.assertEquals(1, pool.getIdle(), + "the session must be back in the idle queue"); + + // And the slot is genuinely free: the next borrow is served. + try (Connection next = ds.getConnection(); Statement st = next.createStatement()) { + st.execute("SELECT 1"); + } + } + + /* + * The isolation must not reorder the fan-out or spare the pool: a hook + * failing below a StatementFinalizer still leaves the borrow ended, the + * same property the always-configured StatementFacade and the other + * in-tree hooks rely on. + */ + @Test + void closeHookFailingBeneathTheStatementFinalizerStillEndsTheBorrow() throws Exception { + DataSource ds = throwingHookPool("org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer;" + + ThrowingCloseHook.class.getName()); + ConnectionPool pool = ds.getPool(); + + Connection c = ds.getConnection(); + try (Statement st = c.createStatement()) { + st.execute("SELECT 1"); + } + Assertions.assertDoesNotThrow(c::close, + "a failing hook below the finalizer must not fail the close either"); + + Assertions.assertEquals(0, pool.getActive(), + "the borrow must end whatever the lower hook made of it"); + Assertions.assertEquals(1, pool.getIdle()); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
