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 3f7e90b3d1beaf70341165d586434f7b4d4d7a9a Author: opencode <[email protected]> AuthorDate: Sun Oct 25 18:45:40 2026 +0100 Do not hand out connections created during connection pool closure borrowConnection() checked the closed flag once at entry. A borrower that passed that check and then took the create path could complete its physical connect while close(force) had already set the closed flag and finished draining the idle and busy queues. The new connection was offered into busy - which shutdown tracking already passed over - and handed to the caller as a working connection from a closed pool, potentially keeping the physical connection open indefinitely. Re-check the closed flag in createConnection() right after the physical connect succeeded, the point of no return for handing the connection to the caller, and fail the borrow with an SQLException; the existing error path disconnects the new connection and gives back the size slot. The return path honours the closed flag the same way in shouldClose(), so this makes the borrow side symmetric. The regression test drives the race with a driver whose connect() sleeps and verifies, from the database side, that the connection created during closure is disconnected. While here, drop assertions in TestInterruptedPoolClose that consulted DataSource.getSize() after close - DataSourceProxy nulls its pool reference before closing, so getSize() lazily built a fresh empty pool instead of inspecting the closed one. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 7 ++ .../tomcat/jdbc/pool/TestCloseDuringBorrow.java | 134 +++++++++++++++++++++ .../tomcat/jdbc/pool/TestInterruptedPoolClose.java | 12 +- webapps/docs/changelog.xml | 8 ++ 4 files changed, 152 insertions(+), 9 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 cc9df5252b..e9767a8ce0 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 @@ -829,6 +829,13 @@ public class ConnectionPool { //connect and validate the connection con.lock(); con.connect(); + if (isClosed()) { + // the pool was closed while this physical connection was + // being created; the shutdown drain has already passed over + // the queues, so neither hand the connection out nor register + // it in busy - the error path below disconnects it + throw new SQLException("Connection pool closed."); + } if (con.validate(PooledConnection.VALIDATE_INIT)) { //no need to lock a new one, its not contented con.setTimestamp(now); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseDuringBorrow.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseDuringBorrow.java new file mode 100644 index 0000000000..c06c07a375 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestCloseDuringBorrow.java @@ -0,0 +1,134 @@ +/* + * 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.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.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Logger; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A borrower whose physical connect() is still in flight when the pool is + * force-closed must not be handed the connection it is creating: the drain + * has already passed over the queues, so that connection would sit in + * {@code busy} of a closed pool, never closed again. The borrow side, like + * the return side, has to honour the closed flag at its point of no return. + */ +class TestCloseDuringBorrow extends JdbcPoolBaseTest { + + /* A driver whose connect() is slow enough for a close() to complete + while a connection attempt is in flight. It signals when it enters + connect() so the test can close the pool mid-attempt deterministically. */ + public static class SlowConnectDriver implements Driver { + + static final CountDownLatch CONNECT_STARTED = new CountDownLatch(1); + static final long CONNECT_DELAY_MS = 1500; + + @Override + public Connection connect(String url, Properties info) throws SQLException { + CONNECT_STARTED.countDown(); + try { + Thread.sleep(CONNECT_DELAY_MS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new SQLException(e); + } + return new org.h2.Driver().connect(url, info); + } + + @Override + public boolean acceptsURL(String url) { + return url != null && url.startsWith("jdbc:h2:"); + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) { + return new DriverPropertyInfo[0]; + } + + @Override + public int getMajorVersion() { + return 0; + } + + @Override + public int getMinorVersion() { + return 0; + } + + @Override + public boolean jdbcCompliant() { + return false; + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException(); + } + } + + @Test + void closeDuringInFlightCreateDoesNotHandOutTheNewConnection() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setDriverClassName(SlowConnectDriver.class.getName()); + ds.getPoolProperties().setInitialSize(0); + ds.createPool(); + + AtomicReference<Throwable> failure = new AtomicReference<>(); + AtomicReference<Connection> borrowed = new AtomicReference<>(); + Thread borrower = new Thread(() -> { + try { + borrowed.set(ds.getConnection()); + } catch (Exception e) { + failure.set(e); + } + }, "borrower-in-flight-during-close"); + borrower.setDaemon(true); + borrower.start(); + + // close while the physical connect() is in flight: the driver signals + // when it enters connect() and sleeps there for CONNECT_DELAY_MS + assertTrue(SlowConnectDriver.CONNECT_STARTED.await(AWAIT_TIMEOUT_MS, + TimeUnit.MILLISECONDS), + "the borrower must be inside the driver's connect()"); + ds.close(true); + + borrower.join(AWAIT_TIMEOUT_MS); + assertFalse(borrower.isAlive(), "the borrower must not outlive the close"); + + assertNotNull(failure.get(), + "the borrower must not be handed a connection from the closed pool"); + assertTrue(failure.get() instanceof SQLException, + "the borrower must be rejected with a SQLException, not " + failure.get()); + assertNull(borrowed.get()); + awaitSessions(0); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestInterruptedPoolClose.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestInterruptedPoolClose.java index f2fab578e1..8ade1e9cfe 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestInterruptedPoolClose.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestInterruptedPoolClose.java @@ -20,7 +20,6 @@ import java.sql.Connection; import java.util.ArrayList; import java.util.List; -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; @@ -71,9 +70,7 @@ class TestInterruptedPoolClose extends JdbcPoolBaseTest { assertTrue(outcome.interruptedAfterClose, "propagateInterruptState=true must keep the interrupt status across close()"); - assertEquals(0, ds.getSize()); - assertEquals(0, poolSessions(), - "a forced close from an interrupted thread must close borrowed connections"); + awaitSessions(0); } @Test @@ -96,9 +93,7 @@ class TestInterruptedPoolClose extends JdbcPoolBaseTest { assertTrue(outcome.interruptedAfterClose, "propagateInterruptState=true must keep the interrupt status across close()"); - assertEquals(0, ds.getSize()); - assertEquals(0, poolSessions(), - "a forced close must drain both queues despite the interrupt"); + awaitSessions(0); } @Test @@ -115,7 +110,6 @@ class TestInterruptedPoolClose extends JdbcPoolBaseTest { assertFalse(outcome.interruptedAfterClose, "propagateInterruptState=false must not leave the interrupt status set"); - assertEquals(0, poolSessions(), - "the drain must proceed after a consumed interrupt"); + awaitSessions(0); } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 216dced4a3..1bc5c30907 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -743,6 +743,14 @@ connections. The shutdown drain now always completes and the interrupted status is propagated once it has. (remm) </fix> + <fix> + Correct a defect that meant a connection borrow that was in flight + while the pool was force-closed could be handed a newly created, + working connection from the closed pool, leaving that physical + connection open indefinitely. The borrow side now honours the closed + flag after the physical connect completes, like the return side + already did, and disconnects the new connection. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
