Following up on my previous post, this is the second installment of Java Concurrency Bugs. In this post, we’ll look at bugs that come from synchronizing on the wrong object.

#1: Don’t synchronize on an object you’re changing [source:java]

synchronized(foo) {

foo = …

}

[/source]

If you do this, then other threads will be synchronizing on a different object than the thread modifying foo. And if those threads are synchronizing on different objects, then you’re getting no mutual exclusion, which presumably was your intention.

#2: Don’t synchronize on a String literal [source:java]

private static final String LOCK = “LOCK”;

…

synchronized(LOCK) {

….

}

[/source]

This looks quite harmless and it may even work fine, but it’s also dangerous. All string literals are interned, so any other component or library in the JVM locking on another (apparently different) string literal “LOCK” will be sharing a lock with this code. This problem does happen in the wild.

If you need an object to lock on and you don’t have any other pre-existing object that makes sense, it’s recommended to just use a new Object().

#3: Don’t synchronize on auto-boxed values

The JLS requires that certain ranges of literals always return the same instance when boxed. Thus locking on a boxed Integer(1) (not a new Integer(1)) will be locking against the same boxed Integer anyone else in the JVM could be using. This is effectively the exact same scenario as #2 – an innocuous-looking lock object could actually be the exact same instance used elsewhere, thus sharing a lock that should be separate.

#4: Don’t synchronize on null

This should be a no-brainer but it can happen. This one’s easy to find though since you’ll get an NPE.

#5: Don’t synchronize on a Lock object

It’s not that you can’t do this but it’s more that it’s probably not what you intended. If you’re synchronizing on a ReentrantLock instance, you probably really want to be calling Lock.lock() instead.

#6: Don’t synchronize on getClass()

In an inheritance hierarchy, it’s possible to fall into the trap of calling getClass() at one point in the hierarchy and believing that this means every class will be synchronizing on the same lock. But if a lower class in the hierarchy runs across this code, you’ll actually be locking on a different lock. [source:java]

// BAD

synchronized(getClass()) {

…

}

[/source]

Generally, you probably want something like this instead: [source:java]

synchronized(Foo.class) {

…

}

[/source]

#7: Be careful locking on a thread-safe object with encapsulated locking

It can be tempting to believe that locking on an object that is thread-safe will allow you to participate in the same lock. And sometimes you’ll be right. But sometimes you won’t. Unfortunately, the only way to tell is to read the docs and the code.

An example where this will work are the synchronized classes in java.util like Vector and Hashtable. These collections lock internally on the “this” instance itself, thus allowing you to participate externally in that lock if you want. But a class like ConcurrentHashMap does internal locking on objects that are not exposed externally, so it’s not possible to participate in that lock. It’s important to know which situation you’re dealing – if you’re writing a class with encapsulated locking, this is a great thing to document.

Hope that was helpful. If I missed something, let me know…