Unsynchronized method producing consistent results

Question

I'm new to Java and playing around with threads. I wrote this small program to check what happens when 3 threads are accessing an unsynchronized method:

class SharedValueHolder {
  public static int globalCounter = 0;
  public static void increment() {
    globalCounter++;
  }
}

class Incrementor implements Runnable {
  public void run() {
    for (int i = 0; i < 5; i++) {
      //reportValue();
      SharedValueHolder.increment();
    }
  }

  private void reportValue() {
    String threadName = Thread.currentThread().getName();
    System.out.println(threadName + " - " + SharedValueHolder.globalCounter);
  }
}

public class ThreadTest {
  public static void main(String... args) throws InterruptedException {
    runTest();
  }

  private static void runTest() throws InterruptedException {
    Thread thread1 = new Thread(new Incrementor());
    Thread thread2 = new Thread(new Incrementor());
    Thread thread3 = new Thread(new Incrementor());

    thread1.start();
    thread2.start();
    thread3.start();

    Thread.sleep(300);
    System.out.println(SharedValueHolder.globalCounter);
  }
}

Suprisingly, the result is almost always 15. Sometimes I do get a 14, but that's very rare.

Now if I uncomment the call to reportValue() in Incrementor.run I see something like this for example:

Thread-2 - 0
Thread-1 - 0
Thread-0 - 0
Thread-0 - 3
Thread-0 - 4
Thread-0 - 5
Thread-0 - 6
Thread-1 - 2
Thread-1 - 8
Thread-1 - 9
Thread-1 - 10
Thread-2 - 1
Thread-2 - 12
Thread-2 - 13
Thread-2 - 14

Which clearly shows that the threads "see" the same values at the same times, but still the result is correct. Could somebody explain to me how this works?

"I do get a 14, but that's very rare." - So it's **not** consistent... as expected. — Jacob G., Apr 27 '18 at 15:51
How did you conclude "that the threads "see" the same values at the same times"? The output does not speak for that. — luk2302, Apr 27 '18 at 15:53
Does it not? As I said, I'm new to Java. What is a good way to verify that? — Marcin Wolniewicz, Apr 27 '18 at 15:54
The fact that there is "1" after the "10" looks ... suspicious. I am unsure what you are asking, is it "why does it not always work" or "why does it sometimes work"? — luk2302, Apr 27 '18 at 15:57
Your numbers are too low. Try iterating to 100000 instead of 5. You are just finishing so quickly that the threads don't interact much. You might also put a sleep(100) in your inner loop if you want to simulate interactions. — Bill K, Apr 27 '18 at 15:59
Yeah ok that actually explains it. Thanks @BillK increasing the numbers did actually provide consistently wrong results as expected. I guess I was expecting the numbers being in the range of 5 - 15 most of the time and was surpised I'm getting a correct result mostly. — Marcin Wolniewicz, Apr 27 '18 at 16:02
Have you tried making the globalCounter variable volatile? I thought increment would be atomic, especially after a compiler optimization that convered it to a pre-increment — muzzlator, Apr 27 '18 at 16:07
Also, using Thread.sleep() as a substitute for calling join() on the threads isn't great practice. You should deterministically join the threads back together, not rely on sleep statements — muzzlator, Apr 27 '18 at 16:08
OK I take it back, x++ isn't atomic: https://stackoverflow.com/questions/25168062/why-is-i-not-atomic http://madbean.com/2003/mb2003-44/ — muzzlator, Apr 27 '18 at 16:13
The best fix, by the way, is probably to make globalVariable an AtomicInteger. You could also make increment synchronized, but I believe AtomicInteger is more optimized. — Bill K, Apr 27 '18 at 16:14

score 2 · Accepted Answer · answered Apr 27 '18 at 16:00

Concurrency is not an easy topic, and one of the reasons it's tricky is because sometimes you may get a correct result (and think your code is good), but that doesn't mean the code is really correct; it means given the environment you're running it with, the number of threads... It's working just fine. But it's likely to fail in a highly concurrent environment.

Still, you say you're seeing the same result almost always, which makes it NOT always.

It's also the scope of your problem, with a loop of 5 elements only. Possibly the second and third thread haven't even started when the first thread finishes.

But it's easy to see it's wrong. Try running this sample instead:

class SharedValueHolder {
  public static int counter = 0;
}

class Incrementor implements Runnable {
  public void run() {
    for (int i = 0; i < 100000; i++) {
      SharedValueHolder.counter++;
    }
  }
}

public class ThreadTest {
  public static void main(String... args) throws InterruptedException {
    Thread thread1 = new Thread(new Incrementor());
    Thread thread2 = new Thread(new Incrementor());
    thread1.start();
    thread2.start();

    Thread.sleep(2000);
    System.out.println(SharedValueHolder.counter);
  }
}

Two threads adding 100.000, so you'd expect 200.000 at the end. But instead, I get:

Unsynchronized method producing consistent results

1 Answers1