Document that Multimap iterator remove leads to ConcurrentModificationException

[bennofs]

Simple example:

class Scratch {
    public static void main(String[] args) {
        final Multimap<String, String> map = HashMultimap.create();
        map.put("test", "x");
        map.put("xx", "z");
        for (Iterator<Map.Entry<String, Collection<String>>> it = map.asMap().entrySet().iterator(); it.hasNext(); ) {
            Map.Entry<String, Collection<String>> entry = it.next();
            Iterator<String> it2 = entry.getValue().iterator();
            it2.next();
            it2.remove();
        }
        System.out.println(map);
    }
}

I think this happens because it2.remove() will detect that the collection is empty, which triggers a .remove call on the underlying map that is not done through the iterator. The correct thing here would be to call .remove on the underlying map iterator as well.

[cpovirk]

This is definitely unfortunate. But I think it's unfixable in the general case, at least without significant tradeoffs.

I haven't tested this, but I'd expect problems with a case like the following:

map.put("a", "a");
map.put("b", "b");
map.put("c", "c");
Iterator<Map.Entry<String, Collection<String>>> it = map.asMap().entrySet().iterator();
Map.Entry<String, Collection<String>> first = it.next();
Map.Entry<String, Collection<String>> second = it.next();
Iterator<String> it2 = first.next().getValue().iterator();
it2.remove();
it.next();

By the time we call it2.remove(), we've already called it.next() again, so we can't use it.remove() to perform the removal. So we end up with the same problem on the next it.next() call. (We could have special handling for the more common case that you've described, but that's probably more complexity than we'd prefer, both in the implementation and in the API.)

There might be more clever solutions we could employ, like making the underlying map key not the String directly but rather a Holder<String>, which we could then null out on a call to remove() instead of fully removing the key. (This would use some extra storage temporarily, but hopefully it would get cleared out eventually when users perform some other structural modification, which we can detect through the view collections. And there's also the ongoing overhead (including indirection) from the Holder itself, though maybe that can be avoided with a custom hash table -- which we can do (and already do in places), but it's extra code (bad for Android) and more complexity.)

For the specific case here, iterating over entries() and removing should work. Of course that might be less convenient in some cases :( Another possibility is to not make change immediately but rather compile a list of entries to remove, which you can then apply after the fact. (Or, similarly, you might build a new multimap from scratch to replace the old.)

Again, none of the alternatives are great general solutions. But I don't think special handling here would be a net win.

[cpovirk]

It doesn't look like we document this, though. We should do that.

[apostrophe]

Doesn't the below (from guava documentation) contradict the behavior that removing the last element of an entry's value/collection removes that entry from the Multimap?

Multimap.get(key) always returns a non-null, possibly empty collection.