0

I have a WeakHashMap of instances implementing an interface (OnPreferenceChangeListener), and I'm trying to call a method (onPreferenceChanged) for each instance in the map.

Despite the fact that I've synchronized on the map itself and I don't believe any of the onPreferenceChanged implementations should be actually changing the map in any way, I'm getting a ConcurrentModificationException in the forEach. The code is:

    private val listeners = Collections.synchronizedMap(WeakHashMap<OnPreferenceChangeListener, Any>())
    fun notifyChanged(property: KProperty<*>) {
        val properties = listOf(property.name)
        synchronized(listeners) {
            listeners.forEach { it.key.onPreferenceChanged(properties) }
        }
    }

And the stacktrace is:

java.util.ConcurrentModificationException
at java.util.WeakHashMap$HashIterator.nextEntry(WeakHashMap.java:806)
at java.util.WeakHashMap$EntryIterator.next(WeakHashMap.java:845)
at java.util.WeakHashMap$EntryIterator.next(WeakHashMap.java:843)
at my.package.notifyChanged(Preferences.kt:537)
...

If I follow the advice and code snippet in Collections.java on the javadoc for public static <K,V> Map<K,V> synchronizedMap(Map<K,V> m) directly, and use this code:

    fun notifyChanged(property: KProperty<*>) {
        val properties = listOf(property.name)
        val keySet = listeners.keys
        synchronized(listeners) {
            val iterator = keySet.iterator()
            while (iterator.hasNext()) {
                iterator.next()
                    .onPreferenceChanged(properties)
            }
        }
    }

The same thing happens.

If I've synchronized on the map itself, what else could be modifying it?

growse
  • 3,554
  • 9
  • 43
  • 66
  • 2
    Is it possible some keys are getting removed via garbage collection (since it's a `WeakHashMap`) while you're using the iterator? Like the docs say, *if the map is structurally modified at any time after the iterator is created, **in any way** except through the iterator's own `remove` method, the iterator will throw a `ConcurrentModificationException`* – cactustictacs Nov 19 '22 at 19:55
  • Erk! Yes, great spot. How could I go about proving that? Is there a convenient way to hint "Don't do any GC in this block?" – growse Nov 19 '22 at 20:01
  • I'm not sure honestly - I know you can request GC with `System.gc()` but even that's more of a hint than actual control, I don't know if you can actually *block* it. But I'm not too sure about this GC idea to be honest - even though the docs don't make any guarantees, from looking around a lot of people are claiming the GC doesn't actually remove the weak references (the references to the keys themselves), just marks them as stale. And it wouldn't make much sense to have an iterator like this that's so inherently dangerous. I've used it before (with multiple threads) and never had issues – cactustictacs Nov 19 '22 at 20:24
  • Are you sure none of your listeners explicitly remove themselves from that map when you call `onPreferenceChanged`? Might be worth adding some logging so you can see exactly where it happens, and if it's consistent – cactustictacs Nov 19 '22 at 20:25
  • Are you sure you don't use `listeners` anywhere else in your code? Is there any reason to keep it public and not private? – broot Nov 19 '22 at 20:53
  • @cactustictacs just read https://docs.oracle.com/javase/8/docs/api/java/util/WeakHashMap.html - " The behavior of the WeakHashMap class depends in part upon the actions of the garbage collector, so several familiar (though not required) Map invariants do not hold for this class. Because the garbage collector may discard keys at any time, a WeakHashMap may behave as though an unknown thread is silently removing entries.". Honestly feels like this pretty severely limits the utility of WeakHashMap though, so I wonder if there's a better way to solve the problem? – growse Nov 19 '22 at 21:47
  • @broot should have been more clear - listeners is private to the class. I made an error abbreviating it. (have edited) – growse Nov 19 '22 at 22:24
  • @growse They mentioned several examples of misbehavior and none of them were throwing CME. I believe (but I'm not sure), it shouldn't throw when iterating during GC. If we look into the implementation, we can see it uses `ReferenceQueue` to track collected objects. I guess it may throw CME if you e.g. access the map while iterating (even reading). – broot Nov 19 '22 at 22:25
  • Why wouldn't GC removing a map entry during iteration not throw a CME? Seems like exactly the case a CME would be thrown, no? – growse Nov 19 '22 at 22:26
  • So I imagine your second example may be wrong, because you access `listeners.keys` outside of synchronized block. But frankly, I'm not sure about this and even if, the first code samples shouldn't have this problem. – broot Nov 19 '22 at 22:27
  • @broot that was taken from https://github.com/openjdk/jdk17/blob/master/src/java.base/share/classes/java/util/Collections.java#L2614 which seems to specifically say "Needn't be in synchronized block" – growse Nov 19 '22 at 22:32
  • I doubt GC can really modify a `WeakHashMap`. It would require it to understand its data structure and logic. Look here: https://github.com/openjdk/jdk17/blob/4afbcaf55383ec2f5da53282a1547bac3d099e9d/src/java.base/share/classes/java/util/WeakHashMap.java#L322 I think this is the place where `WeakHashMap` cleans up itself from collected keys. I think the meaning of this comment in documentation is that `synchronizedMap()` or synchronized blocks are only partially useful, because the map can still be modified by GC, but that doesn't mean directly. – broot Nov 19 '22 at 22:34
  • 1
    I doubt that this is an issue of garbage collection at all. It’s the same old thing, people thinking “ConcurrentModificationException” has anything to do with multi-threading. It’s very likely that one of the listeners adds or removes a listener, in other words, modifies the map within the method you’re calling while you’re iterating over the map. Synchronization doesn’t help here, because it’s the same thread. But in general, [as explained her](https://stackoverflow.com/a/74386184/2711488), weak listeners are a really bad idea anyway. – Holger Nov 21 '22 at 12:57
  • I'll do some tracing to find out if there's any listener that's modifying the map. It's entirely possible that I missed one. – growse Nov 22 '22 at 13:03

0 Answers0