Sunday, May 11, 2008

Which Lock is Which?

I was at JavaOne last week, and attended Bill Pugh's Defective Java talk. Bill had an important point about what kind of locks you want to be using and what kind you don't, and that point is worth repeating.

Bill is, with Dave Hovemeyer, the guy behind FindBugs, which everyone should be using religiously. He was also my graduate advisor; we worked together on the new Java Memory Model. If there are two things Bill knows, they are concurrency and bug patterns.

There is a tremendously useful defensive locking discipline that I use all of the time, and recommend that other people use, too:

class Foo {
private final Object lock = new Object();
private Bar lockProtectsMe;
...
public void blah() {
synchronized (lock) {
// accesses LockProtectsMe...
...
}
}
}

This has the following benefits:
  • If you try to acquire the lock on the Foo object instead of the lock, you run the risk that some code outside the class obtains that lock. Perhaps forever.

  • Using the Foo object also has the downside that it is coarse-grained locking; if you use that object to protect some other field called, say, lockProtectsMeToo, then you won't be able to access both lockProtectsMe and lockProtectsMeToo at the same time, even if they have nothing to do with each other.

  • If you try to acquire the lock on the lockProtectsMe object, you run the risk that someone changes that field. Since you acquire locks on objects, not on fields, if this happens, you will be acquiring a completely different lock.

There are reasons not to use this pattern, of course, and if you are curious, I can fill you in (ask in the comments), but that's not really the point of this post.

Anyway, Bill noticed that quite a few people were using the following pattern to lock objects:

private final String myString = "LOCK";
private Object lockProtectsMe;

...
public void blah() {
synchronized (myString) {
// accesses lockProtectsMe
...
}
}

This is awfully similar to the pattern above, but should never be used. Why? It turns out that when you make a String final and declare it in the source code, that String will be interned. This means that there will be one canonical instance of that String for the entire JVM.

Now, if you are likely to use this pattern in one place, you are likely to use this pattern in lots of places...

class Bar {
private final String lock = "LOCK";
...
}

class Baz {
private final String lock = "LOCK";
...
}

All of these lock fields will be interned. They will all share one common instance. If you run the code:

class Bar {
static final String lock = "LOCK";
}

public class Baz {
static final String lock = "LOCK";
public static void main(String [] args) {
System.out.println(Baz.lock == Bar.lock);
}
}

It will print out true, because, as a result of the interning, those will be the same object. Since we synchronize on objects, not fields, any code that synchronizes on them will be using the same lock. This is potentially disastrous; imagine if you had this code somewhere:

Thread 1:

synchronized (a) {
synchronized (Baz.lock) {
// ...
}
}

Thread 2:

synchronized (Bar.lock) {
synchronized (a) {
// ...
}
}

We've now turned perfectly inoffensive code into a potential deadlock. And that's an ugly one, because it isn't going to be obvious why it happened.

You should be equally careful with synchronizing on autoboxed / unboxed types. The reason for this is similar, and left as an exercise for the reader.

11 comments:

Unknown said...

The bit on final String declarations resulting in canonical internal objects and the implications for using such for locking purposes is particularly interesting.

Thanks for sharing.

Bharath said...

Jeremy,

When would you not want to use such a defensive locking pattern? Thanks.

-Bharath

Jeremy Manson said...

When would you not want to use such a defensive locking pattern? Thanks.

The reason that always leaps to my mind is that you sometimes want to allow client code to acquire the lock. Easy examples are the classes that you get by calling Collections.synchronizedFoo(). They are all locked on "this". The reason you do this is so that you can have external code that composes threadsafe actions:

// This method takes maps that come out of
// Collections.synchronizedMap()
public void putIfAbsent(Map<K,V> m,
    K key, V value) {
  synchronized(m) {
    if (!m.containsKey(key)) {
      m.put(key, value);
    }
  }
}

// later...

Map m = Collections.synchronizedMap(
    new Map());
putIfAbsent(m, key, value);

The putIfAbsent is thread-safe.

konberg said...

Jeremy, this is excellent. Sorry I missed the talk.

Regarding exposing a lock, specifically through C.sF, I thought making 'this' the lock was undocumented and incidental.

One possibility is that an object returned from synchronizedFoo would have a method, getLock(), but it's too late for that.

What would you think of adding getLock() to Object? (Yes, it's too late, I mean if you had to restart concurrency for Java.)

Unknown said...

Hi, nice article pointing, i just have one question. When i for example use

public void writeObjectToOS(String usermail, ArcticProtocol apo) throws IOException {
ObjectOutputStream oos = userOutputs.get(usermail);//map containing all users and their outputstreams
synchronized (oos) {
oos.writeObject(apo);
oos.flush();
}
}

what effect would it have to add synchronized modificator before this method declaration

Thanks

Jeremy Manson said...

Hi, nice article pointing, i just have one question. When i for example use

public void writeObjectToOS(String usermail, ArcticProtocol apo) throws IOException {
ObjectOutputStream oos = userOutputs.get(usermail);//map containing all users and their outputstreams
synchronized (oos) {
oos.writeObject(apo);
oos.flush();
}
}

what effect would it have to add synchronized modificator before this method declaration

Thanks

Hi Marek,

It would have the effect of acquiring a lock on the enclosing object for the duration of the invocation of your method.

I'm pretty sure that that isn't too helpful, though. I can't really tell you more without knowing more about your code.

Jeremy Manson said...



Regarding exposing a lock, specifically through C.sF, I thought making 'this' the lock was undocumented and incidental.

One possibility is that an object returned from synchronizedFoo would have a method, getLock(), but it's too late for that.

What would you think of adding getLock() to Object? (Yes, it's too late, I mean if you had to restart concurrency for Java.)


Gah. I wrote a nice long response to this, and my browser died. Briefly, then:

1) I didn't go over this at the talk, so at least you didn't miss it. :)

2) Making the map's lock object the lock for the map is the documented behavior for Collections.synchronizedMap. Fortunately! Otherwise, it would be impossible to iterate over the map safely. It gives an example of this in the javadoc.

3) If I were designing Java from scratch, I would remove the per-object lock, since it is unnecessary 99.999% of the time. I would tell everyone to use java.util.concurrent.ReentrantLock, and provide a way to hold a lock for a scoped duration (a la the synchronized block).

Danny said...

This is such a simple, yet extremely effective, concept. So many people say "Oh, it works just as well when you synchronize on the method." It works just as well until other developers start synchronizing on your object in way you never expected. :)

How come the designers created the lock-per-object design? Was it originally more efficient and convenient this way?

Jeremy Manson said...

This is such a simple, yet extremely effective, concept. So many people say "Oh, it works just as well when you synchronize on the method." It works just as well until other developers start synchronizing on your object in way you never expected. :)

How come the designers created the lock-per-object design? Was it originally more efficient and convenient this way?


It wasn't really more efficient; it also wasn't more convenient for the Java designers (it was just more stuff to design and build). The general principle they were following was that everything would be thread-aware and thread-safe. The problem was that no such language had ever been designed before. So they put in features that they thought would be useful.

Most of the version 1 features (the original memory model, the oversynchronized collection classes, the built-in locks) turned out not to be quite as useful as they had hoped, but the experience helped them gather enough information to make the concurrency overhaul in JDK5 into something that was really a thing of beauty.

Unknown said...

Hi, i was just writing little article about singleton pattern for locals and i put there some synchronization markings. Is it ok with jvm memory to use volatile double chekced locking in java5>?(http://blog.shakuras.biz)
As you say concurrency should be thing of beauty in j5 ;)

Jeremy Manson said...

Hi, i was just writing little article about singleton pattern for locals and i put there some synchronization markings. Is it ok with jvm memory to use volatile double chekced locking in java5>?(http://blog.shakuras.biz)
As you say concurrency should be thing of beauty in j5 ;)



Hi Zeratul,

I can't load the page you pointed me to, but I get this question enough that I wrote a quick blog post about it.