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 cba5264f9f74adabfee2fa15d34f445ebf53c57a Author: opencode <[email protected]> AuthorDate: Mon Oct 26 09:14:00 2026 +0100 Dispose the connection facade before returning the connection to the pool DisposableConnectionFacade sealed itself in a finally only after delegating close() down the interceptor chain, but the chain hands the PooledConnection back to the pool during that delegation, where it is immediately re-publishable and the shared per-connection chain can be re-armed by a new borrower. Inside the window before the seal, new calls through a stale reference passed the disposed guard and executed against the physical connection now owned by the next borrower, and a concurrent second close could drive the return a second time. For close, the capture of the next interceptor and the disposal are now a single atomic step performed before the delegation runs, and a concurrent second close observes the disposed facade and returns as a no-op. --- .../jdbc/pool/DisposableConnectionFacade.java | 32 ++- .../jdbc/pool/TestDisposableConnectionFacade.java | 265 +++++++++++++++++++++ 2 files changed, 291 insertions(+), 6 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 368e3150e2..a83ef4930a 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 @@ -25,8 +25,8 @@ import java.sql.SQLException; * object of type {@link PooledConnection}. The DisposableConnectionFacade intercepts * two methods: * <ul> - * <li>{@link java.sql.Connection#close()} - returns the connection to the - * pool then breaks the link between cutoff and the next interceptor. + * <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.lang.Object#toString()} - returns a custom string for this * object</li> @@ -82,6 +82,30 @@ public class DisposableConnectionFacade extends JdbcInterceptor { } } + if (compare(CLOSE_VAL, method)) { + // Dispose the facade before delegating close: the chain hands + // the physical connection back to the pool, where it can be + // re-borrowed and the shared interceptor chain re-armed before + // this call returns. Sealing only afterwards would leave a + // window in which new calls pass the disposed guard above and + // execute against the next borrower's connection, and a + // concurrent second close could drive the return a second time. + // Capture and seal are one atomic step under this facade's + // monitor so that of two concurrent closes exactly one observes + // a live link and delegates; the delegation itself runs outside + // the lock. + JdbcInterceptor next; + synchronized (this) { + next = getNext(); + setNext(null); + } + if (next == null) { + // Already disposed by a concurrent close. + return null; + } + return next.invoke(proxy, method, args); + } + try { return super.invoke(proxy, method, args); } catch (NullPointerException e) { @@ -94,10 +118,6 @@ public class DisposableConnectionFacade extends JdbcInterceptor { } throw e; - } finally { - if (compare(CLOSE_VAL, method)) { - setNext(null); - } } } } 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 new file mode 100644 index 0000000000..de244655c7 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestDisposableConnectionFacade.java @@ -0,0 +1,265 @@ +/* + * 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; + +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.SQLException; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.Test; + +/** + * The disposable facade must sever its link to the interceptor chain + * <em>before</em> the delegated close hands the physical connection back to + * the pool. Sealing in a finally after the delegation returns leaves the + * window between {@code pool.returnConnection()} and {@code setNext(null)} + * in which new calls still pass the disposed guard and reach the shared + * chain - which a new borrower may already have re-armed through + * {@code reset()} - so the call executes against the next borrower's + * connection, and a concurrent second close can drive the return a second + * time. + */ +class TestDisposableConnectionFacade { + + private static final long AWAIT_MS = 15000; + + /* Stands in for the cached per-connection chain below the facade. Its + close() marks the connection as returned to the pool and then parks, + reproducing the window in which, in the real chain, the connection + already sits in the idle queue while the facade is not yet sealed. */ + private static final class StubChain extends JdbcInterceptor { + + final CountDownLatch closeEntered = new CountDownLatch(1); + final CountDownLatch closeRelease = new CountDownLatch(1); + final AtomicInteger closeCount = new AtomicInteger(); + final CopyOnWriteArrayList<String> callsAfterReturn = new CopyOnWriteArrayList<>(); + volatile boolean returnedToPool; + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // NOOP + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + String name = method.getName(); + if ("close".equals(name)) { + closeCount.incrementAndGet(); + // ProxyConnection performs pool.returnConnection() at this + // exact point in the delegation + returnedToPool = true; + closeEntered.countDown(); + closeRelease.await(); + return null; + } + if (returnedToPool) { + callsAfterReturn.add(name); + } + Class<?> returnType = method.getReturnType(); + if (returnType == boolean.class) { + return Boolean.FALSE; + } else if (returnType == int.class) { + return Integer.valueOf(0); + } else if (returnType == long.class) { + return Long.valueOf(0L); + } + return null; + } + } + + private static Connection proxyFor(StubChain chain) { + DisposableConnectionFacade facade = new DisposableConnectionFacade(chain); + return (Connection) Proxy.newProxyInstance( + TestDisposableConnectionFacade.class.getClassLoader(), + new Class<?>[] { Connection.class }, + facade); + } + + @Test + void noCallPassesTheFacadeOnceTheConnectionIsReturned() throws Exception { + StubChain chain = new StubChain(); + Connection proxy = proxyFor(chain); + + AtomicReference<Throwable> closeFailure = new AtomicReference<>(); + Thread closer = new Thread(() -> { + try { + proxy.close(); + } catch (Throwable t) { + closeFailure.set(t); + } + }, "close-in-progress"); + closer.setDaemon(true); + closer.start(); + try { + assertTrue(chain.closeEntered.await(AWAIT_MS, TimeUnit.MILLISECONDS), + "the chain close must be entered"); + assertTrue(chain.returnedToPool, + "precondition: the connection has already been handed back"); + + // From this point on nothing may pass through the facade: the + // physical connection is the pool's to hand out again. + assertEquals(Boolean.TRUE, proxy.isClosed(), + "the facade must report closed while the close is still in flight"); + assertEquals(Boolean.FALSE, proxy.isValid(0), + "the facade must report invalid while the close is still in flight"); + assertThrows(SQLException.class, proxy::getAutoCommit, + "post-return calls must be rejected instead of reaching the chain"); + assertEquals(0, chain.callsAfterReturn.size(), + "no call may reach the chain after the connection was returned, saw " + + chain.callsAfterReturn); + } finally { + chain.closeRelease.countDown(); + closer.join(AWAIT_MS); + assertFalse(closer.isAlive(), "the closing thread must have finished"); + } + + assertNull(closeFailure.get(), + () -> "close() must not fail: " + closeFailure.get()); + assertEquals(1, chain.closeCount.get(), "the chain must see exactly one close"); + // A later close stays a no-op and never re-enters the chain + proxy.close(); + assertEquals(1, chain.closeCount.get()); + } + + @Test + void concurrentCloseIsSealedOnceIntoTheChain() throws Exception { + StubChain chain = new StubChain(); + Connection proxy = proxyFor(chain); + + AtomicReference<Throwable> firstFailure = new AtomicReference<>(); + Thread first = new Thread(() -> { + try { + proxy.close(); + } catch (Throwable t) { + firstFailure.set(t); + } + }, "first close"); + first.setDaemon(true); + first.start(); + assertTrue(chain.closeEntered.await(AWAIT_MS, TimeUnit.MILLISECONDS), + "the first close must be inside the chain"); + + AtomicReference<Throwable> second = new AtomicReference<>(); + Thread secondClose = new Thread(() -> { + try { + proxy.close(); + } catch (Throwable t) { + second.set(t); + } + }, "second close"); + secondClose.setDaemon(true); + secondClose.start(); + try { + secondClose.join(AWAIT_MS); + assertFalse(secondClose.isAlive(), + "the second closing thread must not park in the chain"); + + assertNull(second.get(), + () -> "the concurrent close must not fail: " + second.get()); + assertEquals(1, chain.closeCount.get(), + "the concurrent close must observe the disposed facade instead of " + + "driving the return a second time"); + } finally { + chain.closeRelease.countDown(); + } + + first.join(AWAIT_MS); + assertFalse(first.isAlive(), "the first closing thread must have finished"); + assertNull(firstFailure.get(), + () -> "the first close must not fail: " + firstFailure.get()); + assertEquals(1, chain.closeCount.get()); + } + + /* The serialized second close above cannot cover the interleaving that + matters most: two closes released into proxy.close() at the same + instant. Capture-and-seal must be atomic so exactly one of them ever + reaches the chain - the other must observe the disposed facade. */ + @Test + void simultaneousDoubleCloseNeverDelegatesTwice() throws Exception { + // Detection of the non-atomic variant measures ~5*10^-5 per round, + // so the round count is chosen for ~200k to make the guard + // dependable; the whole loop costs roughly two seconds. + final int rounds = 200000; + // Both closers and the test thread meet at each barrier per round + final CyclicBarrier intoClose = new CyclicBarrier(3); + final CyclicBarrier roundDone = new CyclicBarrier(3); + final AtomicReference<Connection> proxyRef = new AtomicReference<>(); + final AtomicReference<Throwable> closerFailure = new AtomicReference<>(); + + Runnable closer = () -> { + try { + for (int i = 0; i < rounds; i++) { + intoClose.await(AWAIT_MS, TimeUnit.MILLISECONDS); + proxyRef.get().close(); + roundDone.await(AWAIT_MS, TimeUnit.MILLISECONDS); + } + } catch (Throwable t) { + closerFailure.compareAndSet(null, t); + } + }; + ExecutorService closers = Executors.newFixedThreadPool(2); + boolean completed = false; + try { + Future<?> first = closers.submit(closer); + Future<?> second = closers.submit(closer); + + for (int round = 0; round < rounds; round++) { + StubChain chain = new StubChain(); + chain.closeRelease.countDown(); // the chain close returns at once + proxyRef.set(proxyFor(chain)); + intoClose.await(AWAIT_MS, TimeUnit.MILLISECONDS); + roundDone.await(AWAIT_MS, TimeUnit.MILLISECONDS); + final int r = round; + assertNull(closerFailure.get(), + () -> "round " + r + ": a closer failed: " + closerFailure.get()); + assertEquals(1, chain.closeCount.get(), + () -> "round " + r + + ": both concurrent closes reached the chain; capture and seal" + + " must be atomic"); + } + first.get(AWAIT_MS, TimeUnit.MILLISECONDS); + second.get(AWAIT_MS, TimeUnit.MILLISECONDS); + completed = true; + } finally { + closers.shutdownNow(); + if (!completed) { + // A closer that died leaves its barriers untripartite, so the + // main thread times out rather than seeing its exception; + // surface the root failure instead. + Throwable t = closerFailure.get(); + if (t != null) { + throw new AssertionError("a closer thread failed: " + t, t); + } + } + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
