Integer counter = 0;
.
.
.
synchronized(counter) {
counter++;
// do other work under sync
}
I thought that the compiler might be accepting this as a valid use of autoboxing, as in
Integer counter = 0;
synchronized(counter) {
int i = counter.intValue();
i++;
// do other work under sync
}
in other words the value is pulled out of the Integer and held in a temporary variable where it is incremented and then thrown away.
This turned out not to be the case at all. The compiler actually creates code that will increment the value like this but it affects counter just as if this were an "int" field! So my coworker was right - this "++" is incrementing the counter like he wanted it to do.
But now there is something else wrong with the code! Java Integers are immutable, and that "++" is assigning a new Integer to the counter variable. This makes his synchronized(counter)statement useless in protecting anything but the counter++. Once that's finished there is a new object in counter. If one thread synchronized on Integer(0) the counter++ would change it to Integer(1) Another thread could then enter the synchronized block holding a lock on Integer(1) while the original thread continued to lock on Integer(0).
t1: synchronized(Integer(0)) {
t1: counter++ // in other words, counter=Integer(1)
t2: synchronized(Integer(1)) {
t1 & t2: // do other work under sync
What else is wrong with this? What about the objects we're synchronizing on? I wrote a short program to look at the Integers generated in code such as this.
public class incInt {
public static void main(String args[]) {
Integer i = 0;
System.out.println("i="+i + " hash="+System.identityHashCode(i));
i++;
System.out.println("i="+i + " hash="+System.identityHashCode(i));
i++;
System.out.println("i="+i + " hash="+System.identityHashCode(i));
System.out.println("resetting to zero");
i = 0;
System.out.println("i="+i + " hash="+System.identityHashCode(i));
i++;
System.out.println("i="+i + " hash="+System.identityHashCode(i));
i++;
System.out.println("i="+i + " hash="+System.identityHashCode(i));
}
}
The result of running this with Oracle's JRE 1.7.0_5 shows that there are canonical values for autoboxed zero, one and two.
> java incInt
i=0 hash=4991049
i=1 hash=32043680
i=2 hash=9499036
resetting to zero
i=0 hash=4991049
i=1 hash=32043680
i=2 hash=9499036
Here's a blog post that claims that [-128,127] are cached by the JVM and used in autoboxing. It turns out that the post is right. I modified the test to print out the hashes for zero, one and two and they are the same objects
> java incInt
i=0 hash=31879808
i=1 hash=6770745
i=2 hash=12835244
resetting to zero
i=0 hash=31879808
i=1 hash=6770745
i=2 hash=12835244
Integer.valueOf(0)=31879808
Integer.valueOf(1)=6770745
Integer.valueOf(2)=12835244
Getting back to the code under review, this means that the synchronization is at least sometimes using a canonical object used by the whole JVM. Anything could sync on Integer.valueOf(0), causing this code to be affected by code running in other threads. All synchronization should be done on private state or by using well-tested concurrency utilities to avoid accidental conflicts and meddling.
