This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 717564a77fe32912caea83463e12659aabdee009 Author: opencode <[email protected]> AuthorDate: Fri Oct 30 23:57:49 2026 +0100 Give the statement decorator proxies a race-free call The decorator's statement proxies kept closed and delegate as plain fields and read them several times per call - the idempotence guard, the isClosed answer, the closed bail-out and then the forwarding call each read them again - while closing a statement from a foreign thread is supported pool behavior: StatementFinalizer closes the statements it tracks from whichever thread closes the connection, and the statement cache's purge closes from pool threads. A borrower past the guard could then read the just-nulled delegate and take Method.invoke's NullPointerException on a null receiver where the closed contract owes the "Statement closed." SQLException, and a thread that did not close might never observe closed at all. Follow the pattern AbstractQueryReport's proxies adopted in 495b82258a: make both fields volatile, take one snapshot of them per call before the guards, answer the closed contract - including a close met by a handle already gone - from the snapshot, and forward through the snapshot's target. closeInvoked() likewise reads the delegate once, which also shuts a double-read window of its own: asking the field twice could reach close() on a receiver a competing close had just nulled. The window this cannot close is the race itself - a call that snapshotted a live delegate and executes into a close landing underneath is the driver's to arbitrate, exactly as in the sibling proxies. --- .../interceptor/StatementDecoratorInterceptor.java | 58 ++++-- .../TestStatementDecoratorCrossThreadClose.java | 214 +++++++++++++++++++++ 2 files changed, 260 insertions(+), 12 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementDecoratorInterceptor.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementDecoratorInterceptor.java index 2c9f7be782..2e7e481d7f 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementDecoratorInterceptor.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/StatementDecoratorInterceptor.java @@ -204,13 +204,17 @@ public class StatementDecoratorInterceptor extends AbstractCreateStatementInterc protected class StatementProxy<T extends Statement> implements InvocationHandler { /** - * Indicates whether this statement has been closed. + * Indicates whether this statement has been closed. A statement can be closed from a thread other than the one + * executing on it - the statement finalizer closes tracked statements from whichever thread closes the + * connection, and the statement cache forces a close from its own purge - so the flag is written by one thread + * and read by others, and must be volatile to reach them at all. */ - protected boolean closed = false; + protected volatile boolean closed = false; /** - * The delegated statement. + * The delegated statement, nulled once a close has been forwarded. It is read by threads other than the one + * that nulls it, so, like {@link #closed}, volatile. */ - protected T delegate; + protected volatile T delegate; private Object actualProxy; /** * The connection handle of the borrow during which this proxy was created, bound through @@ -334,9 +338,17 @@ public class StatementDecoratorInterceptor extends AbstractCreateStatementInterc * @throws SQLException if the underlying statement could not be closed */ public void closeInvoked() throws SQLException { + // Read the delegate once: a second close running concurrently - + // the cache purge, or the application and the finalizer closing + // the same handle - can null the field between two reads, and a + // second read that finds it gone would invoke close() on a null + // receiver. A close that already completed is the one this + // snapshot answers for: the delegate is gone, nothing is + // re-closed, and the handle is retired quietly all the same. + final T target = delegate; try { - if (getDelegate() != null) { - getDelegate().close(); + if (target != null) { + target.close(); } } finally { closed = true; @@ -364,16 +376,33 @@ public class StatementDecoratorInterceptor extends AbstractCreateStatementInterc } // Was close invoked? boolean close = compare(CLOSE_VAL, method); - // Allow close to be called multiple times - if (close && closed) { + // Read the state once: another thread's close() - the pool's + // statement finalizer closes tracked statements from whichever + // thread closes the connection, and the statement cache forces a + // close from its purge - may complete while this call is being + // answered, and flag and delegate are two separate fields, so a + // call that read the flag as open can find the delegate already + // nulled. A close that finished in between is then answered as + // closed, on the missing delegate as much as on the flag, rather + // than by invoking on a null object: that is a + // NullPointerException, and the application is owed the + // SQLException the closed contract promises. The window this + // cannot close is the race itself: a call that reads the + // delegate while it is still live goes to it, an outcome + // indistinguishable from having simply outrun the close. + final boolean wasClosed = closed; + final T target = delegate; + final boolean gone = wasClosed || target == null; + // Allow close to be called multiple times, from either thread + if (close && gone) { return null; } // Are we calling isClosed? if (compare(ISCLOSED_VAL, method)) { - return Boolean.valueOf(closed); + return Boolean.valueOf(gone); } // If we are calling anything else, bail out - if (closed) { + if (gone) { throw new SQLException("Statement closed."); } if (compare(GETCONNECTION_VAL, method)) { @@ -393,8 +422,11 @@ public class StatementDecoratorInterceptor extends AbstractCreateStatementInterc if (close) { closeInvoked(); } else { - // Execute the query - result = method.invoke(delegate, args); + // Execute the query, on the delegate this call read as + // live: the snapshot keeps a concurrent close from + // replacing the receiver with null between the guard and + // the call + result = method.invoke(target, args); } } catch (Throwable t) { if (t instanceof InvocationTargetException && t.getCause() != null) { @@ -428,6 +460,8 @@ public class StatementDecoratorInterceptor extends AbstractCreateStatementInterc buf.append("; Sql="); buf.append(getSql()); buf.append("; Delegate="); + // Reads the live delegate rather than a snapshot: a description + // says what the proxy holds as it is asked. buf.append(getDelegate()); buf.append("; Connection="); buf.append(getConnection()); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementDecoratorCrossThreadClose.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementDecoratorCrossThreadClose.java new file mode 100644 index 0000000000..2e9a691827 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestStatementDecoratorCrossThreadClose.java @@ -0,0 +1,214 @@ +/* + * 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.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.concurrent.CountDownLatch; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * Closing a statement from another thread, the way the pool does it: the + * statement finalizer closes the tracked statements from whichever thread + * closes the connection, and the statement cache forces a close from its own + * purge, while a borrower may still be executing on the decorator proxy of + * the same statement. The close writes two fields - the flag and the delegate + * it nulls - and a call answered in the middle of that can find the flag open + * and the delegate gone, and a close completed on another thread may not be + * seen at all by a thread that never wrote. Answering such a call by invoking + * on the missing delegate escapes to the application as a NullPointerException + * on a null receiver; the closed contract owes it the SQLException instead. + * The racing test is the one that pins that, and fails while the two fields + * are read without a snapshot; the test before it guards the visibility and + * the contract a completed cross-thread close must leave behind. + */ +class TestStatementDecoratorCrossThreadClose { + + private static final long JOIN_TIMEOUT_MS = 30000; + + private static final Method EXECUTE; + private static final Method CLOSE; + private static final Method IS_CLOSED; + + static { + try { + EXECUTE = Statement.class.getMethod("execute", String.class); + CLOSE = Statement.class.getMethod("close"); + IS_CLOSED = Statement.class.getMethod("isClosed"); + } catch (NoSuchMethodException e) { + throw new ExceptionInInitializerError(e); + } + } + + /* Statement oracle: executing answers TRUE while it is open and fails + * with a SQLException once the closer thread has closed it, so every + * failure the borrower can see is one the proxy itself produced rather + * than one the oracle conjures. */ + private static final class FakeStatement implements InvocationHandler { + private volatile boolean closed; + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + String name = method.getName(); + if ("execute".equals(name)) { + if (closed) { + throw new SQLException("statement closed by the closer thread"); + } + return Boolean.TRUE; + } + if ("close".equals(name)) { + closed = true; + return null; + } + if ("isClosed".equals(name)) { + return Boolean.valueOf(closed); + } + if ("toString".equals(name)) { + return "FakeStatement"; + } + throw new UnsupportedOperationException(name); + } + } + + private static Statement openStatement(FakeStatement oracle) { + return (Statement) Proxy.newProxyInstance( + TestStatementDecoratorCrossThreadClose.class.getClassLoader(), + new Class<?>[] { Statement.class }, oracle); + } + + private static void awaitGo(CountDownLatch go) { + try { + go.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + + /* + * Once the other thread's close() has completed, every answer on this + * thread must report the closed statement: the flag says closed, the + * refusal carries the closed contract, a repeated close stays a no-op, + * and the delegate underneath really was closed. + */ + @Test + void aCloseCompletedByAnotherThreadIsAnsweredAsClosedAfterwards() throws Throwable { + StatementDecoratorInterceptor interceptor = new StatementDecoratorInterceptor(); + FakeStatement oracle = new FakeStatement(); + StatementDecoratorInterceptor.StatementProxy<Statement> proxy = + interceptor.new StatementProxy<>(openStatement(oracle), "SELECT 1"); + + Throwable[] closerFailure = new Throwable[1]; + Thread closer = new Thread(() -> { + try { + proxy.invoke(null, CLOSE, null); + } catch (Throwable t) { + closerFailure[0] = t; + } + }); + closer.start(); + closer.join(JOIN_TIMEOUT_MS); + Assertions.assertFalse(closer.isAlive(), "the closing thread must finish"); + Assertions.assertNull(closerFailure[0], "closing the proxy must not fail: " + closerFailure[0]); + + Assertions.assertEquals(Boolean.TRUE, proxy.invoke(null, IS_CLOSED, null), + "isClosed() must report the close that completed on the other thread"); + SQLException refused = Assertions.assertThrows(SQLException.class, + () -> proxy.invoke(null, EXECUTE, new Object[] { "SELECT 1" }), + "a call after the close must be refused, not served and not a NullPointerException"); + Assertions.assertTrue(refused.getMessage().contains("closed"), + "the refusal must carry the closed contract, was: " + refused.getMessage()); + Assertions.assertNull(proxy.invoke(null, CLOSE, null), + "a second close remains a no-op"); + Assertions.assertTrue(oracle.closed, "the close must still have reached the statement"); + } + + /* + * The race itself: a borrower executing in a loop while another thread + * closes the proxy, in the statement finalizer's own sequence - ask + * whether it is closed, then close it. A call may outrun the close and + * be served, or lose to it and be refused with a SQLException, from the + * proxy's guard or from the statement underneath; what it must never + * see is an invoke on a null delegate. Bounded rounds, no sleeps: the + * assertions are checked outcomes, so a failure names the round that + * produced it. + */ + @Test + void aBorrowerRacingACloseFromAnotherThreadNeverFindsANullDelegate() throws Throwable { + StatementDecoratorInterceptor interceptor = new StatementDecoratorInterceptor(); + final int rounds = 150; + final int executions = 250; + for (int round = 0; round < rounds; round++) { + FakeStatement oracle = new FakeStatement(); + StatementDecoratorInterceptor.StatementProxy<Statement> proxy = + interceptor.new StatementProxy<>(openStatement(oracle), "SELECT 1"); + final CountDownLatch ready = new CountDownLatch(2); + final CountDownLatch go = new CountDownLatch(1); + final Throwable[] borrower = new Throwable[1]; + final Throwable[] closer = new Throwable[1]; + + Thread executing = new Thread(() -> { + ready.countDown(); + awaitGo(go); + try { + for (int i = 0; i < executions; i++) { + Object served = proxy.invoke(null, EXECUTE, new Object[] { "SELECT 1" }); + if (!Boolean.TRUE.equals(served)) { + borrower[0] = new IllegalStateException("execute() answered " + served); + return; + } + } + } catch (Throwable t) { + borrower[0] = t; + } + }); + Thread closing = new Thread(() -> { + ready.countDown(); + awaitGo(go); + try { + if (Boolean.FALSE.equals(proxy.invoke(null, IS_CLOSED, null))) { + proxy.invoke(null, CLOSE, null); + } + } catch (Throwable t) { + closer[0] = t; + } + }); + + executing.start(); + closing.start(); + ready.await(); + go.countDown(); + executing.join(JOIN_TIMEOUT_MS); + closing.join(JOIN_TIMEOUT_MS); + Assertions.assertFalse(executing.isAlive() || closing.isAlive(), + "round " + round + ": both threads must finish"); + + Assertions.assertTrue(borrower[0] == null || borrower[0] instanceof SQLException, + "round " + round + ": the borrower may be served or refused with a SQLException, " + + "but saw: " + borrower[0]); + Assertions.assertTrue(closer[0] == null || closer[0] instanceof SQLException, + "round " + round + ": the closer may close or find the statement closed, " + + "but saw: " + closer[0]); + Assertions.assertEquals(Boolean.TRUE, proxy.invoke(null, IS_CLOSED, null), + "round " + round + ": the joined close must have closed the proxy for everyone"); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
