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 62d4d7bb4f26848aac349686d58988d88932c91f Author: opencode <[email protected]> AuthorDate: Mon Nov 2 19:56:31 2026 +0100 Harden and document the disposable connection facade The disposable facade is the topmost interceptor of the connection handle the pool hands out, the layer whose dispose makes a closed handle inert. A full review of the class (defect report plus a clean-room review round) found one functional defect, since fixed, and a class javadoc that was an incomplete and partly incorrect specification of the contract, now rewritten to match the code. The closed-handle answer used to ride on the NullPointerException the dead chain throws: invoke() delegated through the base class and caught any NPE that surfaced, turning it into the "PooledConnection has already been closed." SQLException whenever the chain link read null at catch time. The catch could not tell the sentinel from a genuine failure from below: a call the facade had already dispatched into the live chain, whose driver or interceptor then threw an NPE, was rewritten into the misleading closed answer - its trace discarded entirely, no cause, no suppression, no log - whenever a concurrent dispose (an abandoned-connection reap, a close on another thread, a pool shutdown) sealed the facade before the catch rechecked the link. invoke() now reads the chain link once, into a snapshot, and decides every disposed answer from that snapshot before dispatching, directly rather than through the base class's exception; a failure that surfaces from the dispatch is therefore by construction a real failure from below and propagates untouched, and a call that entered the chain before a concurrent dispose sealed it is refused by the borrow fences the layers below already run. The dispose's capture-and-seal under the facade monitor is unchanged. A regression test reproduces the misattribution window single-threaded (the chain seals the facade, then fails with an NPE of its own): with the old catch it surfaced the misleading SQLException; now the real failure reaches the application with its identity intact. The documentation was made true. The javadoc claimed the facade "wraps an object of type PooledConnection" - it wraps the per-borrow statement facade above the cached chain whose tail is the ProxyConnection over the session. It listed three intercepted methods where the class answers seven - the equals and hashCode identity guarantees, the isClosed and isValid liveness answers and the close and abort no-ops a released handle owes the specification were undocumented, and the load-bearing dispose contract, that every other method on a disposed handle fails with the closed-connection SQLException, was stated nowhere. The useEquals paragraph stated the polarity backwards and named the wrong configuration source: comparison defaults to String.equals, the value this facade carries is copied from the interceptor it wraps, and the pool-level property reaches only the ProxyConnection at the chain's bottom - verified against the constructors in ConnectionPool.setupConnection and StatementFacade. The dispose bullet's "link between cutoff and the next interceptor", a term that exists nowhere in the codebase, names this facade now. With the contract in the javadoc, the two comments that restated it were trimmed; the rationale comments, which the clean-room review confirmed hold against every call site and test, stay. jdbc-pool suite: 735 green; javadoc builds warning-free. --- .../jdbc/pool/DisposableConnectionFacade.java | 85 +++++++++++++++------- .../jdbc/pool/TestDisposableConnectionFacade.java | 53 +++++++++++++- 2 files changed, 112 insertions(+), 26 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DisposableConnectionFacade.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DisposableConnectionFacade.java index 65f2220ba7..b4b54c7e8b 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DisposableConnectionFacade.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DisposableConnectionFacade.java @@ -23,18 +23,41 @@ import java.sql.SQLException; import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** - * A DisposableConnectionFacade object is the top most interceptor that wraps an object of type - * {@link PooledConnection}. The DisposableConnectionFacade intercepts three methods: + * A DisposableConnectionFacade object is the top most interceptor of the connection handle the pool hands out: + * the pool creates one per borrow, wrapping the per-borrow {@link StatementFacade} above the interceptor chain + * cached on the {@link PooledConnection}, which ends in the {@link ProxyConnection} over the physical session. + * Disposing the facade - breaking the link to that chain - is what makes a handle inert once its borrow has + * ended, so a handle kept past its borrow can never drive the session the pool has re-served to the next + * borrower. + * <p> + * The DisposableConnectionFacade intercepts seven method names. While the facade is live, the answers marked + * live are given; once it is disposed, the names marked disposed are answered by the facade itself and the call + * never reaches the chain: * <ul> - * <li>{@link java.sql.Connection#close()} - breaks the link between cutoff and the next interceptor, then returns the - * connection to the pool. May be called multiple times.</li> - * <li>{@link java.sql.Connection#abort(java.util.concurrent.Executor)} - disposes of the facade in the same way, then - * delegates the abort to the chain, which ends the borrow. A facade already disposed answers the call with the - * no-op the JDBC specification prescribes for a closed connection.</li> - * <li>{@link java.lang.Object#toString()} - returns a custom string for this object</li> + * <li>{@link java.sql.Connection#close()} - live: breaks the link between this facade and the next interceptor, + * then delegates the close to the chain, which returns the connection to the pool; may be called multiple + * times. Disposed: the no-op the JDBC specification prescribes for a closed connection.</li> + * <li>{@link java.sql.Connection#abort(java.util.concurrent.Executor)} - live: disposes of the facade in the + * same way, then delegates the abort to the chain, which ends the borrow. Disposed: the no-op the + * specification prescribes for a closed connection.</li> + * <li>{@link java.lang.Object#equals(Object)} and {@link java.lang.Object#hashCode()} - answered in both + * states, with the identity of the handle: each borrow is one handle even over a physical session the pool has + * re-served.</li> + * <li>{@link java.sql.Connection#isClosed()} - live: delegated. Disposed: <code>true</code>.</li> + * <li>{@link java.sql.Connection#isValid(int)} - live: delegated. Disposed: <code>false</code>, the answer the + * JDBC contract gives a closed connection.</li> + * <li>{@link java.lang.Object#toString()} - live: delegated. Disposed: a custom string, so a released handle + * stays printable for logging.</li> * </ul> - * By default method comparisons is done on a String reference level, unless the - * {@link PoolConfiguration#setUseEquals(boolean)} has been called with a <code>true</code> argument. + * Every other method on a disposed handle fails with an {@link SQLException} reporting the closed connection, + * the way a use of a closed connection fails; a live facade delegates it to the chain. + * <p> + * By default method names are compared with {@link String#equals(Object)}; {@link #setUseEquals(boolean)} + * called with a <code>false</code> argument switches an instance to reference comparison. This facade takes its + * value from the interceptor it wraps - in a configured pool, the top configured interceptor's own value, + * decided by that interceptor's <code>useEquals</code> property; the pool-level + * {@link PoolConfiguration#setUseEquals(boolean)} reaches the {@link ProxyConnection} at the bottom of the + * chain, not this class. */ public class DisposableConnectionFacade extends JdbcInterceptor { /** @@ -64,6 +87,12 @@ public class DisposableConnectionFacade extends JdbcInterceptor { @Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + // Read the link once, into a snapshot: the disposed answers, the + // dispose and the dispatch below are all decided about one state of + // the facade, and a call can only ever enter the chain this guard + // looked at - so a failure surfacing from the dispatch can only have + // come from below it. + JdbcInterceptor next = getNext(); if (compare(EQUALS_VAL, method)) { // Object.equals must answer false - never throw - for a null // or foreign argument, so the handler extraction has to be @@ -74,7 +103,7 @@ public class DisposableConnectionFacade extends JdbcInterceptor { this.equals(Proxy.getInvocationHandler(other))); } else if (compare(HASHCODE_VAL, method)) { return Integer.valueOf(this.hashCode()); - } else if (getNext() == null) { + } else if (next == null) { if (compare(ISCLOSED_VAL, method)) { return Boolean.TRUE; } else if (compare(CLOSE_VAL, method)) { @@ -106,29 +135,35 @@ public class DisposableConnectionFacade extends JdbcInterceptor { // itself runs outside the lock, which matters most for abort: the // specification allows the driver to take an extended period in // it, and a concurrent close must not wait on that. - JdbcInterceptor next; + JdbcInterceptor captured; synchronized (this) { - next = getNext(); + captured = getNext(); setNext(null); } - if (next == null) { + if (captured == null) { // Already disposed by a concurrent close. return null; } - return next.invoke(proxy, method, args); + return captured.invoke(proxy, method, args); } - try { - return super.invoke(proxy, method, args); - } catch (NullPointerException e) { - if (getNext() == null) { - if (compare(TOSTRING_VAL, method)) { - return "DisposableConnectionFacade[null]"; - } - throw new SQLException(MessageFactory.get("disposableConnectionFacade.pooledConnectionClosed")); + if (next == null) { + // The disposed answer is decided on this call's own chain read, + // not by catching the NullPointerException the dead chain throws + // around the delegation: that catch could not tell the sentinel + // from a genuine NullPointerException raised below the facade by + // a call it had already dispatched, so a driver or interceptor + // failure that landed while a concurrent dispose was sealing the + // facade was rewritten into this misleading closed failure with + // its trace discarded. A failure surfacing from the dispatch + // below is such a failure and stays what it is; a call that + // entered the chain before a concurrent dispose sealed it is + // refused by the borrow fence the layers below the facade run. + if (compare(TOSTRING_VAL, method)) { + return "DisposableConnectionFacade[null]"; } - - throw e; + throw new SQLException(MessageFactory.get("disposableConnectionFacade.pooledConnectionClosed")); } + return next.invoke(proxy, method, args); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDisposableConnectionFacade.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDisposableConnectionFacade.java index ab47200249..4fae405a3d 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDisposableConnectionFacade.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDisposableConnectionFacade.java @@ -92,7 +92,33 @@ class TestDisposableConnectionFacade { } } - private static Connection proxyFor(StubChain chain) { + /* Stands in for a chain that both seals the facade and then fails with + an NPE of its own - the shape of a driver or interceptor failure + landing while a concurrent abandon-close seals the facade, run + single-threaded so the interleaving is guaranteed. */ + private static final class SealThenFailChain extends JdbcInterceptor { + + final AtomicInteger closeCount = new AtomicInteger(); + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // NOOP + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if ("close".equals(method.getName())) { + closeCount.incrementAndGet(); + return null; + } + // The dispose of the close below reaches the facade while this + // call is in flight, exactly as a reap on another thread would. + ((Connection) proxy).close(); + throw new NullPointerException("failure from below the facade"); + } + } + + private static Connection proxyFor(JdbcInterceptor chain) { DisposableConnectionFacade facade = new DisposableConnectionFacade(chain); return (Connection) Proxy.newProxyInstance( TestDisposableConnectionFacade.class.getClassLoader(), @@ -146,6 +172,31 @@ class TestDisposableConnectionFacade { Assertions.assertEquals(1, chain.closeCount.get()); } + /* The disposed answer used to ride on catching the NullPointerException + the dead chain throws. That catch also caught genuine failures from + below: a call the facade had already dispatched into the live chain, + whose driver or interceptor then threw an NPE, was rewritten into the + misleading "PooledConnection has already been closed." SQLException - + with its trace discarded - whenever a concurrent dispose sealed the + facade before the catch rechecked the link. A call that entered the + chain must see its own failure. */ + @Test + void aFailureFromBelowSurvivesASealThatLandsWhileTheCallIsInFlight() throws Exception { + SealThenFailChain chain = new SealThenFailChain(); + Connection proxy = proxyFor(chain); + + NullPointerException failure = Assertions.assertThrows(NullPointerException.class, + proxy::getAutoCommit, + "the real failure from below the facade must reach the application, " + + "not the disposed facade's already-closed answer"); + Assertions.assertEquals("failure from below the facade", failure.getMessage(), + "the application must be told what actually failed"); + Assertions.assertEquals(1, chain.closeCount.get(), + "precondition: the call sealed the facade before it failed"); + Assertions.assertTrue(proxy.isClosed(), + "precondition: the facade was sealed before the failure surfaced"); + } + @Test void concurrentCloseIsSealedOnceIntoTheChain() throws Exception { StubChain chain = new StubChain(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
