2

I ran across a doubt if below is thread safe,

// is this thread safe, final int MAX_COUNT = 3 ?
if (retryCount.get() < MAX_COUNT) {
    // some other code
    retryCount.getAndIncrement();
} else {
    // reset count & some other code
    retryCount.set(0);
}

Is the above conditional check thread safe ?

djm.im
  • 3,295
  • 4
  • 30
  • 45
Vijay
  • 384
  • 4
  • 16

2 Answers2

4

No it doesn't.

Suppose two threads T1 and T2 and retryCount contains actually the 2 value.
Suppose T1 executes if(retryCount.get() < MAX_COUNT){ (evaluated to true) but doesn't reach retryCount.getAndIncrement();.
T1 is paused. T2 is resumed.
T2 executes if(retryCount.get() < MAX_COUNT){ that is still evaluated to true.

So you are sure that retryCount will be valued to 4.

You need explicit synchronization and in this case the AtomicInteger may not be required :

synchronized(lock){
  if(retryCount.get() < MAX_COUNT){
  // some other code
  retryCount.getAndIncrement();
  }else{
   // reset count & some other code
   retryCount.set(0);
  } 
}
davidxxx
  • 125,838
  • 23
  • 214
  • 215
  • 2
    Alternatively could use `retryCount.getAndUpdate(i -> (i + 1) % (MAX_COUNT + 1))`. – Bubletan Jan 17 '18 at 20:43
  • 1
    @Bubletan you're leaving out the 'other code' portion. – matt Jan 17 '18 at 20:43
  • 1
    @matt Well I did assume the other code can be executed in any order. – Bubletan Jan 17 '18 at 20:54
  • 2
    @Bubletan’s example can be extended to `if(retryCount.updateAndGet(i -> (i + 1) % (MAX_COUNT + 1)) != 0) { /* code for ordinary case */ } else { /* code when resetting */ }` if that’s not suitable, i.e. the order of the “other code” and the counter update matters, then `AtomicInteger` is not the right tool for the job. As then, all other access to the `retryCount` and to whatever “other code” modifies needs synchronization (*on the same lock*) too… – Holger Jan 23 '18 at 16:35
1

No it's not thread safe because the code is performing what's called a check-then-act operation.

AtomicInteger is thread safe in itself meaning it's individual methods are atomic but performing compound operations are not atomic. so the above code needs synchronization

here's some great notes from Java Concurrency In Practice

Ramanlfc
  • 8,283
  • 1
  • 18
  • 24