1

I have one thread that updates data in Map and several threads that read this data. Now my code looks like this:

public class Updater {
    private ConcurrentMap<String, Integer> valuesMap = new ConcurrentHashMap<>();
    private ReadWriteLock reentrantReadWriteLock = new ReentrantReadWriteLock();

    public void update(Settings settings) {
        reentrantReadWriteLock.writeLock().lock();
        try {
            for (Map.Entry<String, Integer> entry : valuesMap.entrySet()) {
                valuesMap.put(entry.getKey(), 
                              entry.getValue() + settings.getUpdateValue());
            }
        } finally {
            reentrantReadWriteLock.writeLock().unlock();
        }
    }

    public Integer getValue(String key) {
        reentrantReadWriteLock.readLock().lock();
        try {
            return valuesMap.get(key);
        } finally {
            reentrantReadWriteLock.readLock().unlock();
        }
    }
}

But I think I overdid it. Can I use only ConcurrentHashMap in this situation?

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
Violetta
  • 509
  • 4
  • 12
  • I'm 90% sure that `ConcurrentHashMap` will be enough. The only think I would change is to use `ConcurrentHashMap.compute()` for updating instead of `getValue()` + `put()` combo - even tho it won't really matter in case of single thread updating. – Amongalen Jun 15 '20 at 14:36

1 Answers1

3

Can I use only ConcurrentHashMap in this situation?

It depends on whether you want the update operation to be atomic; i.e. whether you want a reader to never see the state of the map when only some of the put operations have been performed.

  • If update doesn't need to be atomic, then locking is unnecessary. In fact if is an unwanted concurrency bottleneck.

  • If update needs to be atomic, then the lock is necessary. But if you are controlling access with locks, then you don't need to use a ConcurrentHashMap. A simple HashMap would be better.

I don't think that ConcurrentHashMap has a way to implement multiple put operations as an atomic action.

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
  • 1
    That’s right, there is no way to make atomic updates of multiple keys with `ConcurrentHashMap` and any use of an additional blocking mechanism defeats the entire purpose of `ConcurrentHashMap`. When no atomicity across multiple keys is needed, the OP’s original code still needs to fix the update of a single key, i.e. by replacing `valuesMap.put(entry.getKey(), entry.getValue()+settings.getUpdateValue());` with `valuesMap.merge(entry.getKey(), settings.getUpdateValue(), Integer::sum);`. Assuming that `settings.getUpdateValue()` is not subject to concurrent updates, the lock can be removed then. – Holger Jun 17 '20 at 08:34