Why is this code not thread safe?

Question

In code snippet below, declaring the doThings() method as static would make the class thread-safe. Is the reason for this that if multiple TestSeven threads are started and since x is a static variable a race condition could occur ?

public class TestSeven extends Thread{

    private static int x;

    public synchronized void doThings(){
        int current = x;
        current++;
        x = current;
    }

    public void run(){
        doThings();
    }

    public static void main(String args[]){
        TestSeven t = new TestSeven();
        Thread thread = new Thread(t);
        thread.start();
    }
}

Stridulous

Btw you pass a TestSeven which is a Thread as an argument to the Thread constructor. This works because Thread IS-A Runnable, but is not recommended, you'd better make TestSeven implement Runnable – Clapp

Answer 1

Yes, exactly. The synchronized nature of doThings only stops it from being called by multiple threads concurrently on the same instance. The variable x is shared on a global basis, not on a per-instance basis, therefore it's unsafe.

In real world terms, think of it as a bathroom with several doors - someone can open one door and then lock it, but that doesn't stop someone else from coming in via a different door...

Kuntz

+1: locking on the current thread, which is the case here, is almost always pointless. – Mientao

Answer 2

I think if the method is not static, each TestSeven object would synchronize using its own lock - so there will be one thread per lock, and none of them will have to wait for another thread. If the method is declared static, I seem to recall they lock on the corresponding Class object.

Coated

Answer 3

Just to add that if you declare method doThings static, it will synchronize on the class lock rather than instance lock so then it will be bullet-proof.

Koa

Answer 4

yes. Race condition could occur in this. As you are making the method synchronized not your variable. So according to the definition of the race condition, one thread will read the value of variable while other in synchronized method can write it. So a race condition will be there.

Betaine

Answer 5

You synchronize your code on this, meaning on that instance of TestSeven. x is static, so it will not be locked. That is why, from different instances, you can access the same x. In order to det a lock on that attribute, you'll need to synchronize on the class.

Oribelle