Java synchronized out of order?

Question

I started testing out something tricky but ended being surprised by level 0...

public class Test implements Runnable
{
    Integer i = 0;

    public static void main(String[] args)
    {
        Test test = new Test();
        for (int j = 0; j < 100; ++j)
        {
            Thread t = new Thread(test);
            t.setName("" + j);
            t.start();
        }
    }

    @Override
    public void run()
    {
        synchronized (i)
        {
            System.out.println ("-->Entering synch thread " + Thread.currentThread().getName() + " i=" + ++i);
            System.out.flush();
            System.out.println ("   Synchronized, thread " + Thread.currentThread().getName() + " i=" + ++i);
            System.out.flush();
            try 
            {
                Thread.sleep (0); 
            }
            catch (InterruptedException e ) {}
            System.out.println ("<--Exiting synch thread " + Thread.currentThread().getName() + " i=" + ++i);
            System.out.flush();
        }
    }
}

I expected the output to be in order in the count, not thread name. But this is what I got instead:

-->Entering synch thread 0 i=1
-->Entering synch thread 3 i=3
-->Entering synch thread 4 i=4
    Synchronized, thread 4 i=5
-->Entering synch thread 5 i=6
<--Exiting synch thread 4 i=7
-->Entering synch thread 2 i=2
    Synchronized, thread 2 i=9
<--Exiting synch thread 2 i=10
    Synchronized, thread 5 i=8

How can this be? This is as simple as it gets. This is like being told Santa isn't true!

If I synchronized(this), it's in order as expected. So at least some sanity is in place. But, still, in this particular situation, why would synchronized(i) be insufficient?

I know, System.out.flush() isn't necessary but given the shock, I had to make sure.

After running it a few times, it's clear that threads have their local copy of i. Making i volatile doesn't solve the problem, though. This shouldn't be happening. If it's due to JVM optimization, then it's a bug.

I'm using JDK 1.7.0_21.

I'll be sulking in a corner in a fetal position until some kind soul solves this.

Answer 1

Though you have added an answer with explanation, I think it worth mentioning the reason of "new object being created"

It is actually because of auto-boxing/unboxing of Java causing the problem.

In brief, when you are doing

Integer i = ...;
++i;

You need to understand Integer does not allow ++ operator. It is the compiler that knows that i is an int wrapper (Integer), so that it automatically convert i to int (auto-unboxing) and convert it back to Integer (auto-boxing) when you are doing ++i.

i.e. What happened internally will be something like:

Integer i = ...;
int itmp = i.intValue();
++itmp;
i = Integer.valueOf(itmp);

Hence, every time you do ++i, it is pointing to a new Integer silently. Therefore, threads are synchronizing on different Integer object instances, which cause the problem.

---

And, the proper way to perform synchronization in your case, given that you were originally locking an instance member variable, it means you are aiming to lock base on the Test object instance. So the change is as easy as:

@Override
public void run()
{
    synchronized (this)
    {
        System.out.println ("-->Entering synch thread " + Thread.currentThread().getName() + " i=" + ++i);
        .....
    }
}

or even better:

@Override
public synchronized void run()
{
    System.out.println ("-->Entering synch thread " + Thread.currentThread().getName() + " i=" + ++i);
    .....
}

Answer 2

Sorry to bother you with the stupid question. I forgot Integer is immutable. Wanted to delete it but it looks like it actually could help a few...

So the moral of the story is

1. Don't lock on immutable objects unless you're sure it won't change, e.g., declared final. Immutable objects are newly created when modified. So you won't be locking on a shared object. This is exactly what happened here.

2. If you have to lock on something that will be modified, make very sure its reference is not modified, e.g., by ref assignment.

3. To make life simpler, try lock on final objects only so that you know its reference won't change.