Guava MultiMap and ConcurrentModificationException [duplicate]

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 stackoverflow.com/questions/1675037/…

finnw
– finnw

2009-11-06 17:21:58 +00:00
Commented Nov 6, 2009 at 17:21 — finnw
– finnw, Commented Nov 6, 2009 at 17:21

Vladimir Matveev · Accepted Answer · 2013-06-01 05:31:38Z

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 1, 2013 at 5:31

Vladimir Matveev

129k35 gold badges311 silver badges308 bronze badges

answered Oct 15, 2009 at 12:56

MHarris

1,8213 gold badges21 silver badges35 bronze badges

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

3 Comments

Antoine Claval Over a year ago

Ok, i get the point. But Iterator<E> 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 Over a year ago

Ok, the iterator on the values of the map can remove the values. Thanks. I cannot accepts your response for now, Iterator<E,T> is not compiling, but edit it and i will accept.

MHarris Over a year ago

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.

Dimitris Andreou · Accepted Answer · 2010-01-23 00:50:00Z

5

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.

answered Jan 23, 2010 at 0:50

Dimitris Andreou

8,9062 gold badges35 silver badges36 bronze badges

1 Comment

Alkanshel Over a year ago

...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.

Jared Levy · Accepted Answer · 2009-11-04 03:37:43Z

4

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.

answered Nov 4, 2009 at 3:37

Jared Levy

2,02614 silver badges12 bronze badges

1 Comment

Antoine Claval Over a year ago

I like the removeAll(Collection.singleton(stuffToRemove)). Thanks.

Evgeny Kiselev · Accepted Answer · 2017-05-11 18:31:39Z

1

In java8 you can also use a lambda approach:

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

answered May 11, 2017 at 18:31

Evgeny Kiselev

2161 silver badge8 bronze badges

Comments

Mr. Polywhirl · Accepted Answer · 2013-12-14 01:23:39Z

1

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();
  }
}

edited Dec 14, 2013 at 1:23

answered Dec 14, 2013 at 1:18

Mr. Polywhirl

49.1k12 gold badges96 silver badges147 bronze badges

Collectives™ on Stack Overflow

Guava MultiMap and ConcurrentModificationException [duplicate]

5 Answers 5

3 Comments

1 Comment

1 Comment

Comments

Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

3 Comments

1 Comment

1 Comment

Comments

Comments

Linked

Related