synchronise on non-final object [duplicate]

Question

private volatile Object obj = new MyObject();
void foo() 
{
  synchronized(obj)
  {
    obj.doWork();
  }
}
void bar()
{
  synchronized(obj)
  {
    obj.doWork();
    obj = new MyObject(); // <<<< notice this line (call it line-x)
  }
}

Suppose at a certain point in time, a thread t_bar is executing bar(), and another one t_foo is executing foo, and that t_bar has just acquired obj, so t_foo is, in effect, waiting.

After the sync-block in bar is executed, foo will get to execute its sync-block, right? What value of obj would it see? The old one? Or the new one set in bar?

(I would hope that the new value is seen, that's the whole point of coding it that way, but I want to know if this is a 'safe' bet)

Straticulate

@javapirate: I find it VERY rude that you edited my post simply because YOU prefer your own K&R formatting style? I'm sorry, but I will have to reformat it back. – Straticulate

Sorry. I thought of getting ridding of lot of empty spaces. Go ahead! – Koester

One distinction that might help understanding is that objects or not final or not-final, the variable that hold references to them are. The lock is on the object, not on the variable. – Gutow

@MiserableVariable I understand that, but thanks for the clarification! – Straticulate

Answer 1

In the exact situation you described, yes, the read of obj inside foo's synchronized block will see the new value set by the previous bar's synchronized block.

The fun part is, it doesn't always happen in that exact situation. The program is not thread safe, for example, if immediately after bar() exits, the same threads invokes another bar(), while the foo thread is locking on the old object. The bar thread locks on the new object, so the two threads are executing concurrently, both executing obj.doWork() on the same new obj.

We can probably partially fix it by

// suppose this line happens-before foo()/bar() calls
MyObject obj = new MyObject();

void foo()
    while(true)
        MyObject tmp1 = obj;
        synchronized(tmp1)
            MyObject tmp2 = obj;
            if(tmp2==tmp1)
                tmp2.doWork();
                return;
            // else retry

this at least guarantees no current invocations of obj.doWork() on the same obj, since obj.doWork() can only occur in a synchronized block that locks the exact same obj

Cathicathie

How come foo is still locking the old object? After bar exits, obj will have had new value everywhere, yes? – Straticulate

@One No, foo will still hold a lock to the old object. See docs.oracle.com/javase/specs/jls/se7/html/jls-14.html#jls-14.19, or look at the answer to my question 2 edits ago (where I answered that question. – Myongmyopia

Ok, it holds the lock to the old object, that I understand. But it looks like any reference inside the sync-block will be referring to the new value ob obj... ? – Straticulate

the foo thread locked on the old object, then inside the block, it sees the new object, so work is done on the new object under the lock of old object. – Cathicathie

Answer 2

It will behave normally as if object reference was not changed internally. Reason being that test for lock on object will be done only once. So even if object changes internally, thread will keep waiting and behaviour will remian same as if object was same [unchanged].

I tried another thing. I placed a sleep statement just after new object was created and then started the next thread and as expected both threads started working simultaneously. See the code below.

public class ChangeLockObjectState {

    private volatile Object obj = new Object();

    void foo() {
        synchronized (obj) {
            try {
                System.out.println("inside foo");
                Thread.sleep(10000);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }
    }

    void bar() {
        synchronized (obj) {
            try {
                System.out.println("inside bar");
                Thread.sleep(5000);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }

            obj = new Object(); // <<<< notice this line (call it line-x)

            System.out.println("going out of  bar");

            try {

                Thread.sleep(5000);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }

            System.out.println("wait over");

        }
    }

    /**
     * @param args
     * @throws InterruptedException
     */
    public static void main(String[] args) throws InterruptedException {
        final ChangeLockObjectState test = new ChangeLockObjectState();

        new Thread(new Runnable() {

            @Override
            public void run() {
                test.bar();

            }
        }).start();

        Thread.sleep(6000);

        new Thread(new Runnable() {

            @Override
            public void run() {
                test.foo();

            }
        }).start();

    }

}

Cyclotron

Answer 3

This is unsafe and broken. Changing the object you lock on doesn't work.

When a thread tries to enter a synchronized block it first has to evaluate the expression in parens in order to figure out what lock it needs. If the lock changes after that, the thread doesn't have any way of knowing that, it eventually acquires the old lock and enters the synchronized block. At that point it sees the object and evaluates that, getting the new reference, and calls the method on it with the old (now irrelevant) lock, and without holding the new lock, even though some other thread might have the new lock held and could be executing the method on the same object concurrently.

Alessandro

Answer 4

The new value is shown. And it works even without the making obj volatile. That's because the sychronization is still hold on the old object and provides visibility to the new value once the waiting thread (t_foo) gets inside. Here is the test:

public class Main3 {
    private MyObject obj = new MyObject(1);

    void foo()
    {
        synchronized(obj)
        {
            System.out.println(obj.number);
            obj.doWork();
        }
    }

    void bar()
    {
        synchronized(obj)
        {
            System.out.println(obj.number);

            obj.doWork();

            //force the foo thread to wait at the synchronization point
            for(int i = 0; i < 1000000000l; i++);

            obj = new MyObject(2); // <<<< notice this line (call it line-x)
        }
    }

    public static void main(String[] args) throws InterruptedException {
        final Main3 m3 = new Main3();

        Thread t1 = new Thread( new Runnable() {
            @Override
            public void run() {
                m3.bar();
            }
        });

        Thread t2 = new Thread(new Runnable() {
            @Override
            public void run() {
                m3.foo();
            }
        });

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

class MyObject {
    int number;

    public MyObject(int number) {
        this.number = number;
    }

    public void doWork() {
    }
}

Drysalter

Answer 5

The new value of obj will be read.

From the standard's section on Happens before:

A write to a volatile field (§8.3.1.4) happens-before every subsequent read of that field.

From definition of shared variable:

All instance fields, static fields, and array elements are stored in heap memory. In this chapter, we use the term variable to refer to both fields and array elements. Local variables (§14.4), formal method parameters (§8.4.1), and exception handler parameters (§14.20) are never shared between threads and are unaffected by the memory model.

The read of obj inside of the synchronized block is separate from the initial evaluation of the expression obj to determine the which object's build-in monitor to lock. The reassignment of obj will happen before the first read, but not the second. Since obj is a volatile field, that second read must see the updated value of obj.

Myongmyopia

how is it linked to this question? – Cyclotron

Oops, you're right. Answered the wrong question :-( – Myongmyopia

Changed to answer to correct question. – Myongmyopia