CODEWITHSUNDEEP
javamultithreadingconcurrenthashmap
user788052
user788052

Reputation: 143

ConcurrentHashMap: Need for synchronization when storing a collection as value

I have some code that uses ConcurrentHashMap and wanted to confirm whatever I am doing is correct or not.

The map stores String as key and a collection (Set) as value. I initially had an implementation where I would use lock striping pattern for doing some operations atomically (put, remove, etc.) but then realized that instead of that I can use the computeIfAbsent/putIfAbsent (for put) and computeIfAbsent (for remove). Multiple threads call all 4 methods (get, put, removeKey, removeValue).

Can anyone tell me if my AFTER code is correct and thread-safe? Thanks in advance!

private final ConcurrentMap<K, Set<V>> map = new ConcurrentHashMap<>();
private final ConcurrentMap<K, Object> locks = new ConcurrentHashMap<>();

public Set<V> getValue(K key) {
    if (map.get(key) == null) return null;
    return map.get(key).stream().map(k -> k.getValue()).collect(Collectors.toSet());
}

BEFORE

private void putValue(K key, V value) {
        Object lock = locks.putIfAbsent(key, new Object());
    if (lock == null) lock = locks.get(key);
        Set<V> existingSet = map.computeIfAbsent(key, k -> {
            Set<V> values = new ConcurrentSkipListSet<>(MY_COMPARATOR);
            values.add(value);
            return values;
        });
        if (existingSet != null) { // <- my guess is that this is unnecessary
            synchronized (lock) {
                map.get(key).add(expiringValue);
            }
        }
    }

public void removeKey(K key) {
    if (key == null) return;
    if (map.get(key) != null) {
        Object lock = locks.get(key);
        synchronized (lock) {
            map.remove(key);
        }
        locks.remove(key);
    }
}

public void removeValue(K key, V value) {
    if (key == null || value == null) return;
    if (map.get(key) != null) {
        Object lock = locks.get(key);
        synchronized (lock) {
            map.get(key).remove(value);
            if (map.get(key).size() == 0) {
                map.remove(key);
            }
        }
    }
}

AFTER

private void putValue(K key, V value) {
    map.computeIfAbsent(key, k -> new ConcurrentSkipListSet<>(MY_COMPARATOR)).add(value);
}

public void removeKey(K key) {
    map.remove(key);
}

public void removeValue(K key, V value) {
    Set<ExpiringValue<V>> resultingValues = map.computeIfPresent(key, (k, values) -> {
        values.remove(value);
        return values;
    });
    if (resultingValues != null && resultingValues.size() == 0) {
        map.remove(key);
    }
}

Upvotes: 1

Views: 1094

Answers (2)

Radiodef
Radiodef

Reputation: 37855

I think the most stable you could do is like this:

private void putValue(K key, V value) {
    map.compute(key, (k, values) -> {
        if (values == null) {
            values = new ConcurrentSkipListSet<>(MY_COMPARATOR);
        }
        values.add(value);
        return values;
    });
}

public void removeKey(K key) {
    map.remove(key); // No change.
}

public void removeValue(K key, V value) {
    map.computeIfPresent(key, (k, values) -> {
        values.remove(value);
        return values.isEmpty() ? null : values;
    });
}

Those guarantee that all actions are atomic. The use of compute in putValue feels a little clunky, but ConcurrentHashMap guarantees the behavior, so it's whatever.

The remapping function returning null in computeIfPresent is just the correct way to do what you were already trying to do:

If the function returns null, the mapping is removed.

Upvotes: 1

Kayaman
Kayaman

Reputation: 73568

At least one possible problem is that you can lose values with your putValue. The computeIfAbsent is thread-safe, but the add() is not anymore. Therefore you can (depending on your logic) come up with the following scenario:

T1: computeIfAbsent creates the set for key K
T2: removes K
T1: performs add on the set, which is no longer in the map

Upvotes: 1

Related Questions