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 6e31054166c7be0853a221b1cbb1aeea23265452 Author: opencode <[email protected]> AuthorDate: Thu Oct 29 23:13:02 2026 +0100 FairBlockingQueue: enforce the Iterator.remove() contract in FairIterator FairIterator.remove() silently accepted calls without an intervening next() and repeated remove() calls: it only checked whether the last-returned field was non-null. A remove() before the first next() was a silent no-op and a second remove() could act on the live queue a second time, with no IllegalStateException as the Iterator contract requires. FairIterator now tracks whether a removal is permitted, throws IllegalStateException when it is not, and clears the last-returned element after a successful removal. The FairIterator javadoc now also states that the iterator walks a snapshot while remove() deletes from the live queue, so a re-offered same object is removed as well - the same weakly consistent behaviour as the JDK's snapshot iterators. Tests pin the exception cases, live-queue removal and Collection.removeIf. --- .../apache/tomcat/jdbc/pool/FairBlockingQueue.java | 21 ++++++++--- .../tomcat/jdbc/pool/TestFairBlockingQueue.java | 41 ++++++++++++++++++++-- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/FairBlockingQueue.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/FairBlockingQueue.java index 4b23b33ef9..60d1719bad 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/FairBlockingQueue.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/FairBlockingQueue.java @@ -755,12 +755,18 @@ public class FairBlockingQueue<E> implements BlockingQueue<E> { } /** - * Iterator safe from concurrent modification exceptions. + * Iterator safe from concurrent modification exceptions. The iterator walks a snapshot of the queue contents taken + * when it was created, but {@link #remove()} removes the last returned element - or, for element types with value + * semantics, an element equal to it - from the live queue. + * An element that was taken out of the queue and offered back as the same object between the snapshot and the + * removal is therefore also removed from the live queue - like the weakly consistent iterators of the JDK, this + * iterator makes no promise about seeing the current state of the queue. */ protected class FairIterator implements Iterator<E> { E[] elements = null; int index; E element = null; + boolean canRemove = false; /** * Creates a new FairIterator with a snapshot of the current queue contents. @@ -789,20 +795,27 @@ public class FairBlockingQueue<E> implements BlockingQueue<E> { throw new NoSuchElementException(); } element = elements[index++]; + canRemove = true; return element; } @Override public void remove() { + if (!canRemove) { + // Either next() has not been called yet, or the element it + // returned has already been removed - the Iterator contract + // demands an IllegalStateException, not a silent no-op + throw new IllegalStateException(); + } final ReentrantLock lock = FairBlockingQueue.this.lock; lock.lock(); try { - if (element != null) { - FairBlockingQueue.this.items.remove(element); - } + FairBlockingQueue.this.items.remove(element); } finally { lock.unlock(); } + element = null; + canRemove = false; } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFairBlockingQueue.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFairBlockingQueue.java index fc89954c82..955caad418 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFairBlockingQueue.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestFairBlockingQueue.java @@ -18,6 +18,7 @@ package org.apache.tomcat.jdbc.pool; import java.util.ArrayList; import java.util.Collections; +import java.util.Iterator; import java.util.List; import java.util.concurrent.CancellationException; import java.util.concurrent.CountDownLatch; @@ -39,8 +40,8 @@ import org.junit.jupiter.api.Test; * polls, the terminal failure of an asynchronous poll whose timed get * times out, the termination of every outstanding poll once the waiters * are cancelled, the collection operations the queue deliberately - * leaves unsupported, and the rejection of null elements by every - * insertion path. + * leaves unsupported, the rejection of null elements by every + * insertion path, and the removal contract of the queue iterator. */ class TestFairBlockingQueue { @@ -94,6 +95,42 @@ class TestFairBlockingQueue { Assertions.assertTrue(seen.contains("added")); } + @Test + void iteratorRemoveRequiresAnInterveningNext() { + FairBlockingQueue<String> queue = new FairBlockingQueue<>(); + queue.offer("drop"); + queue.offer("keep"); + Iterator<String> iterator = queue.iterator(); + Assertions.assertThrows(IllegalStateException.class, iterator::remove, + "remove() before the first next() must fail per the Iterator contract"); + iterator.next(); + iterator.remove(); // the legal call + Assertions.assertThrows(IllegalStateException.class, iterator::remove, + "a second remove() without an intervening next() must fail per the Iterator contract"); + Assertions.assertEquals(1, queue.size(), + "the failed second remove() must not remove a further element"); + } + + @Test + void iteratorRemoveTakesTheElementOutOfTheLiveQueue() { + FairBlockingQueue<String> queue = new FairBlockingQueue<>(); + queue.offer("drop"); + queue.offer("keep"); + for (Iterator<String> iterator = queue.iterator(); iterator.hasNext();) { + if ("drop".equals(iterator.next())) { + iterator.remove(); + } + } + Assertions.assertEquals(1, queue.size(), "the removed element must leave the live queue"); + Assertions.assertFalse(queue.contains("drop")); + Assertions.assertTrue(queue.contains("keep"), "the other elements must stay in the queue"); + // Collection.removeIf is a default method that routes through + // Iterator.remove(), so it must honour the same contract + Assertions.assertTrue(queue.removeIf("keep"::equals)); + Assertions.assertEquals(0, queue.size()); + Assertions.assertFalse(queue.removeIf("keep"::equals)); + } + @Test void nullElementsAreRejectedByEveryInsertionPath() throws Exception { FairBlockingQueue<String> queue = new FairBlockingQueue<>(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
