HashMap.remove() synchronized by using String.intern() on the Key, does this even work? Or is this broken code?

Question

I recently came across the following construct

Map<String,Value> map = new HashMap<>();
...
Value getValue(String key) {
    synchronized (key.intern()) {
        return map.remove(key);
    }
}

Given that intern() is usually not that fast, I doubt this would outperform using synchronized, Collections.synchronizedMap(Map) or ConcurrentHashMap. But even if this construct would be faster then all other methods in this particular case: Is this properly synchronized? I doubt that this is thread safe, as the remove could happen while the hash table is reorganized. But even if this would work, I suspect that code to be broken given that HashMap javadoc states:

If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally.

@Dan Getz - you're right of course! I'll remove my comments. Whilst interned strings are thread safe, synchronizing on *different keys* is the equivalent of synchronizing on *different interned Strings* and is therefore completely unsafe! — Matt Coubrough, Dec 09 '14 at 23:20
Note the code snippet you've posted has no concurrency concerns, since `map` isn't being accessed by multiple threads. Presumably your actual application *is* using `map` concurrently, but I just wanted to call that out. — dimo414, Dec 09 '14 at 23:40

score 7 · Accepted Answer · edited May 23 '17 at 12:21

This is not sufficient to safely access a HashMap from multiple threads. In fact, it's all but guaranteed to break something. By synchronizing on a given key the map can still be unsafely modified concurrently, as long as separate threads are working with different keys.

Consider if these three threads were trying to run at the same time:

Thread 1                Thread 2                 Thread 3
synchronized("a") {     synchronized("a") {      synchronized("b") {
  map.remove("a");        map.remove("a");         map.remove("b");
}                       }                        }

Threads 1 and 2 would correctly wait for each other since they're synchronizing on the same object (Java interns string constants). But Thread 3 is unimpeded by the work going on in the other threads, and immediately enters its synchronized block since no one else is locking "b". Now two different synchronized blocks are interacting with map simultaneously, and all bets are off. Before long, your HashMap will be corrupt.

Collections.synchronizedMap() correctly uses the map itself as the synchronizing object and therefore locks the whole map, not just the keys being used. This is the only robust way to prevent internal corruption of a HashMap being accessed from multiple threads.

ConcurrentHashMap correctly does what I think the code you posted is trying to do by locking internally on subsets of all the keys in the map. That way, multiple threads can safely access different keys on different threads and never block each other - but if the keys happen to be in the same bucket, the map will still block. You can modify this behavior with the concurrencyLevel constructor argument.

See also: Java synchronized block vs. Collections.synchronizedMap

As an aside, lets suppose for the sake of argument that synchronized(key.intern()) was a reasonable way to concurrently access a HashMap. This would still be incredibly error prone. If just a single place in the application failed to call .intern() on a key, everything could come crashing down.

I'd say it's not guaranteed that thread1 and thread2 are synchronized on the same object (I don't know if JLS says that those strings are always used from the pool and therefore the same instances). But otherwise yes, I also think that such a pattern is broken. — Flow, Dec 10 '14 at 08:58
I didn't include the `.intern()` calls simply for brevity, but modern versions of Java do indeed intern all string constants. Whether you ensure your keys are interned or not doesn't really matter however, synchronizing keys is a broken pattern regardless. — dimo414, Dec 10 '14 at 13:52

HashMap.remove() synchronized by using String.intern() on the Key, does this even work? Or is this broken code?

1 Answers1