0

Will the following code cause race condition issue if several threads invoke the "incrementCount" method?

public class sample {

       private AtomicInteger counter = new AtomicInteger(0);

       public int getCurrentCount {
          int current = counter.getAndIncrement();
          if (counter.compareAndSet(8, 0)) current = 0;
          return current;
      }
}

If it causes race condition, what are the possible solution other than using synchronized keyword?

bharath
  • 14,283
  • 16
  • 57
  • 95
kenn3th
  • 1,187
  • 2
  • 22
  • 47

5 Answers5

3

You probably don't want to let the counter exceed 8 and this won't work. There are race conditions.

It looks like you want a mod 8 counter. The easiest way is to leave the AtomicInteger alone and use something like

int current = counter.getAndIncrement() & 7;

(which is fixed and optimized version of % 8). For computations mod 8 or any other power of two it works perfectly, for other number you'd need % N and get problems with int overflowing to negative numbers.

The direct solution goes as follows

public int getCurrentCount {
    while (true) {
        int current = counter.get();
        int next = (current+1) % 8;
        if (counter.compareAndSet(current, next))) return next;
    }
}

This is about how getAndIncrement() itself works, just slightly modified.

maaartinus
  • 44,714
  • 32
  • 161
  • 320
2

Yes, it probably does not do what you want (there is a kind of race condition).

  1. One thread may call getAndIncrement() and receive a 8
  2. A second thread may call getAndIncrement() and receive a 9
  3. The first thread tries compareAndSet but the value is not 8
  4. The second thread tries compareAndSet but the value is not 8

If there's no risk of overflowing, you could do something like

return counter.getAndIncrement() % 8;

Relying on that something does not overflow seems like a poor idea to me though, and I would probably do roughly what you do, but let the method be synchronized.

Community
  • 1
  • 1
aioobe
  • 413,195
  • 112
  • 811
  • 826
  • 1
    +0: % 8 can return a negative number. – Peter Lawrey Mar 17 '11 at 10:08
  • Using `counter.getAndIncrement() & 7;` works always and is much faster. Something like this is applicable to powers of two only. – maaartinus Mar 17 '11 at 10:13
  • Computing `x & 7` takes one cycle, computing `x % 8` takes dozens of them. Of course in the context of the whole program (or even in this context) it may be absolutely negligible. No hurry to optimize, but `&` simply behaves much better than `%`. – maaartinus Mar 17 '11 at 14:18
1

Based on the code for getAndIncrement()

public int getCurrentCount() {
  for(;;) {
    int courrent = counter.get();
    int next = current + 1;
    if (next >= 8) next = 0;
    if (counter.compareAndSet(current, next))
      return current;
  }
}

However a simpler implementation in your case is to do

public int getCurrentCount() {
  return counter.getAndIncrement() & 0x7;
}
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
1

What are you trying to achieve? Even if you use the fixes proposed by ajoobe or maartinus you can end up with different threads getting the same answer - consider 20 threads running simultaneously. I don't see any interesting significance of this "counter" as you present it here - you may as well just pick a random number between 0 and 8.

djna
  • 54,992
  • 14
  • 74
  • 117
0

I assume that the what you want is to have a counter form 0 to 7.

If that is the case then a race condition can possibly happen and the value of counter can become 9.

Unless you are ok to use % soln. as said by others, you micht have to use synchronized.

Suraj Chandran
  • 24,433
  • 12
  • 63
  • 94