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 77a831aa2609f3e0ff5c01e862f536919017fc8d Author: opencode <[email protected]> AuthorDate: Sun Oct 25 21:00:56 2026 +0100 Fix lost and duplicated wake-ups in the pool wait protocol Destroying the last connection between a borrower's maxActive check and its registration as a waiter meant no wake-up notification was sent, leaving the borrower blocked - forever when maxWait was zero or less - although a slot was free. Re-check the limit right after registering the waiter and create a connection rather than waiting for a notification that may never arrive. Gate the wake-up placeholder in release() on the call actually having destroyed the connection. Releasing a connection that was already released - for example eviction racing an application return - still injected a placeholder for every call, inflating the pool size and handing out more physical connections than maxActive. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 26 ++- .../tomcat/jdbc/pool/TestReleaseWaiters.java | 203 +++++++++++++++++++++ webapps/docs/changelog.xml | 10 + 3 files changed, 236 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 08021f514f..bfe9608296 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 @@ -691,9 +691,11 @@ public class ConnectionPool { if (con == null) { return; } + boolean destroyed; try { con.lock(); - if (con.release()) { + destroyed = con.release(); + if (destroyed) { //counter only decremented once size.addAndGet(-1); con.setHandler(null); @@ -704,8 +706,10 @@ public class ConnectionPool { } // we've asynchronously reduced the number of connections // we could have threads stuck in idle.poll(timeout) that will never be - // notified - if (waitcount.get() > 0) { + // notified. Only a call that actually destroyed a connection opened a + // slot; offering a placeholder for a connection that was already + // released would run the pool over maxActive. + if (destroyed && waitcount.get() > 0) { if (!idle.offer(create(true))) { log.warn("Failed to add a new connection to the pool after releasing a connection " + "when at least one thread was waiting for a connection."); @@ -768,6 +772,22 @@ public class ConnectionPool { long timetowait = Math.max(0, maxWait - (System.currentTimeMillis() - now)); waitcount.incrementAndGet(); try { + // a connection may have been destroyed between the size check + // above and the registration of this waiter; such a releasing + // thread saw no waiter and will not notify this poll, so + // re-check the limit now rather than wait for a notification + // that may never arrive. Skip the case where a connection (or + // a wake-up placeholder) is already queued to be taken. + if (idle.isEmpty() && size.get() < getPoolProperties().getMaxActive()) { + //atomic duplicate check + if (size.addAndGet(1) > getPoolProperties().getMaxActive()) { + //if we got here, two threads passed through the first if + size.decrementAndGet(); + } else { + //create a connection, we're below the limit + return createConnection(now, con, username, password); + } + } //retrieve an existing connection con = idle.poll(timetowait, TimeUnit.MILLISECONDS); } catch (InterruptedException ex) { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestReleaseWaiters.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestReleaseWaiters.java new file mode 100644 index 0000000000..1613d5eaf1 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestReleaseWaiters.java @@ -0,0 +1,203 @@ +/* + * 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.ResultSet; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The {@code waitcount} protocol between {@code release()} and + * {@code borrowConnection()} must neither lose a wake-up nor double-count + * one. A borrower that registers as a waiter just after the last connection + * was destroyed must still get served (the releasing thread saw no waiter and + * sends no notification), and only a release that actually destroyed a + * connection may hand out a wake-up placeholder - a second release of the + * same connection would otherwise run the pool over maxActive. + */ +class TestReleaseWaiters extends JdbcPoolBaseTest { + + /* + * Pool properties that notice - and widen - the window inside + * borrowConnection() between the maxActive check that failed and the + * registration of the waiter. The maxWait computation at that spot is the + * only pool call that runs there, so the probe fires exactly in the + * window the release() notification can miss. The window signal is a + * one-shot: whoever arms it gets exactly the next getMaxWait() call, so + * nothing else may poll the property between arm() and the borrower's + * arrival. + */ + private static final class SlotReleaseProbe extends PoolProperties { + + private static final long serialVersionUID = 1L; + + private final CountDownLatch inWindow = new CountDownLatch(1); + private final AtomicBoolean armed = new AtomicBoolean(); + + @Override + public int getMaxWait() { + if (armed.compareAndSet(true, false)) { + inWindow.countDown(); + sleep(300); + } + return super.getMaxWait(); + } + + void arm() { + armed.set(true); + } + + boolean awaitInWindow() throws InterruptedException { + return inWindow.await(5, TimeUnit.SECONDS); + } + } + + @Test + void destroyingTheLastConnectionJustBeforeAWaiterRegistersStillLeavesTheSlotUsable() + throws Exception { + SlotReleaseProbe props = new SlotReleaseProbe(); + props.setUrl(getUrl()); + props.setDriverClassName("org.h2.Driver"); + props.setUsername("sa"); + props.setPassword(""); + props.setJmxEnabled(false); + props.setInitialSize(0); + props.setMinIdle(0); + props.setMaxActive(1); + // <= 0 means: wait forever for a notification + props.setMaxWait(-1); + DataSource ds = manage(new DataSource()); + ds.setPoolProperties(props); + ds.createPool(); + + Connection holder = ds.getConnection(); // the only slot is taken + + final Connection[] served = new Connection[1]; + final Throwable[] failure = new Throwable[1]; + Thread borrower = new Thread(() -> { + try { + served[0] = ds.getConnection(); + } catch (Throwable x) { + failure[0] = x; + } + }, "probe-borrower"); + props.arm(); + borrower.start(); + assertTrue(props.awaitInWindow(), + "borrower should have reached the window between the maxActive " + + "check and the waiter registration"); + + // Destroy the only connection while no waiter is registered yet: the + // release sees waitcount == 0 and sends no notification. Bumping the + // pool version makes the return path discard the connection instead + // of parking it in idle. + ds.getPool().purgeOnReturn(); + holder.close(); + + borrower.join(AWAIT_TIMEOUT_MS); + if (borrower.isAlive()) { + borrower.interrupt(); // unstick it before failing + fail("a borrower whose notification was lost waits forever even " + + "though a slot is free"); + } + assertNull(failure[0], "the borrower should have been served"); + try (ResultSet rs = served[0].createStatement().executeQuery("SELECT 1")) { + assertTrue(rs.next()); + } + awaitSessions(1); // the old one destroyed, the borrower's created + served[0].close(); + assertEquals(1, ds.getIdle()); + } + + @Test + void duplicateReleaseOfTheSameConnectionDoesNotCreateAnExtraPlaceholder() + throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxWait(4000); + ds.createPool(); + + Connection holder = ds.getConnection(); // maxActive is reached + PooledConnection pc = + interceptorOf(holder, ProxyConnection.class).getConnection(); + ConnectionPool pool = ds.getPool(); + + final Thread[] waiters = new Thread[2]; + final Connection[] served = new Connection[2]; + for (int i = 0; i < waiters.length; i++) { + final int slot = i; + waiters[i] = new Thread(() -> { + try { + served[slot] = ds.getConnection(); + } catch (Throwable ignore) { + // timing out is the correct outcome for the loser + } + }, "waiter-" + i); + waiters[i].start(); + } + awaitUntil(() -> ds.getWaitCount() == 2); + + // Destroy the connection: exactly one waiter wakes up on the one + // wake-up placeholder. + pool.release(pc); + awaitUntil(() -> ds.getWaitCount() == 1); + + // A second release of the same, already destroyed connection must not + // wake the second waiter too - there is only one slot. + pool.release(pc); + + // The wake-up placeholder was the notification for the one real + // destruction; consuming it must leave exactly one waiter behind. If + // the duplicate release handed out its own placeholder, the remaining + // waiter wakes up too - one slot cannot serve two borrowers. Give it + // a good fraction of maxWait, it is handed the placeholder directly + // and would wake within milliseconds if it were woken at all. + sleep(500); + assertEquals(1, ds.getWaitCount(), + "the duplicate release must not wake the remaining waiter"); + + for (Thread waiter : waiters) { + waiter.join(AWAIT_TIMEOUT_MS); + if (waiter.isAlive()) { + waiter.interrupt(); + fail("waiter is stuck"); + } + } + int servedCount = (served[0] == null ? 0 : 1) + (served[1] == null ? 0 : 1); + assertEquals(1, servedCount, + "only the destruction of the connection should have opened a slot"); + awaitSessions(1); + for (Connection c : served) { + if (c != null) { + c.close(); + } + } + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index ab94333ab3..1d15d6373a 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -753,6 +753,16 @@ flag after the physical connect completes, like the return side already did, and disconnects the new connection. (remm) </fix> + <fix> + Correct two races in the wake-up protocol between releasing and + waiting threads in the connection pool. Destroying the last connection + while a borrower was about to register as a waiter could leave that + borrower blocked although a slot was free (forever when + <code>maxWait</code> was set to 0 or less), and releasing an already + released connection still injected an extra wake-up placeholder, + inflating the pool size and allowing more physical connections than + <code>maxActive</code>. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
