Guava MultiMap and ConcurrentModificationException

Question

I don't understand why I get a ConcurrentModificationException when I iterate through this multimap. I read the following entry, but I am not sure if I understood the whole thing. I tried to add a synchronized block. But my doubt is what to synchronize with, and when.

The multimap is a field and created like this :

private Multimap<GenericEvent, Command> eventMultiMap =   
   Multimaps.synchronizedMultimap(HashMultimap.<GenericEvent, Command> create());

and used like this :

eventMultiMap.put(event, command);

and like this ( I tried to synchronize this part on the map, but without success )

for (Entry<GenericEvent, Command> entry : eventMultiMap.entries()) {
    if (entry.getValue().equals(command)) {
        eventMultiMap.remove(entry.getKey(), entry.getValue());
        nbRemoved++;
    }
}

See also http://stackoverflow.com/questions/1675037/removing-items-from-a-collection-in-java-while-iterating-over-it — finnw, Nov 06 '09 at 17:21

score 11 · Answer 1 · edited Jun 01 '13 at 05:31

11

Calling remove on a collection while you're iterating through it will cause a ConcurrentModificationException every time, even if it's all done in the same thread - the Right Thing to do is get an explicit iterator and call .remove() on that.

Edit: Modifying your example:

Iterator<Map.Entry<GenericEvent, Command>> i = eventMultiMap.entries().iterator();
while (i.hasNext()) {
    if (i.next().getValue().equals(command)) {
        i.remove();
        nbRemoved++;
    }
}

edited Jun 01 '13 at 05:31

Vladimir Matveev

120,085
34
287
296

answered Oct 15 '09 at 12:56

MHarris

1,801
3
21
34

Ok, i get the point. But Iterator can only takes one parameter type. No ? If i iterate trought the values of my map, and remove one them. Will the corresponding value will deleted in the original map ( eventMultiMap ) ? – Antoine Claval Oct 15 '09 at 13:10
Ok, the iterator on the values of the map can remove the values. Thanks. I cannot accepts your response for now, Iterator is not compiling, but edit it and i will accept. – Antoine Claval Oct 15 '09 at 13:30
Sorry. Don't have google-collections at work, so I couldn't test the code before posting it. If they're designed like the Java HashMap, it should be possible to use an Iterator over the Entry objects - I've edited to show this and will doublecheck it when I get home, if you've not had a chance to try it in the meantime. – MHarris Oct 15 '09 at 15:24

score 5 · Answer 2 · answered Jan 23 '10 at 00:50

You may wish to see this blogpost for another pitfall yielding a ConcurrentModificationException when traversing a multimap, with no other thread interfering. In short, if you traverse multimap's keys, accessing the respective collection of values associated with each key and remove some element from such a collection, if that element happens to be the last of the collection you are going to have ConcurrentModificationException when you try to access the next key - because emptying a collection triggers the removal of the key, thus structurally modifying the multimap's keyset.

...so the fix for this would be to check the size of the value-collection while you're iterating it, and if ever that value-collection only has one entry left when you're going to remove from it, just remove from the keyset's iterator instead. — Alkanshel, Jun 07 '19 at 22:02

score 4 · Answer 3 · answered Nov 04 '09 at 03:37

If another thread could modify your multimap while this logic is running, you'll need to add a synchronized block to MHarris's code:

synchronized (eventMultimap) {
  Iterator<Entry<GenericEvent, Command>> i = eventMultiMap.entries.iterator();
  while (i.hasNext()) {
    if (i.next().getValue().equals(command)) {
        i.remove();
        nbRemoved++;
    }
  }
}

Or, you could omit the iterator as follows,

synchronized (eventMultimap) {
  int oldSize = eventMultimap.size();
  eventMultimap.values().removeAll(Collections.singleton(command));
  nbRemoved = oldSize - eventMultimap.size();
}

The removeAll() call doesn't require synchronization. However, if you omit the synchronized block, the multimap could mutate between the removeAll() call and one of the size() calls, leading to an incorrect value of nbRemoved.

Now, if your code is single-threaded, and you just want to avoid a ConcurrentModificationException call, you can leave out the Multimaps.synchronizedMultimap and synchronized (eventMultimap) logic.

I like the removeAll(Collection.singleton(stuffToRemove)). Thanks. — Antoine Claval, Nov 04 '09 at 09:12

Mr. Polywhirl · Answer 4 · 2013-12-14T01:23:39.737

I prefer Multimap.values().iterator() if you don't care about the key. You should also try to stay away from using synchronized blocks as much as possible, because you cannot prioritize reads/writes effectively.

ReadWriteLock lock = new ReentrantReadWriteLock();
Lock writeLock = lock.writeLock(); 

public void removeCommands(Command value) {
  try {
    writeLock.lock();
    for (Iterator<Command> it = multiMap.values().iterator(); it.hasNext();) {
      if (it.next() == value) {
        it.remove();
      }
    }
  } finally {
    writeLock.unlock();
  }
}

score 1 · Accepted Answer · answered May 11 '17 at 18:31

1

In java8 you can also use a lambda approach:

eventMultiMap.entries().removeIf(genericEventCommandEntry -> genericEventCommandEntry.getValue().equals(command));

answered May 11 '17 at 18:31

Evgeny Kiselev

216
1
8

Guava MultiMap and ConcurrentModificationException

5 Answers5

Linked