Why is this code not thread-safe even though an AtomicInteger is used to track progress?

Question

I tried making a class that extends thread which simply takes an array of strings and prints the first 2 strings out alternately for 10000 iterations. I keep track of the index to print from using an AtomicInteger (counter), however the output sometimes prints this: hello hello hello w hello hello etc. instead of alternating at each iteration. Why is this and how could I fix it without just putting 'synchronized' in the run method?

public class MyThreadDelegate implements Runnable {

  List<String> words;
  AtomicInteger counter = new AtomicInteger(0);

  public MyThread(List<String> words) {
    this.words = words;
  }

  @Override
  public void run() {
    for (int i = 0; i < 10000; i++) {
      System.out.println(words.get(counter.getAndIncrement()%2) + counter.get());
    }
  }

  public static void main(String[] args) {

    MyThreadDelegate myThreadDelegate = new MyThreadDelegate(Arrays.asList("hello", "w"));

    Thread t1 = new Thread(MyThreadDelegate);
    Thread t2 = new Thread(MyThreadDelegate);

    t1.start();
    t2.start();
  }
}

Please! Change `MyThread extends Thread` to `MyThreadDelegate implements Runnable`. Your `myThread` object is _not_ a thread because you never call `myThread.start()`. Your `t1` and `t2` objects are threads which each use your `myThread` object as their `Runnable` _[delegate](https://en.wikipedia.org/wiki/Delegation_pattern)_. — Solomon Slow, Apr 26 '19 at 13:27
P.S., This exercise teaches you something about how to sync-up threads, but in any real program, if the threads don't spend at least _some_ time doing independent, un-synchronized tasks, then that pretty much defeats the whole purpose of using threads in the first place. — Solomon Slow, Apr 26 '19 at 13:53

Rick · Accepted Answer · 2023-03-08T12:24:12.457

While the numbers are retrieved one by one, the rest of the method isn't synced up. So sometimes this might happen:

t1: gets value 0 from the counter
t2: gets value 1 from the counter
t2: prints w
t1: prints hello

A quick fix would be to put the whole System.out line in a synchronized block, but that would not guarantee that the threads take turns. It just guarantees that echt value is retrieved, incremented and printed before the next one.

If you want to have the threads actually take turns, you'll have to implement some sort of locking. But if you want to have the threads wait for each other, why are you using multiple threads?

EDIT: also, you should probably have MyThread implement Runnable instead of extending Thread if you're going to use it this way. See this link for more: https://www.baeldung.com/java-runnable-vs-extending-thread (Solomon Slow beat me to it :)

Why is this code not thread-safe even though an AtomicInteger is used to track progress?

1 Answers1