Understanding and resolving ConcurrentModificationException

Question

I'm dealing with an intermittent, hard to reproduce ConcurrentModificationException in a chunk of legacy code:

class LegacyCode {
    private final WeakHashMap<A, B> mItems = new WeakHashMap<A, B>();

    public void someWork() {
        final List<A> copy = new LinkedList<A>();

        for (final A item : mItems.keySet()) {
            copy.add(item);
        }

        for (final A item : copy) {
            item.someMethod();
        }
    }

    public void addItem(final A item) {
        mItems.put(item, new B());
    }

    public void removeItem(final A item) {
        mItems.remove(item);
    }
}

The CME is being thrown in:

for (final A item : mItems.keySet()) {
    copy.add(item);
}

I'm not entirely sure as to why we create copy in this manner. The CME is thrown because either addItem(A) or removeItem(A) is called while the for-each loop is running.

Questions

Is my understanding of why CME is being thrown correct?
Will I avoid CME if I replace the for-each loop with:

final List<A> copy = new LinkedList<A>(mItems.keySet());
Will this change be equivalent to the for-each loop we will be replacing? As far as I can tell, both snippets create a shallow copy of mItems.keySet() in copy.

Community · Accepted Answer · 2020-06-20 09:12:55Z

2

Is my understanding of why CME is being thrown correct?

Absolutely. This is exactly what is happening.

Will I avoid CME if I replace the for-each loop with:
final List<A> copy = new LinkedList<A>(mItems.keySet());

No, you would not, because constructor of LinkedList<A> would have a similar loop. So you are correct in saying that this change will be equivalent to the for-each loop we will be replacing.

As far as fixing this problem is concerned, there is no off-the-shelf concurrent replacement for WeakHashMap class in Java standard library. You can address this the obvious way by making addItem and removeItem synchronized, and adding a synchronized block around the loop constructing a copy. You can also look into third-party collections that solve this problem without using synchronized in your code.

edited Jun 20, 2020 at 9:12

CommunityBot

11 silver badge

answered Jun 18, 2017 at 16:26

Sergey Kalinichenko

729k85 gold badges1.2k silver badges1.6k bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

user3264740 Over a year ago

Thanks for clearing this up. I understand why placing the the for-each loop inside a synchronized block is required. But, what is the reason behind making addItem and removeItem synchronized?

Sergey Kalinichenko Over a year ago

@user3264740 You need both reading and writing sides synchronized. Otherwise, foreach loop would freely acquire the lock, even though addItem or removeItem may be in progress.

Collectives™ on Stack Overflow

Understanding and resolving ConcurrentModificationException

1 Answer 1

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related