How to make updating BigDecimal within ConcurrentHashMap thread safe

Question

I am making an application that takes a bunch of journal entries and calculate sum.

Is below way of doing it is thread/concurrency safe when there are multiple threads calling the addToSum() method. I want to ensure that each call updates the total properly.

If it is not safe, please explain what do I have to do to ensure thread safety.

Do I need to synchronize the get/put or is there a better way?

private ConcurrentHashMap<String, BigDecimal> sumByAccount;

public void addToSum(String account, BigDecimal amount){
    BigDecimal newSum = sumByAccount.get(account).add(amount);
    sumByAccount.put(account, newSum);
}

Thanks so much!

Update:

Thanks everyone for the answer, I already get that the code above is not thread-safe.

Thanks Vint for suggesting the AtomicReference as an alternative to synchronize. I was using AtomicInteger to hold integer sums before and I was wondering if there are something like that for BigDecimal.

Is the a definitive conclusion on the pro and con of the two?

Answer 1

You can use synchronized like the others suggested but if want a minimally blocking solution you can try AtomicReference as a store for the BigDecimal

ConcurrentHashMap<String,AtomicReference<BigDecimal>> map;

public void addToSum(String account, BigDecimal amount) {
    AtomicReference<BigDecimal> newSum = map.get(account);
    for (;;) {
       BigDecimal oldVal = newSum.get();
       if (newSum.compareAndSet(oldVal, oldVal.add(amount)))
            return;
    }
}

Edit - I'll explain this more:

An AtomicReference uses CAS to atomically assigns a single reference. The loop says this.

If the current field stored in AtomicReference == oldVal [their location in memory, not their value] then replace the value of the field stored in AtomicReference with oldVal.add(amount). Now, any time after the for-loop you invoke newSum.get() it will have the BigDecimal object that has been added to.

You want to use a loop here because it is possible two threads are trying to add to the same AtomicReference. It can happen that one thread succeeds and another thread fails, if that happens just try again with the new added value.

With moderate thread contention this would be a faster implementation, with high contention you are better off using synchronized

Answer 2

Your solution is not thread safe. The reason is that it is possible for a sum to be missed since the operation to put is separate from the operation to get (so the new value you are putting into the map could miss a sum that is being added at the same time).

The safest way to do what you want to do is to synchronize your method.