ConcurrentHashMap remove method Thread Safetiness

Question

I am trying to browse through Spring Source Code (5.2.8.RELEASE), and found the following code.

package org.springframework.core;

public class SimpleAliasRegistry implements AliasRegistry {
    /** Map from alias to canonical name. */
    private final Map<String, String> aliasMap = new ConcurrentHashMap<>(16);

    @Override
    public void removeAlias(String alias) {
        synchronized (this.aliasMap) {     // what if we remove such synchronized line ?
            String name = this.aliasMap.remove(alias);
            if (name == null) {
                throw new IllegalStateException("No alias '" + alias + "' registered");
            }
        }
    }
}

But what I uncertain is the reason why there is a synchronized keyword in removeAlias method, is it really necessary? What if we remove the synchronized keyword? What will happen?

Here is my thought. ConcurrentHashMap should be thread safe when we are calling its provided method, like put, get and remove. And we need to use synchronized to lock the object only when we are performing multiple actions here, like having get first then put or remove. But we want to make the block run without interuption.

Is my thought wrong, or is there any reason why Spring removeAlias method is designed in such way, many thanks.

Update

I also found the time when it is updated, where the developer intentionally do it in this way, when fixing the issue, SimpleAliasRegistry registerAlias not atomic, and here is the reason he wrote out.

I've guarded registerAlias and removeAlias with synchronization, allowing the existing synchronized resolveAliases method and others to reliably see consistent state.

However, I just think synchronized in registerAlias is neccessary but still not convince why one need to do synchronized in removeAlias method, can someone explain to me please?

Source: https://github.com/spring-projects/spring-framework/issues/21119 [Issue]

Source: https://github.com/spring-projects/spring-framework/commit/1b1a69a144f657d46c752f1c017f64d3302891d2 [Changes for that issue]

There may be other operations in that code that rely on locking the entire map. It looks weird to me, `ConcurrentHashMap` shouldn't normally need that sort of locking, but don't neglect that the programmer may have a need for locking that object in some other part of the code. Or the programmer made a mistake. It happens. — markspace, Jul 05 '21 at 23:42
I agree with [comment by markspace](https://stackoverflow.com/questions/68263007/concurrenthashmap-remove-method-thread-safetiness#comment120644859_68263007). In addition, I wonder why the `aliasMap` is declared as a `Map` rather than as a `ConcurrentMap`. I suspect this code was originally written without thinking to use a concurrent map instance, hence the `synchronized` to manage concurrency. Spring’s development was started before the release of Java 5 that brought `ConcurrentMap` & `ConcurrentHashMap` in 2004. But my Comment here is all wild speculation, I have no real knowledge. — Basil Bourque, Jul 06 '21 at 00:38
Here is the place where the code is located, https://github.com/spring-projects/spring-framework/blob/main/spring-core/src/main/java/org/springframework/core/SimpleAliasRegistry.java — CHANist, Jul 06 '21 at 01:01

Panagiotis Bougioukos · Answer 1 · 2021-07-06T20:45:46.350

Using synchronized onConcurrentHashMap is like canceling the use of ConcurrentHashMap. So in that case a HashMap with synchronized would be the same.

The point of synchronized is to lock the whole map. ConcurrentHashMap is implemented without the need to lock the whole map as it provides segments lock, which automatically lock a much smaller part of the ConcurrentHashMap during concurent access.

Maybe there is a bug to the project and the OP found a quick fix by locking the whole map.

Checking a little more this issue I think the OP wanted to make a java method atomic on map level (get put) and was afraid that the java method would not behave as expected so it went forward and synchronized everywhere the whole map. As he have also written he could also take advantage of compute(..), computeIfAbsent(..), computeIfPresent(..) methods of concurentHashMap to avoid that but I think he took the quick road of synchronizing the whole map.

He could also have converted the concurentHashMap into a HashMap after he chose that fix as he have cancelled the main usage of concurentHashMap.

Take also a look here (SO thread) to understand why the OP has struggled with that issue.

score 0 · Answer 2 · answered Jul 06 '21 at 02:16

in my thought : registerAlias/getAliases both use synchronized lock this map. if removeAlias method without synchronized, maybe occur this sense: in removeAlias after call this.aliasMap.remove, it has successful remove alias, but the removeAlias not finish, now other thread call registerAlias put a same alias in this map. so removeAlias is not successful perform. in removeAlias internal has different data

ConcurrentHashMap remove method Thread Safetiness

Update

2 Answers2