'this' is not the synchronization you're looking for

I'm no expert on multi-threaded applications. But I am quite good at juggling and making balloon animals. So if this post about Java synchronization strikes you as amateur, misguided, or wrong, then you have every right to call me a clown in the comments. That's what they're there for.

I suspect I'm not unique among Java developers in how I was introduced to concurrency issues. It started with a bug report, this time from some terribly misguided attempt at using threads to increase performance. Ugh. I worked out the easiest solution, which was to add method synchronization. It worked, that's good, so what's the issue? Liveness is the issue. Liveness and information hiding.

Consider this simple example of a class with a synchronized method:

class Counter {
 private int count = 0

 public synchronized void countUp() {
   (1..10000).each {
     count++
   }
 }
}

In Groovy the statement (1..10000).each {...} will perform something 10,000 times, so this method atomically increases the counter to 10000. (Also, the Java semi-colons aren't needed). So what's this about liveness? Consider the naive usage by two different threads:

def counter = new Counter()

//start a 2nd thread that synchronizes on counter
new Thread(
 {
   synchronized(counter) {
     //pretend to do some work
     Thread.sleep(5000);
   }
 }).start()

//try to count up on this thread
counter.countUp()

The problem is that the synchronized method from the first example and the synchronized statement in the second example are using the same lock (or "monitor"). If the worker thread that sleeps for 5 seconds acquires the lock first then it will starve the thread trying to call count Up( ). Sometimes the code here completes in 1 second, and sometimes it takes 6. Probably not the type of error you wanted to expose when you originally added the synchronized keyword. So how does this work?

A synchronized method uses the lock associated with the "this" reference. So any synchronized method:

synchronized void foo() {
 //do something 
};

Can just as equally be written like this:

void foo() {
 synchronized (this) {
   //do something
 }
};

Using this approach it becomes pretty obvious that anyone with a reference to the object could create a synchronized block on that object, and thus interfere with the liveness of the method call. Any time you use method synchronization you are exposing your synchronization policy publicly. Is how your object is synchronized really something you want publicly visible? I'd say not. This is surely an implementation detail. And just like you don't expose object state by not using public fields, I suggest you hide your synchronization policy by not using method synchronization. This is simple information hiding, but I overlooked it for years.

So what do I do now? I synchronize on an internal field like so:

class Counter {
 private int count = 0
 private Object lock = new Object()

 public void countUpSafely() {
   synchronized (lock) {
     (1..10000).each {
       count++
     }
   }
 }
}

The additional benefit is that your synchronization token can now carry with it an intent revealing name. Trust me, the maintenance programmers in the future will thank you. And if you ever want to find the usage of the object's lock, then the private variable is there for the searching. And that means you're free to change the implementation knowing that users of the code won't be affected.

I'm not sure who said it, but a reviewer of Brian Goetz's Java Concurrency in Practice said that, after reading the book, he was convinced that the only person in the world that really did understand concurrency was Goetz himself. It's an amazing book, worth several reads, and I echo the reviewer's thoughts entirely.

Now it's your turn to sound off. I'll give you a start. "Yo clown, stick to your circus skills... [insert concurrency edge case here]." And for this wanting to tinker with the code, the entire Groovy script is available here. Thanks!