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 447ac5e9119305885446a23232aa3b6439b7d605 Author: opencode <[email protected]> AuthorDate: Tue Nov 3 16:44:35 2026 +0100 Hold the claimed windows against an Error from the driver Three paths take a connection out of a queue, work on it over the driver, and then hand it to the other queue or release it: the borrow that reconnects before handing the session over, the return that asks the driver whether the session is closed and settles the transaction on the way back, and the idle sweep that tests and reconnects outside either path. Each of the three runs guarded, because a throw escaping the window leaves the connection in neither queue - invisible to the abandoned check, to the eviction sweep and to the close drain alike, never disconnected, and gone from the maxActive budget for the life of the pool. The guards were written for an exception. An Error arriving over the same call was never their subject, so it walked straight out of the window, doing exactly the harm the guard next to it exists to prevent - and the comment on two of them says so in as many words. The arrival is not exotic. A driver, or the wrapper around a driver, is loaded through whichever class loader the pool was configured with, and the ordinary failure of a web application loader that has stopped is a LinkageError, the NoClassDefFoundError of a class its context no longer holds. Such an Error on a maxAge reconnect ended that way: the sweep left the pool with a connection counted in its size and in neither of its queues, every later borrow timing out on a full pool, and the pool's own close unable to reach the session to give the slot back. The three guards now take Throwable and apply the module's fatal policy first, so what cannot be recovered from still propagates and everything short of that is read as the failed check or refused borrow each of them already reports - the return closing quietly over a driver that cannot answer for its session, the borrow answered with the checked failure a borrow owes, the sweep taking the connection out of service and ending its round. One test per window, each on a pool of exactly one slot so that a connection lost between the queues is visible immediately as a pool that can hand out nothing, and each requiring the whole outcome the guard promises: the session disconnected, the slot back in the budget, the work not broken by the driver's failure, and the next borrow served afresh. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 33 ++- .../pool/TestThrowingDriverOnClaimedPaths.java | 282 +++++++++++++++++++++ 2 files changed, 312 insertions(+), 3 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index a4b336f39b..1fc81ca409 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -1652,7 +1652,20 @@ public class ConnectionPool { // Validation failed. throw new SQLException(MessageFactory.get("connectionPool.failedToValidateNewConnection")); } - } catch (Exception x) { + } catch (Throwable x) { + // The module's fatal policy runs first, as everywhere else. + // What is left is the refused borrow this guard already + // reports: the reconnect and the validation above reach the + // driver over a connection this borrow has claimed out of the + // idle queue, and a throwable that unwound them now would leave + // that connection in neither queue - invisible to the abandoned + // check, to the sweep and to the close drain alike, and gone + // from the budget for the life of the pool. A LinkageError is + // an ordinary arrival behind a driver loaded or wrapped through + // a web application class loader, and a borrow the driver could + // not carry out is what the borrower is owed, as the checked + // exception a borrow answers with. + ExceptionUtils.handleThrowable(x); if (credentialsDiffer && !reconnected && con.isInitialized() && !con.isDiscarded()) { // The attempt was refused before it gave up the session // this connection was holding: that session is open, it @@ -1977,7 +1990,15 @@ public class ConnectionPool { } release(con); } - } catch (RuntimeException x) { + } catch (Throwable x) { + // The module's fatal policy runs first: what a driver + // fails with an Error is the same failed check this + // guard already retires on an unchecked exception, and + // the Error a driver wrapper loaded through a web + // application class loader throws once that context has + // stopped is that loader's ordinary failure, not a sign + // the JVM cannot be carried on. + ExceptionUtils.handleThrowable(x); // Everything above runs after the connection was // claimed from the busy queue and calls into the // driver - a closed check, a transaction termination, @@ -2222,7 +2243,13 @@ public class ConnectionPool { } else { keep = reconnectIfExpired(con) && con.validate(PooledConnection.VALIDATE_IDLE); } - } catch (RuntimeException x) { + } catch (Throwable x) { + // The module's fatal policy runs first, as everywhere + // else: a non-fatal Error from behind one of the calls + // below is a check that cannot vouch for the session, + // which is what this guard already retires a driver's + // unchecked exception as. + ExceptionUtils.handleThrowable(x); // The checks above swallow every checked driver // failure, but an unchecked throw from behind one of // them - a validation ending a transaction is one diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDriverOnClaimedPaths.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDriverOnClaimedPaths.java new file mode 100644 index 0000000000..5fd468abf1 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestThrowingDriverOnClaimedPaths.java @@ -0,0 +1,282 @@ +/* + * 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.Field; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.Driver; +import java.sql.DriverPropertyInfo; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Logger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * Three paths take a connection out of a queue, call into the driver over + * it, and then either hand it to the other queue or release it: the borrow + * that reconnects before handing the session over, the return that checks + * and terminates the transaction on the way back, and the idle sweep that + * tests and reconnects outside either path. Each of them is guarded against + * an unchecked exception, because a throw escaping the window between the + * two queues leaves the connection in neither of them - invisible to the + * abandoned check, to the eviction sweep and to the close drain alike, never + * disconnected, and gone from the maxActive budget for the life of the pool. + * <p> + * An {@code Error} reaches those calls the same way it reaches the rest of + * the pool: the driver, or the wrapper around it that a web application + * class loader loaded, fails with a {@link LinkageError} - the + * {@link NoClassDefFoundError} of a context that has since stopped is that + * loader's ordinary failure, not an extraordinary one. A guard that catches + * only exceptions lets exactly that arrival strand the connection, which is + * the loss the guard exists to prevent. The three tests below put an Error + * behind each of the three windows and require of each the outcome the + * guard's own comment promises: the connection out of service, its slot + * returned, and the work - the close, the borrow, the sweep - reporting the + * failure rather than being broken by it. + */ +class TestThrowingDriverOnClaimedPaths extends JdbcPoolBaseTest { + + /* + * The physical method the connection should fail from with an Error, or + * null to let everything through, and the one-shot for the driver's own + * connect(). Static, because the driver is instantiated by class name and + * the tests alone decide when the window opens and shuts. + */ + private static final AtomicReference<String> THROW_FROM = new AtomicReference<>(); + private static final AtomicBoolean FAIL_NEXT_CONNECT = new AtomicBoolean(); + private static final AtomicInteger CONNECTS = new AtomicInteger(); + + private static void resetDriver() { + THROW_FROM.set(null); + FAIL_NEXT_CONNECT.set(false); + CONNECTS.set(0); + } + + /* + * One-slot pool on the throwing driver, with no competing sweeper: the + * single slot makes a connection that went missing between the queues + * visible at once, as the pool can no longer hand anything out. + */ + private DataSource oneSlotPool() throws SQLException { + DataSource ds = newDataSource(); + ds.setDriverClassName(ThrowingDriver.class.getName()); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(1); + // a real maxAge, so the reconnect is really attempted; the tests + // expire the connection themselves, no clock waiting involved + ds.getPoolProperties().setMaxAge(60000); + // no timer: the sweep and the borrow are driven by the tests + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(0); + ds.getPoolProperties().setMaxWait(15000); + ds.createPool(); + return ds; + } + + /* + * Expire a connection deterministically: rewind its last-connected clock + * instead of waiting out a small maxAge. + */ + private static void forceExpired(PooledConnection pc) throws Exception { + Field lastConnected = PooledConnection.class.getDeclaredField("lastConnected"); + lastConnected.setAccessible(true); + lastConnected.setLong(pc, System.currentTimeMillis() - 120000L); + } + + /* The state a claimed window that failed correctly leaves behind. */ + private void assertNothingStranded(DataSource ds) throws Exception { + Assertions.assertEquals(0, ds.getPool().getIdle()); + Assertions.assertEquals(0, ds.getPool().getActive()); + Assertions.assertEquals(0, ds.getPool().getSize()); + awaitSessions(0); + // and the single slot was not lost: the next borrow connects afresh + try (Connection again = ds.getConnection()) { + Assertions.assertNotNull(again); + } + Assertions.assertEquals(0, ds.getPool().getActive()); + ds.close(); + awaitSessions(0); + } + + @Test + void anErrorFromTheDriverOnTheReturnDoesNotStrandTheConnection() throws Exception { + resetDriver(); + DataSource ds = oneSlotPool(); + Connection first = ds.getConnection(); + awaitSessions(1); + + // shouldClose() asks the driver whether the session is closed, on the + // way back from the busy queue + THROW_FROM.set("isClosed"); + try { + // shouldClose() asks the driver whether the session is closed + // after the return claimed the connection out of the busy queue. + // The driver's Error must not unwind the return with the + // connection in neither queue. + Assertions.assertDoesNotThrow(first::close); + } finally { + THROW_FROM.set(null); + } + + assertNothingStranded(ds); + } + + @Test + void anErrorFromTheDriverOnTheIdleSweepDoesNotStrandTheConnection() throws Exception { + resetDriver(); + DataSource ds = oneSlotPool(); + Connection first = ds.getConnection(); + awaitSessions(1); + PooledConnection pc = (PooledConnection) first.unwrap(PooledConnection.class); + first.close(); + forceExpired(pc); + + FAIL_NEXT_CONNECT.set(true); + try { + // reconnectIfExpired() closes the expired session and asks the + // driver for another one, inside the window in which the sweep has + // claimed the connection out of the idle queue. The sweep has to + // read the Error as the failed check it is and end its round. + Assertions.assertDoesNotThrow(() -> ds.getPool().testAllIdle()); + } finally { + FAIL_NEXT_CONNECT.set(false); + } + + Assertions.assertTrue(CONNECTS.get() >= 2, + "the sweep must have reached the driver's connect() the way the failure was armed"); + assertNothingStranded(ds); + } + + @Test + void anErrorFromTheDriverOnTheBorrowFailsTheBorrowAndKeepsTheSlot() throws Exception { + resetDriver(); + DataSource ds = oneSlotPool(); + Connection first = ds.getConnection(); + awaitSessions(1); + PooledConnection pc = (PooledConnection) first.unwrap(PooledConnection.class); + first.close(); + forceExpired(pc); + + FAIL_NEXT_CONNECT.set(true); + try { + // the borrow that reconnects an expired connection calls the + // driver over the claimed window too; the borrower is owed the + // failure the pool owes any refused borrow, and owed it as the + // checked exception a borrow reports in + Assertions.assertThrows(SQLException.class, ds::getConnection); + } finally { + FAIL_NEXT_CONNECT.set(false); + } + + assertNothingStranded(ds); + } + + /** + * Driver handing out the real H2 connections wrapped in a proxy that + * fails the currently armed method, or its own connect() when that is + * armed once, with an Error. Configured through {@code driverClassName}, + * so it must stay a public static class with a no-arg constructor. + */ + public static class ThrowingDriver implements Driver { + + private final Driver delegate = new org.h2.Driver(); + + @Override + public Connection connect(String url, Properties info) throws SQLException { + // Counted before the window: an attempt that failed is still one + // the test needs to see the sweep reached + CONNECTS.incrementAndGet(); + if (FAIL_NEXT_CONNECT.compareAndSet(true, false)) { + // The shape of a stopped context: the driver class itself is + // still here, but a class it needs is not + throw new NoClassDefFoundError( + "the context that loaded this driver wrapper has stopped"); + } + Connection connection = delegate.connect(url, info); + if (connection == null) { + return null; + } + return (Connection) Proxy.newProxyInstance( + ThrowingDriver.class.getClassLoader(), + new Class<?>[] { Connection.class }, + new ThrowingHandler(connection)); + } + + @Override + public boolean acceptsURL(String url) throws SQLException { + return delegate.acceptsURL(url); + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) throws SQLException { + return delegate.getPropertyInfo(url, info); + } + + @Override + public int getMajorVersion() { + return delegate.getMajorVersion(); + } + + @Override + public int getMinorVersion() { + return delegate.getMinorVersion(); + } + + @Override + public boolean jdbcCompliant() { + return delegate.jdbcCompliant(); + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + return delegate.getParentLogger(); + } + } + + /* Fails the armed method by name with an Error; delegates the rest. */ + private static class ThrowingHandler implements InvocationHandler { + + private final Connection vendor; + + ThrowingHandler(Connection vendor) { + this.vendor = vendor; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if (method.getName().equals(THROW_FROM.get())) { + throw new NoClassDefFoundError("simulated stopped context from " + method.getName() + "()"); + } + try { + return method.invoke(vendor, args); + } catch (InvocationTargetException x) { + throw x.getCause(); + } + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
