This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 6b1020ca6f7e28c6c008c572c9b6cd2fa33bb921 Author: opencode <[email protected]> AuthorDate: Mon Oct 26 12:42:24 2026 +0100 Remove the unfinished, dead MultiLockFairBlockingQueue class MultiLockFairBlockingQueue was an experimental, striped variant of the fair blocking queue that was never finished. Its only instantiation was commented out, no configuration property existed to select it, and the async-retrieval branch that could recognise it was therefore unreachable, so the class was dead code and must be judged by the impact it would have if ever wired in. Judged that way, three high severity defects made it unsafe to keep: - offer(), poll() and take() only ever look at their own stripe and the put and poll round-robin counters are independent, so items and waiters end up on unrelated stripes. A borrower could then block indefinitely while an idle connection sat in another stripe, a non-blocking poll() could report the queue empty although it was not (making the pool surface spurious exhaustion errors on ordinary traffic), and the fairness the class javadoc promises was per-stripe only. - poll(timeout) propagated an InterruptedException without deregistering its waiter latch, so the next offer routed to that stripe handed a pooled connection directly to the abandoned latch: one permanently lost connection per interrupted borrower. - The FairIterator constructor never populated its snapshot array, so iterator() always yielded nothing; wiring the class in would have silently disabled the idle connection sweep, including the maxIdle/minIdle/maxAge eviction and the testWhileIdle validation. Fixing the first defect means restructuring the cross-stripe rendezvous of a class no configuration can enable, so the class is removed instead of repaired. Fair queueing remains available through FairBlockingQueue. Also removes the now unreachable instanceof branch in ConnectionPool.getConnectionAsync(), the commented-out instantiation in ConnectionPool.init(), the class's spotbugs false-positive filter entry, and its remaining references in the jdbc-pool test plan. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 4 - .../jdbc/pool/MultiLockFairBlockingQueue.java | 591 --------------------- res/spotbugs/filter-false-positives.xml | 6 - webapps/docs/changelog.xml | 15 + 4 files changed, 15 insertions(+), 601 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 8dc3e8d7f1..1326d58f9d 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 @@ -192,9 +192,6 @@ public class ConnectionPool { if (idle instanceof FairBlockingQueue<?>) { Future<PooledConnection> pcf = ((FairBlockingQueue<PooledConnection>)idle).pollAsync(); return new ConnectionFuture(pcf); - } else if (idle instanceof MultiLockFairBlockingQueue<?>) { - Future<PooledConnection> pcf = ((MultiLockFairBlockingQueue<PooledConnection>)idle).pollAsync(); - return new ConnectionFuture(pcf); } else { throw new SQLException("Connection pool is misconfigured, doesn't support async retrieval. Set the 'fair' property to 'true'"); } @@ -512,7 +509,6 @@ public class ConnectionPool { //make space for 10 extra in case we flow over a bit if (properties.isFairQueue()) { idle = new FairBlockingQueue<>(); - //idle = new MultiLockFairBlockingQueue<PooledConnection>(); //idle = new LinkedTransferQueue<PooledConnection>(); //idle = new ArrayBlockingQueue<PooledConnection>(properties.getMaxActive(),false); } else { diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/MultiLockFairBlockingQueue.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/MultiLockFairBlockingQueue.java deleted file mode 100644 index 4b78617b45..0000000000 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/MultiLockFairBlockingQueue.java +++ /dev/null @@ -1,591 +0,0 @@ -/* - * 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.util.ArrayList; -import java.util.Collection; -import java.util.Iterator; -import java.util.LinkedList; -import java.util.NoSuchElementException; -import java.util.concurrent.BlockingQueue; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.Future; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.locks.ReentrantLock; - -/** - * <b>EXPERIMENTAL AND NOT YET COMPLETE!</b> - * - * - * An implementation of a blocking queue with fairness waiting and lock dispersal to avoid contention. - * invocations to method poll(...) will get handed out in the order they were received. - * Locking is fine grained, a shared lock is only used during the first level of contention, waiting is done in a - * lock per thread basis so that order is guaranteed once the thread goes into a suspended monitor state. - * <br> - * Not all of the methods of the {@link java.util.concurrent.BlockingQueue} are implemented. - * - * @param <E> Type of element in the queue - */ - -public class MultiLockFairBlockingQueue<E> implements BlockingQueue<E> { - - final int LOCK_COUNT = Runtime.getRuntime().availableProcessors(); - - final AtomicInteger putQueue = new AtomicInteger(0); - final AtomicInteger pollQueue = new AtomicInteger(0); - - /** - * Get the next put index. - * @return the next put index - */ - public int getNextPut() { - int idx = Math.abs(putQueue.incrementAndGet()) % LOCK_COUNT; - return idx; - } - - /** - * Get the next poll index. - * @return the next poll index - */ - public int getNextPoll() { - int idx = Math.abs(pollQueue.incrementAndGet()) % LOCK_COUNT; - return idx; - } - /** - * Phase one entry lock in order to give out - * per-thread-locks for the waiting phase we have - * a phase one lock during the contention period. - */ - private final ReentrantLock[] locks = new ReentrantLock[LOCK_COUNT]; - - /** - * All the objects in the pool are stored in a simple linked list - */ - final LinkedList<E>[] items; - - /** - * All threads waiting for an object are stored in a linked list - */ - final LinkedList<ExchangeCountDownLatch<E>>[] waiters; - - /** - * Creates a new fair blocking queue. - */ - @SuppressWarnings("unchecked") // Can create arrays of generic types - public MultiLockFairBlockingQueue() { - items = new LinkedList[LOCK_COUNT]; - waiters = new LinkedList[LOCK_COUNT]; - for (int i=0; i<LOCK_COUNT; i++) { - items[i] = new LinkedList<>(); - waiters[i] = new LinkedList<>(); - locks[i] = new ReentrantLock(false); - } - } - - //------------------------------------------------------------------ - // USED BY CONPOOL IMPLEMENTATION - //------------------------------------------------------------------ - /** - * Will always return true, queue is unbounded. - * {@inheritDoc} - */ - @Override - public boolean offer(E e) { - int idx = getNextPut(); - //during the offer, we will grab the main lock - final ReentrantLock lock = this.locks[idx]; - lock.lock(); - ExchangeCountDownLatch<E> c = null; - try { - //check to see if threads are waiting for an object - if (!waiters[idx].isEmpty()) { - //if threads are waiting grab the latch for that thread - c = waiters[idx].poll(); - //give the object to the thread instead of adding it to the pool - c.setItem(e); - } else { - //we always add first, so that the most recently used object will be given out - items[idx].addFirst(e); - } - } finally { - lock.unlock(); - } - //if we exchanged an object with another thread, wake it up. - if (c!=null) { - c.countDown(); - } - //we have an unbounded queue, so always return true - return true; - } - - /** - * Will never timeout, as it invokes the {@link #offer(Object)} method. - * Once a lock has been acquired, the - * {@inheritDoc} - */ - @Override - public boolean offer(E e, long timeout, TimeUnit unit) throws InterruptedException { - return offer(e); - } - - /** - * Fair retrieval of an object in the queue. - * Objects are returned in the order the threads requested them. - * {@inheritDoc} - */ - @Override - public E poll(long timeout, TimeUnit unit) throws InterruptedException { - int idx = getNextPoll(); - E result = null; - final ReentrantLock lock = this.locks[idx]; - try { - //acquire the global lock until we know what to do - lock.lock(); - //check to see if we have objects - result = items[idx].poll(); - if (result==null && timeout>0) { - //the queue is empty we will wait for an object - ExchangeCountDownLatch<E> c = new ExchangeCountDownLatch<>(1); - //add to the bottom of the wait list - waiters[idx].addLast(c); - //unlock the global lock - lock.unlock(); - //wait for the specified timeout - if (!c.await(timeout, unit)) { - //if we timed out, remove ourselves from the waitlist - lock.lock(); - waiters[idx].remove(c); - lock.unlock(); - } - //return the item we received, can be null if we timed out - result = c.getItem(); - } else { - //we have an object, release - lock.unlock(); - } - } finally { - if (lock.isHeldByCurrentThread()) { - lock.unlock(); - } - } - return result; - } - - /** - * Request an item from the queue asynchronously - * @return - a future pending the result from the queue poll request - */ - public Future<E> pollAsync() { - int idx = getNextPoll(); - Future<E> result = null; - final ReentrantLock lock = this.locks[idx]; - try { - //grab the global lock - lock.lock(); - //check to see if we have objects in the queue - E item = items[idx].poll(); - if (item==null) { - //queue is empty, add ourselves as waiters - ExchangeCountDownLatch<E> c = new ExchangeCountDownLatch<>(1); - waiters[idx].addLast(c); - //return a future that will wait for the object - result = new ItemFuture<>(c); - } else { - //return a future with the item - result = new ItemFuture<>(item); - } - } finally { - lock.unlock(); - } - return result; - } - - @Override - public boolean remove(Object e) { - for (int idx=0; idx<LOCK_COUNT; idx++) { - final ReentrantLock lock = this.locks[idx]; - lock.lock(); - try { - boolean result = items[idx].remove(e); - if (result) { - return result; - } - } finally { - lock.unlock(); - } - } - return false; - } - - @Override - public int size() { - int size = 0; - for (int idx=0; idx<LOCK_COUNT; idx++) { - size += items[idx].size(); - } - return size; - } - - @Override - public Iterator<E> iterator() { - return new FairIterator(); - } - - @Override - public E poll() { - int idx = getNextPoll(); - final ReentrantLock lock = this.locks[idx]; - lock.lock(); - try { - return items[idx].poll(); - } finally { - lock.unlock(); - } - } - - @Override - public boolean contains(Object e) { - for (int idx=0; idx<LOCK_COUNT; idx++) { - boolean result = items[idx].contains(e); - if (result) { - return result; - } - } - return false; - } - - - //------------------------------------------------------------------ - // NOT USED BY CONPOOL IMPLEMENTATION - //------------------------------------------------------------------ - @Override - public boolean add(E e) { - return offer(e); - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public int drainTo(Collection<? super E> c, int maxElements) { - throw new UnsupportedOperationException("int drainTo(Collection<? super E> c, int maxElements)"); - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public int drainTo(Collection<? super E> c) { - return drainTo(c,Integer.MAX_VALUE); - } - - @Override - public void put(E e) throws InterruptedException { - offer(e); - } - - @Override - public int remainingCapacity() { - return Integer.MAX_VALUE - size(); - } - - @Override - public E take() throws InterruptedException { - return this.poll(Long.MAX_VALUE, TimeUnit.MILLISECONDS); - } - - @Override - public boolean addAll(Collection<? extends E> c) { - Iterator<? extends E> i = c.iterator(); - while (i.hasNext()) { - E e = i.next(); - offer(e); - } - return true; - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public void clear() { - throw new UnsupportedOperationException("void clear()"); - - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public boolean containsAll(Collection<?> c) { - throw new UnsupportedOperationException("boolean containsAll(Collection<?> c)"); - } - - @Override - public boolean isEmpty() { - return size() == 0; - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public boolean removeAll(Collection<?> c) { - throw new UnsupportedOperationException("boolean removeAll(Collection<?> c)"); - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public boolean retainAll(Collection<?> c) { - throw new UnsupportedOperationException("boolean retainAll(Collection<?> c)"); - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public Object[] toArray() { - throw new UnsupportedOperationException("Object[] toArray()"); - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public <T> T[] toArray(T[] a) { - throw new UnsupportedOperationException("<T> T[] toArray(T[] a)"); - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public E element() { - throw new UnsupportedOperationException("E element()"); - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public E peek() { - throw new UnsupportedOperationException("E peek()"); - } - - /** - * {@inheritDoc} - * @throws UnsupportedOperationException - this operation is not supported - */ - @Override - public E remove() { - throw new UnsupportedOperationException("E remove()"); - } - - - - //------------------------------------------------------------------ - // Non cancellable Future used to check and see if a connection has been made available - //------------------------------------------------------------------ - /** - * Future for an item in the queue. - * @param <T> the item type - */ - protected class ItemFuture<T> implements Future<T> { - /** The item. */ - protected volatile T item = null; - /** The latch. */ - protected volatile ExchangeCountDownLatch<T> latch = null; - /** Whether cancelled. */ - protected volatile boolean canceled = false; - - /** - * Constructor. - * @param item the item - */ - public ItemFuture(T item) { - this.item = item; - } - - /** - * Constructor. - * @param latch the latch - */ - public ItemFuture(ExchangeCountDownLatch<T> latch) { - this.latch = latch; - } - - @Override - public boolean cancel(boolean mayInterruptIfRunning) { - return false; //don't allow cancel for now - } - - @Override - public T get() throws InterruptedException, ExecutionException { - if (item!=null) { - return item; - } else if (latch!=null) { - latch.await(); - return latch.getItem(); - } else { - throw new ExecutionException("ItemFuture incorrectly instantiated. Bug in the code?", new Exception()); - } - } - - @Override - public T get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { - if (item!=null) { - return item; - } else if (latch!=null) { - boolean timedout = !latch.await(timeout, unit); - if (timedout) { - throw new TimeoutException(); - } else { - return latch.getItem(); - } - } else { - throw new ExecutionException("ItemFuture incorrectly instantiated. Bug in the code?", new Exception()); - } - } - - @Override - public boolean isCancelled() { - return false; - } - - @Override - public boolean isDone() { - return (item!=null || latch.getItem()!=null); - } - - } - - //------------------------------------------------------------------ - // Count down latch that can be used to exchange information - //------------------------------------------------------------------ - /** - * Count down latch that can exchange information. - * @param <T> the item type - */ - protected class ExchangeCountDownLatch<T> extends CountDownLatch { - /** The item. */ - protected volatile T item; - /** - * Constructor. - * @param i the count - */ - public ExchangeCountDownLatch(int i) { - super(i); - } - /** - * Get the item. - * @return the item - */ - public T getItem() { - return item; - } - /** - * Set the item. - * @param item the item - */ - public void setItem(T item) { - this.item = item; - } - } - - //------------------------------------------------------------------ - // Iterator safe from concurrent modification exceptions - //------------------------------------------------------------------ - /** - * Iterator safe from concurrent modification exceptions. - */ - protected class FairIterator implements Iterator<E> { - /** The elements. */ - E[] elements = null; - /** The current index. */ - int index; - /** The current element. */ - E element = null; - - /** - * Constructor. - */ - @SuppressWarnings("unchecked") // Can't create arrays of generic types - public FairIterator() { - ArrayList<E> list = new ArrayList<>(MultiLockFairBlockingQueue.this.size()); - for (int idx=0; idx<LOCK_COUNT; idx++) { - final ReentrantLock lock = MultiLockFairBlockingQueue.this.locks[idx]; - lock.lock(); - try { - elements = (E[]) new Object[MultiLockFairBlockingQueue.this.items[idx].size()]; - MultiLockFairBlockingQueue.this.items[idx].toArray(elements); - - } finally { - lock.unlock(); - } - } - index = 0; - elements = (E[]) new Object[list.size()]; - list.toArray(elements); - } - @Override - public boolean hasNext() { - return index<elements.length; - } - - @Override - public E next() { - if (!hasNext()) { - throw new NoSuchElementException(); - } - element = elements[index++]; - return element; - } - - @Override - public void remove() { - for (int idx=0; idx<LOCK_COUNT; idx++) { - final ReentrantLock lock = MultiLockFairBlockingQueue.this.locks[idx]; - lock.lock(); - try { - boolean result = MultiLockFairBlockingQueue.this.items[idx].remove(elements[index]); - if (result) { - break; - } - } finally { - lock.unlock(); - } - } - - } - - } -} diff --git a/res/spotbugs/filter-false-positives.xml b/res/spotbugs/filter-false-positives.xml index 86febd7a02..b2d4c6c7ce 100644 --- a/res/spotbugs/filter-false-positives.xml +++ b/res/spotbugs/filter-false-positives.xml @@ -1417,12 +1417,6 @@ <Method name="compare" /> <Bug code="ES" /> </Match> - <Match> - <!-- Lock is released --> - <Class name="org.apache.tomcat.jdbc.pool.MultiLockFairBlockingQueue" /> - <Method name="poll" /> - <Bug code="UL" /> - </Match> <Match> <!-- SQL is from config so is considered safe --> <Class name="org.apache.tomcat.jdbc.pool.PooledConnection" /> diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 2cb0ad9675..204d695658 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -799,6 +799,21 @@ when the connection is returned regardless of the garbage collector. (remm) </fix> + <fix> + Remove the unfinished, experimental + <code>MultiLockFairBlockingQueue</code> class together with the + unreachable code that referenced it. The class was never + selectable - its instantiation in the pool was commented out and + no configuration property existed to enable it - while its + striped design kept items and waiting threads on unrelated + stripes, so enabling it as it stood could hang borrowers while + idle connections sat in another stripe, report the pool as + exhausted although it was not, lose a connection permanently + whenever a waiting thread was interrupted, and silently disable + the idle connection sweep because the queue's iterator was + always empty. Fair queueing remains available through + <code>FairBlockingQueue</code>. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
