Fix potential ConcurrentModificationException in Reaper clean-up thread.

Author: markt-asf

src/changes/changes.xml

@@ -55,6 +55,7 @@ The <action> type attribute can be add,update,fix,remove.
     <action type="fix" dev="ggregory" due-to="Gary Gregory">Remove -nouses directive from maven-bundle-plugin. OSGi package imports now state 'uses' definitions for package imports, this doesn't affect JPMS (from org.apache.commons:commons-parent:80).</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.pool3.PoolUtils$ErodingFactor] At PoolUtils.java:[line 101] AT_NONATOMIC_OPERATIONS_ON_SHARED_VARIABLE.</action>
     <action type="fix" dev="ggregory" due-to="shengulong, Gary Gregory" issue="POOL-422">org.apache.commons.pool3.impl.GenericObjectPool.create(Duration) duration computation doesn't match 2.x. See also PR #409 from shengulong.</action>
+    <action type="fix" dev="markt"    due-to="Coverity Scan">Fix potential ConcurrentModificationException in EvictionTimer thread clean-up.</action>
     <!-- REMOVE -->
     <action dev="ggregory" type="remove">Deprecations from version 2.x have been removed.</action>
     <!-- UPDATE -->

src/main/java/org/apache/commons/pool3/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,6 +74,20 @@ private static final class Reaper implements Runnable {
         @Override
         public void run() {
             synchronized (EvictionTimer.class) {
+                /*
+                 * 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());
+                        iterator.remove();
+                    }
+                }
+
                 for (final Entry<WeakReference<BaseGenericObjectPool<?, ?>.Evictor>, WeakRunner<BaseGenericObjectPool<?, ?>.Evictor>> entry : TASK_MAP
                         .entrySet()) {
                     if (entry.getKey().get() == null) {

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

@@ -973,13 +973,25 @@ public void tearDown() throws Exception {
     void testAbandonedPool() throws TestException, InterruptedException {
         final GenericObjectPoolConfig<String> config = new GenericObjectPoolConfig<>();
         config.setJmxEnabled(false);
-        GenericObjectPool<String, TestException> abandoned = new GenericObjectPool<>(simpleFactory, config);
-        abandoned.setDurationBetweenEvictionRuns(Duration.ofMillis(100)); // Starts evictor
 
-        // This is ugly, but forces GC to hit the pool
-        final WeakReference<GenericObjectPool<String, TestException>> 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, TestException> abandoned1 = new GenericObjectPool<>(simpleFactory, config);
+        abandoned1.setDurationBetweenEvictionRuns(Duration.ofMillis(100)); // Starts evictor
+        GenericObjectPool<String, TestException> 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, TestException>> ref1 = new WeakReference<>(abandoned1);
+        abandoned1 = null;
+        while (ref1.get() != null) {
+            System.gc();
+            Thread.sleep(100);
+        }
+        final WeakReference<GenericObjectPool<String, TestException>> ref2 = new WeakReference<>(abandoned2);
+        abandoned2 = null;
+        while (ref2.get() != null) {
             System.gc();
             Thread.sleep(100);
         }