Java synchronization(object locking) not working as expected

Question

Please consider the below code.

import static java.lang.System.out;

public class Task
{
    public Integer k =  new Integer(10) ;

    public Task()
    {
        out.println(k + " constructor of Task : " + Thread.currentThread().getName());
    }
}

import static java.lang.System.out;

public class Executor2 implements Runnable
{
    private Task task;
    public Executor2(Task t)
    {
        out.println("constructor of Executor2 : " + Thread.currentThread().getName());
        task = t;
    }

    @Override
    public void run()
    {
        synchronized(task.k)
        {
            task.k = 88;
            out.println("changed value of task.k to : " + task.k + " " + Thread.currentThread().getName());
            try
            {
                out.println("sleeping");
                Thread.sleep(5000);
            }
            catch (InterruptedException ex)
            {
                ex.printStackTrace(out);
            }
            out.println("done");
        }
    }
}

import static java.lang.System.out;

public class Executor3 implements Runnable
{
    private Task task;
    public Executor3(Task t)
    {
        out.println("constructor of Executor3 : " + Thread.currentThread().getName());
        task = t;
    }

    @Override
    public void run()
    {
        synchronized(task.k)
        {
          task.k = 888;
          out.println("changed value of task.k to : " + task.k + " " + Thread.currentThread().getName());
        }
    }
}
------------------------------------------------------------
public class Main
{
    public static void main(String[] args)
    {
        Task task = new Task();

        Executor2 executor2 = new Executor2(task);
        Thread thread2 = new Thread(executor2);
        thread2.start();

        Executor3 executor3 = new Executor3(task);
        Thread thread3 = new Thread(executor3);
        thread3.start();
    }
}

Below is the output of the program.

10 constructor of Task : main
constructor of Executor2 : main
constructor of Executor3 : main
changed value of task.k to : 88 Thread-0
sleeping
changed value of task.k to : 888 Thread-1
done

The surprising thing here is output line : changed value of task.k to : 888 Thread-1 which is not expected to be printed before output line : done. How come the lock on Integer object is released before sleep duration has passed?

Thanks.

possible duplicate of http://stackoverflow.com/questions/16280699/synchronized-block-for-an-integer-object — Radiodef, May 21 '15 at 20:36
Bear in mind that even though you started thread2 before thread3, it is possible for thread3's `run` method to be invoked first. They might or might not attempt to synchronize on the same object, since each changes task.k. — VGR, May 21 '15 at 20:57

score 2 · Accepted Answer · answered May 21 '15 at 21:38

2

    synchronized(task.k)
    {
      task.k = 888;

Changing the object you're synchronizing on kind of defeats the point of the synchronization. You then try to print the new object while you're still synchronized on the old one. Don't replace objects threads are synchronizing on!

answered May 21 '15 at 21:38

David Schwartz

179,497
17
214
278

Interesting. Isn't the synchronised lock implemented Using the object's monitor lock which in turn is used for protecting exclusive access to the object's state. So if the state is changed after holding the lock, why is there an issue? – The Roy Mar 06 '16 at 02:19
1

@DROY It's not the object's state that's changed, the object is replaced with a whole new object. Remember, `Integer` is immutable, so if it has a different value, it's a different object. – David Schwartz Mar 06 '16 at 03:11
Got it. I overlooked the specific implementation used here. But if I had used a mutable data member for lock and changed the state, it should have been ok? – The Roy Mar 06 '16 at 03:13
@DROY Yes. You got it. – David Schwartz Mar 06 '16 at 03:16

score 1 · Answer 2 · answered May 21 '15 at 21:45

Extending what David Schwartz said: In almost all cases, if you're going to write this:

synchronized (foo) {
    ...
}

Then the variable that refers to the lock object should be a final field:

class MyClass {

    final Object foo = new Object();
    ...
    void someMethod(...) {
        synchronized (foo) {
            ...
        }
    }
    ...
}

That will prevent you from making the same mistake in the future.

score 0 · Answer 3 · answered Mar 06 '16 at 01:45

Agree with above. Also while using private lock, one pitfall is not to lock on string literals - String literals are shared resource.

private final String lock = “xx”;

private final String lock = new String(“xxx”);

The second locking is fine.

Java synchronization(object locking) not working as expected

3 Answers3