ConcurrentHashMap с синхронизированным

Question

Я поддерживаю некоторый устаревший код и нашел некоторую реализацию с synchronized ключевым словом на ConcurrentHashMap.Мне кажется это ненужным:

public class MyClass{

    private final Map<MyObj, Map<String, List<String>>> conMap = new ConcurrentHashMap<>();

    //...

    //adding new record into conMap:
    private void addToMap(MyObj id, String name, String value){
        conMap.putIfAbsent(id, new ConcurrentHashMap<>());
        Map<String, List<String>> subMap = conMap.get(id);
        synchronized(subMap){                            // <-- is it necessary?
            subMap.putIfAbsent(name, new ArrayList<>());
            subMap.get(name).add(value);
        }
    }

    //...

    public void doSomthing((MyObj id){
        List<Map<String, List<String>>> mapsList = new LinkedList<>();
        for(MyObj objId: conMap.keySet()){              
            if(objId.key1.equals(id.key1)){
                mapsList.add(conMap.get(objId));
            }
        }

        for(Map<String, List<String>> map: mapsList){
            synchronized(map){                       // <-- is it necessary?
                if(timeout <= 0){
                    log(map.size());
                    for(List<String> value: map.values(){
                        log(id, value);
                    }
                }
                else{
                    int sum = 0;
                    for(map.Entry<String, List<String>> val: map.entrySet()){
                        sum += val.getValue().size();
                    }
                    log(sum);
                    map.wait(timeout);
            }
    }

    //...

}

Итак, разумно ли использовать ключ synchronized для объекта, который уже параллелен?Или это две разные вещи?

Answer 1

Это необходимо, поскольку несколько потоков могут пытаться добавить к одному и тому же ArrayList одновременно. synchonized защищает от этого, поскольку ArrayList явно не синхронизирован.

Начиная с Java 8 у нас есть computeIfAbsent, что означает, что путы, за которыми следуют получаемые ими, могут быть упрощены. Я бы написал так, синхронизация не требуется:

conMap.computeIfAbsent(id, k -> new ConcurrentHashMap<>())
    .computeIfAbsent(name, k -> new CopyOnWriteArrayList<>()) // or other thread-safe list
    .add(value);

Answer 2

В этом случае:

    synchronized(subMap){                            // <-- is it necessary?
        subMap.putIfAbsent(name, new ArrayList<>());
        subMap.get(name).add(value);
    }

нужен synchronized. Без этого у вас могло бы быть два потока, одновременно обновляющих тот же самый экземпляр ArrayList. Поскольку ArrayList не является поточно-ориентированным, метод addToMap также не будет поточно-ориентированным.

В этом случае:

        synchronized(map){                       // <-- is it necessary?
            if(/*condition*/){
                log(map.size());
                for(List<String> value: map.values(){
                    log(id, value);
                }
            }
            else{
                int sum = 0;
                for(map.Entry<String, List<String>> val: map.entrySet()){
                    sum += val.getValue().size();
                }
                log(sum);
                map.wait(timeout);
        }

synchronized необходим.

• В ветви if метод log (или что-то вызванное из него), вероятно, будет вызывать ArrayList::toString, который будет повторять каждый ArrayList. Без синхронизации на уровне подкарты может быть одновременный add другим потоком (например, вызов addToMap). Это означает, что существуют опасности с памятью, и ConcurrentModificationException может быть возможным в методе toString().

• В ветви else вызов size() обращается к полю размера в каждом ArrayList в подкарте. Без синхронизации на уровне подкарты в одном из этих списков может быть одновременное add. Это может привести к тому, что метод size() вернет устаревшее значение. Кроме того, вы не гарантированно увидите записи карты, добавленные в подкарту, пока вы выполняете ее. Если произойдет любое из этих событий, sum может быть неточным. (Является ли это действительно проблемой, зависит от требований к этому методу: неточные подсчеты могут быть приемлемыми.)

Answer 3

ConcurrentHashMap синхронизирует каждый отдельный вызов метода, так что никакой другой поток не может получить доступ к карте (и, возможно, нарушить внутреннюю структуру данных карты).

Синхронизированный блок синхронизирует два или более последовательных вызова метода, поэтомучто никакой другой поток не может изменить структуру данных между вызовами (и, возможно, нарушить согласованность данных в отношении логики приложения).

Обратите внимание, что синхронизированный блок работает, только если весь доступ к HashMapвыполняется из синхронизированных блоков с использованием одного и того же объекта монитора.

Answer 4

Другие ответы не соответствуют этому биту ...

   for(Map<String, List<String>> map: mapsList){
        synchronized(map){                       // <-- is it necessary?
            if(/*condition*/){
                ...iterate over map...
            }
            else {
                ...iterate over map...
            }
        }
   }

Это необходимо? Трудно сказать.

Что такое /*condition*/? Не препятствует ли синхронизация в map другому потоку A изменять значение /*condition*/ после того, как поток B проверил его, но до или во время выполнения потоком B одной из двух ветвей? Если это так, то блок synchronized может быть очень важным.

Как насчет этих итераций? Не мешает ли синхронизация в map другому потоку A изменять содержимое карты во время итерации потока B? Если это так, то блок synchronized может быть очень важным.