16

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?

Desmond Zhou
  • 1,369
  • 1
  • 11
  • 18

4 Answers4

18

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

John Vint
  • 39,695
  • 7
  • 78
  • 108
  • 1
    Good idea, but I think you have something to correct: you call `newSum.get()` twice in the condition check, but it could change in between the two calls. You should maybe use a `do { } while()` where you only call `get` once. – toto2 Dec 19 '11 at 21:37
  • 1
    According to the Java Language Spec, "It is recommended that code not rely crucially on" the requirement that "that the operands of operators appear to be evaluated in a specific *evaluation order*, namely, from left to right." So although I believe that your code is sound, I think it's probably better to write something like `while(true) { BigDecimal oldVal = newSum.get(); BigDecimal newVal = oldVal.add(amount); if(newSum.compareAndSet(oldVal, newVal)) break; }`. – ruakh Dec 19 '11 at 21:41
  • Are you sure that `synchronized` is better for high contention? I thought (for no particular reason) that CAS was always better. – toto2 Dec 19 '11 at 21:42
  • @toto2 you are absolutely right, thanks let me update that now! – John Vint Dec 19 '11 at 21:42
  • @toto2 with high contention threads will often fail because of many retries where as synchronize will queue/manage them easier with higher overall throughput. – John Vint Dec 19 '11 at 21:43
  • 1
    @toto2: Since the call on the right happens after the call on the left, any change between the two calls would cause the `compareAndSet` to do nothing and return `false`. So I believe it's technically O.K.; but I agree with you anyway that it would be better call `get` only once, so that the code is easier to reason about. – ruakh Dec 19 '11 at 21:43
  • @ruakh I naively (and granted too quickly) put together the solution without considering that loss of atomicity, thanks for your update – John Vint Dec 19 '11 at 21:45
  • @ruakh Are you sure that the order of operations is guaranteed here since the code does not exist in a synchronized block? I agree with your earlier comment about doing the get only once, since I am not sure that there is a happens before guarantee between newSum.get() and newSum.get().add(), so the compiler may be free to reorder those instructions. – Trevor Freeman Dec 19 '11 at 21:48
  • @increment1 If you take a look at the AtomicXXX.incrementAndGet methods, they use a similar style and it is guaranteed to be thread safe (without synchronized). To address your other point it isnt so much an ordering/memory issue as it was more an atomicity issue. – John Vint Dec 19 '11 at 21:52
  • @increment1: I'm not sure, no, but I believe so, since AtomicReference offers the same guarantees as volatiles, and a volatile write in another thread would have to be ordered with respect to volatile reads in this thread. But this stuff is confusing, and I repeat that I am not sure. I'll sleep on it! (By the way, there certainly *is* a happens-before guarantee, since the JMM specifies that two actions in the same thread always have a happens-before -- even with non-volatiles. It's just that a happens-before guarantee doesn't mean as much as it sounds like it means.) – ruakh Dec 19 '11 at 22:05
  • @ruakh you are right. There is a happens-before relationship (which includes both compiler and memory ordering) with respect to `AtomicReference.compareAndSet` and `AtomicReference.get` – John Vint Dec 19 '11 at 22:09
  • @JohnVint Thanks! I know synchronize would work but I haven't seen AtomicReference before, will definitely have a deep look into this. – Desmond Zhou Dec 19 '11 at 22:39
  • @ruakh: The evaluation order of the arguments to `compareAndSet` wouldn't matter since BigDecimal is immutable, `oldVal.add(amount)` does not change `oldVal`. – Jörn Horstmann Dec 20 '11 at 10:08
  • @JörnHorstmann: Er, sorry, but you've come in a bit late. The evaluation order certainly did matter, for the answer that was there when I posted my first comment. The answer has changed since then, to address the concerns raised in the discussion. (See http://stackoverflow.com/posts/8567817/revisions.) – ruakh Dec 20 '11 at 12:57
  • @ruakh: Oh, sorry, I did not notice the older revisions. The discussion makes much more sense now :) – Jörn Horstmann Dec 20 '11 at 13:25
4

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.

Trevor Freeman
  • 7,112
  • 2
  • 21
  • 40
4

That is not safe, because threads A and B might both call sumByAccount.get(account) at the same time (more or less), so neither one will see the result of the other's add(amount). That is, things might happen in this sequence:

  • thread A calls sumByAccount.get("accountX") and gets (for example) 10.0.
  • thread B calls sumByAccount.get("accountX") and gets the same value that thread A did: 10.0.
  • thread A sets its newSum to (say) 10.0 + 2.0 = 12.0.
  • thread B sets its newSum to (say) 10.0 + 5.0 = 15.0.
  • thread A calls sumByAccount.put("accountX", 12.0).
  • thread B calls sumByAccount.put("accountX", 15.0), overwriting what thread A did.

One way to fix this is to put synchronized on your addToSum method, or to wrap its contents in synchronized(this) or synchronized(sumByAccount). Another way, since the above sequence of events only happens if two threads are updating the same account at the same time, might be to synchronize externally based on some sort of Account object. Without seeing the rest of your program logic, I can't be sure.

ruakh
  • 175,680
  • 26
  • 273
  • 307
2

Yes, you need to synchronize since otherwise you can have two threads each getting the same value (for the same key), say A and thread 1 add B to it and thread 2 adds C to it and store it back. The result now will not be A+B+C, but A+B or A+C.

What you need to do is lock on something that is common to the additions. Synchronizing on get/put will not help, unless you do

synchronize {
    get
    add
    put
}

but if you do that then you will prevent threads from updating values even if it is for different keys. You want to synchronize on the account. However, synchronizing on the string seems unsafe as it could lead to deadlocks (you don't know what else locks the string). Can you create an account object instead and use that for locking?

Roger Lindsjö
  • 11,330
  • 1
  • 42
  • 53