Avoid potential ConcurrentModificationException by using Iterator.

[aooohan]

[codecov-commenter]

Codecov Report

Merging #182 (c89be0d) into master (c2f0a2b) will increase coverage by 0.03%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #182      +/-   ##
============================================
+ Coverage     82.06%   82.09%   +0.03%     
  Complexity      763      763              
============================================
  Files            42       42              
  Lines          3066     3072       +6     
  Branches        308      309       +1     
============================================
+ Hits           2516     2522       +6     
  Misses          445      445              
  Partials        105      105              

Impacted Files	Coverage Δ
...a/org/apache/commons/pool2/impl/EvictionTimer.java	85.71% <100.00%> (+1.33%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[garydgregory]

This PR needs a unit test that fails without its main changes. Otherwise, it's just a regression waiting to happen.

[garydgregory]

Hi @aooohan You'll need a unit test to prove this is a problem because the code appears to provide all the locking needed through synchronized blocks and methods.

[aooohan]

Hi @garydgregory

Sorry for the late reply.

The following is unit test.

        try (final GenericObjectPool<String, RuntimeException> pool = new GenericObjectPool<>(new BasePooledObjectFactory<String, RuntimeException>() {
            @Override
            public String create() {
                return null;
            }
            @Override
            public PooledObject<String> wrap(final String obj) {
                return new DefaultPooledObject<>(obj);
            }
        })) {
            for (int i = 0; i < 1000; i++) {
                final BaseGenericObjectPool<String, RuntimeException>.Evictor evictor2 = pool.new Evictor();
                EvictionTimer.schedule(evictor2, Duration.ofSeconds(10), Duration.ofSeconds(30));
            }
            System.gc();
            System.out.println("gc结束");
            TimeUnit.SECONDS.sleep(100);
        }

There are two steps：

1. If you run it straight away, nothing appears to be happening.
2. if you modify Reaper a bit, you'll see that it actually throws a ConcurrentModificationException, but I haven't figured out why the exception isn't thrown in the first step.

private static class Reaper implements Runnable {
        @Override
        public void run() {
            synchronized (EvictionTimer.class) {
                try {
                    for (final Entry<WeakReference<BaseGenericObjectPool<?, ?>.Evictor>, WeakRunner<BaseGenericObjectPool<?, ?>.Evictor>> entry : TASK_MAP
                            .entrySet()) {
                        if (entry.getKey().get() == null) {
                            executor.remove(entry.getValue());
                            TASK_MAP.remove(entry.getKey());
                        }
                    }
                } catch (Exception e) {
                    e.printStackTrace();
                    System.out.println(e);
                }
                if (TASK_MAP.isEmpty() && executor != null) {
                    executor.shutdown();
                    executor.setCorePoolSize(0);
                    executor = null;
                }
            }
        }
    }

[garydgregory]

Hi @aooohan Thank you for the update. We can wait until you update this PR with a failing test.

[aooohan]

We can wait until you update this PR with a failing test.

It is somewhat difficult to verify this through unit tests, as there is nothing to assert and I think it is just a usage error and unit tests are not very necessary.

[garydgregory]

Then this PR won't make it because:

• You cannot prove anything is wrong
• You have not addressed my comment on why the locking already provided through synchronized blocks and synchronized methods is not enough
• No unit tests mean the code can be reverted at any time and the build will pass therefore creating a regression (if something was indeed wrong)

[aooohan]

Then this PR won't make it because:

• You cannot prove anything is wrong
• You have not addressed my comment on why the locking already provided through synchronized blocks and synchronized methods is not enough
• No unit tests mean the code can be reverted at any time and the build will pass therefore creating a regression (if something was indeed wrong)

... Well, I'll close this.

[aooohan]

You have not addressed my comment on why the locking already provided through synchronized blocks and synchronized methods is not enough

For this, it's not a synchronization problem, but an incorrect delete operation for Map, and although the exception thrown is ConcurrentModificationException, it has nothing to do with concurrency