Need to understand the problem in AtomicInteger code usage in multithreaded environment

Question

In one of the interview, a coding question was asked to me and I had to find the problem in that code and suggest proper solution.

Please find below the entire code:

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;

public class Atomic {

    static AtomicInteger count = new AtomicInteger(0);
    static int counter = 0;

    public static class Runnable extends Thread {

    public void run() {
        while (count.getAndSet(1) != 0) {
            try {
                Thread.sleep(3000);
            } catch (Exception e) {

            }
        }
        counter = counter + 1;
        count.set(0);
    }

}

public static void main(String[] args) {
    ExecutorService executor = Executors.newFixedThreadPool(10);
    for (int i = 0; i < 10; i++) {
        Runnable runnable = new Runnable();
        executor.execute(runnable);
    }
    executor.shutdown();
}

}

This code is running properly. But question is , there is some problem in this code if number of threads get increased or if I run For loop for almost 10000 times.

I tried to find the problem, but couldn't find one.

Hint: What might happen if two threads try to increment `counter` at the same time? — Ted Hopp, Feb 26 '20 at 07:42
A non-functional issue in this code is "`public static class Runnable extends Thread {`". That *works*, but oof, that's really not a good idea, in at least 2 ways. — Andy Turner, Feb 26 '20 at 07:50

score 4 · Accepted Answer · answered Feb 26 '20 at 08:07

There are several things wrong with this code. You've not stated with "there is some problem" means, but here are the things that jump out.

Firstly, the counter variable is not updated safely. Multiple threads don't have guaranteed visibility of the last-written value; nor do you have the guarantee that no other thread has updated its value in between the read and the write.

The simple solution to this: change counter to an AtomicInteger, and use getAndIncrement or incrementAndGet to increment it.

Secondly, public static class Runnable extends Thread { is extremely dubious.

Don't hide the names of commonly-known Java classes (this is hiding java.lang.Runnable)
Don't extend Thread directly, especially when all you need is a java.lang.Runnable to add execute with an ExecutorService.

A more suitable class declaration would be:

public static class MyRunnable implements Runnable {

(or whatever you want to call it)

Or you can just declare an anonymous class:

executor.execute(new Runnable() { /* body */ });

Or you can just declare a lambda:

executor.execute(() -> { /* body */ });

Thirdly, count doesn't really seem to be serving an obvious purpose here. The logic of the runnable seems to be:

If "flag" is false:
1. Set "flag" to true
2. Increment a variable
3. Set "flag" to false
Otherwise:
1. Wait 3 seconds
2. Try again

count is playing the role of "flag" here. It's effectively just an AtomicBoolean.

But you don't need a separate count variable at all, if you make the counter an AtomicInteger:

while (true) {
  int current = counter.get();
  if (counter.compareAndSet(current, current + 1)) {
    // Nothing else is trying to update "current" at the same time:
    // we updated it. Stop.
    break;
  }

  // Something else is trying to update at the same time.
  // Sleep for 3 seconds.
  Thread.sleep(3000);
}

Need to understand the problem in AtomicInteger code usage in multithreaded environment

1 Answers1