Fix potential ConcurrentModificationException in Reaper clean-up thread.

Author: markt-asf

src/changes/changes.xml

@@ -61,6 +61,7 @@ The <action> type attribute can be add,update,fix,remove.
     <action type="fix" dev="ggregory" due-to="Wei Guo, Gary Gregory">Fix site link from the About page to the Download page, see also #387.</action>
     <action type="fix" dev="ggregory" due-to="Gary Gregory">Operation on the "idleHighWaterMark" shared variable in "ErodingFactor" class is not atomic [org.apache.commons.pool2.PoolUtils$ErodingFactor] At PoolUtils.java:[line 98] AT_NONATOMIC_OPERATIONS_ON_SHARED_VARIABLE.</action>
     <action type="fix" dev="ggregory" due-to="Gary Gregory">org.apache.commons.pool2.impl.GenericObjectPool.create(Duration) should normalize a negative duration to zero.</action>
+    <action type="fix" dev="markt"    due-to="Coverity Scan">Fix potential ConcurrentModificationException in EvictionTimer thread clean-up.</action>
     <!-- ADD -->
     <action type="add" dev="ggregory" due-to="Gary Gregory">Add org.apache.commons.pool2.PooledObject.nonNull(PooledObject).</action>
     <action type="add" dev="ggregory" due-to="Gary Gregory">Add org.apache.commons.pool2.PooledObject.getObject(PooledObject).</action>

src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java

@@ -21,6 +21,7 @@
 import java.security.PrivilegedAction;
 import java.time.Duration;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.Map.Entry;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
@@ -73,11 +74,17 @@ private static final class Reaper implements Runnable {
         @Override
         public void run() {
             synchronized (EvictionTimer.class) {
-                for (final Entry<WeakReference<BaseGenericObjectPool<?>.Evictor>, WeakRunner<BaseGenericObjectPool<?>.Evictor>> entry : TASK_MAP
-                        .entrySet()) {
+                /*
+                 * Need to use iterator over TASK_MAP so entries can be removed when iterating without triggering a
+                 * ConcurrentModificationException.
+                 */
+                final Iterator<Entry<WeakReference<BaseGenericObjectPool<?>.Evictor>, WeakRunner<BaseGenericObjectPool<?>.Evictor>>> iterator =
+                        TASK_MAP.entrySet().iterator();
+                while (iterator.hasNext()) {
+                    final Entry<WeakReference<BaseGenericObjectPool<?>.Evictor>, WeakRunner<BaseGenericObjectPool<?>.Evictor>> entry = iterator.next();
                     if (entry.getKey().get() == null) {
                         executor.remove(entry.getValue());
-                        TASK_MAP.remove(entry.getKey());
+                        iterator.remove();
                     }
                 }
                 if (TASK_MAP.isEmpty() && executor != null) {

src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java

@@ -936,13 +936,25 @@ public void tearDown() throws Exception {
     void testAbandonedPool() throws Exception {
         final GenericObjectPoolConfig<String> config = new GenericObjectPoolConfig<>();
         config.setJmxEnabled(false);
-        GenericObjectPool<String> abandoned = new GenericObjectPool<>(simpleFactory, config);
-        abandoned.setDurationBetweenEvictionRuns(Duration.ofMillis(100)); // Starts evictor
-        assertEquals(abandoned.getRemoveAbandonedTimeout(), abandoned.getRemoveAbandonedTimeoutDuration().getSeconds());
-        // This is ugly, but forces GC to hit the pool
-        final WeakReference<GenericObjectPool<String>> ref = new WeakReference<>(abandoned);
-        abandoned = null;
-        while (ref.get() != null) {
+
+        // Need to create at least 2 pools to test EvictorTimer Reaper for ConcurrentModificationException
+        // Note: If the test fails, the ConcurrentModificationException isn't visible without modifications to the
+        //       Reaper class.
+        GenericObjectPool<String> abandoned1 = new GenericObjectPool<>(simpleFactory, config);
+        abandoned1.setDurationBetweenEvictionRuns(Duration.ofMillis(100)); // Starts evictor
+        GenericObjectPool<String> abandoned2 = new GenericObjectPool<>(simpleFactory, config);
+        abandoned2.setDurationBetweenEvictionRuns(Duration.ofMillis(100)); // Starts evictor
+
+        // This is ugly, but forces GC to hit the pools
+        final WeakReference<GenericObjectPool<String>> ref1 = new WeakReference<>(abandoned1);
+        abandoned1 = null;
+        while (ref1.get() != null) {
+            System.gc();
+            Thread.sleep(100);
+        }
+        final WeakReference<GenericObjectPool<String>> ref2 = new WeakReference<>(abandoned2);
+        abandoned2 = null;
+        while (ref2.get() != null) {
             System.gc();
             Thread.sleep(100);
         }