CODEWITHSUNDEEP
javamultithreadingthread-safety
du369
du369

Reputation: 821

Is this code not thread safe?

I was expecting this code to be thread safe. I ran it a few times, but got different results. However, if I uncomment the sleep(1000) part, it prints 10000 every time (at least from the results from my test runs).

So what's wrong? Could it be something to do with thread.join()?

public class Test implements Runnable{

    private int x;

    public synchronized void run(){
            x++;
    }

    public static void main(String args[]){
        Test test = new Test();
        Thread thread = null;
        for (int i = 0; i < 10000; i++) {
            thread = new Thread(test);
            try {
                thread.join();
            } catch (InterruptedException e) {}
            thread.start();
        }
//        try {
//          Thread.sleep(1000);
//      } catch (InterruptedException e) {
//          e.printStackTrace();
//      }
        System.out.println(test.x);
    }
}

edit: oops, my bad. I misunderstood how Thread#join functions. And synchronizing on run() method is a bad idea.

Upvotes: 2

Views: 206

Answers (3)

Nathan Hughes
Nathan Hughes

Reputation: 96385

There are two problems here:

  • a race condition where the main thread finishes before all the worker threads.

  • a memory visibility issue where the main thread is not guaranteed to see the updated value of x.

Thread#join is implemented using Object#wait. The condition variable used is the alive flag on the Thread:

groovy:000> new Thread().isAlive()
===> false

Thread.join is checking the alive flag before the thread has started, so isAlive returns false and join returns before the thread can start. The counter still gets incremented eventually, but since the join doesn't happen for that thread then the main thread may be printing out the results for x before all the threads can execute.

Adding the sleep gives all the threads enough time to finish up that x is what you expect by the time that the main thread prints it out.

In addition to the race condition, there is a memory visibility issue since the main thread is accessing x directly and is not using the same lock as the other threads. You should add an accessor to your Runnable using the synchronized keyword:

public class Test implements Runnable{

    private int x;

    public synchronized void run(){
        x++;
    }

    public synchronized int getX() {
        return x;
    }

and change the main method to use the accessor:

    System.out.println(test.getX());

Memory visibility issues may not be apparent since they depend on how aggressive the JVM is about caching and optimizing. If your code runs against a different JVM implementation in production, and you don't adequately guard against these issues, you may see errors there that you can't reproduce locally on a PC.

Using AtomicInteger would simplify this code and allow solving the memory visibility problem while removing synchronization.

Upvotes: 2

khelwood
khelwood

Reputation: 59096

thread.join() should be called after thread.start().

join() means "block until the thread finishes". That only makes sense after the thread has started.

Presumably your Thread.sleep() call actually waits long enough for all the threads (that you effectively didn't join) to finish. Without it, the threads might not all have finished when you print out the value of x.

Upvotes: 4

duffymo
duffymo

Reputation: 308733

You don't add synchronized to the run method. Each thread gets its own.

You have to synchronize the mutable, shared data. In your case, that's the integer x. You can synchronize get/set or use AtomicInteger.

Upvotes: 1

Related Questions