Is this broken double checked locking?

Question

Checkstyle reports this code as "The double-checked locking idiom is broken", but I don't think that my code actually is affected by the problems with double-checked locking.

The code is supposed to create a row in a database if a row with that id doesn't exist. It runs in a multi-threaded environment and I want to avoid the primary-key-exists SQL-exceptions.

The pseudo-code:

private void createRow(int id) {
  Row row = dao().fetch(id);
  if (row == null) {
     synchronized (TestClass.class) {
        row = dao().fetch(id);
        if (row == null) {
           dao().create(id);
        }
     }
  }
}

I can agree that it looks like double-checked locking, but I am not using static variables and the code in fetch() and create() is probably too complex to be inlined and put out of order.

Am I wrong or checkstyle? :)

Answer 1

Assuming you want that innermost line to read:

row = dao().create(id);

It's not a classic double-checked lock problem assuming dao().fetch is properly mutexed from the create method.

Edit: (code was updated)

The classic problem of a double-checked lock is having a value assigned before initialization occurs where two threads are accessing the same value.

Assuming the DAO is properly synchronized and will not return a partially initialized value, this doesn't suffer from the flaws of the double-checked lock idiom.

Answer 2

I think in this case, checkstyle is correct. In your code as presented, consider what would happen if two threads both had row == null at the entry to the synchronized block. Thread A would enter the block, and insert the new row. Then after thread A exits the block, thread B would enter the block (because it doesn't know what just happened), and try to insert the same new row again.

I see you just changed the code and added a pretty important missing line in there. In the new code, you might be able to get away with that, since two threads won't be relying on changes to a shared (static) variable. But you might be better off seeing if your DBMS supports a statement such as INSERT OR UPDATE.

Another good reason to delegate this functionality to the DBMS is if you ever need to deploy more than one application server. Since synchronized blocks don't work across machines, you will have to do something else in that case anyway.

Answer 3

Sorry, I forgot the extra fetch() right after the synchronization row.

Code is more correct now.

Answer 4

If you're tempted to write code like this, consider:

• Since Java 1.4, synchronizing methods has become pretty cheap. It's not free but the runtime really doesn't suffer that much that it's worthwhile to risk data corruption.

• Since Java 1.5, you have the Atomic* classes which allow you to read and set fields in an atomic way. Unfortunately, they don't solve your problem. Why they didn't add AtomicCachedReference or something (which would call an overridable method when get() is called and the current value == null) is beyond me.

• Try ehcache. It allows you to set up a cache (i.e. and object which allows you to call code if a key is not contained in a map). This is usually what you want and the caches really solve your problem (and all those other problems which you didn't know they even existed).

Answer 5

As others have pointed out, this code will do what you intend as is, but only under a strict set of non-obvious assumptions:

1. The Java code is non-clustered (see @Greg H's answer)
2. The "row" reference is only being checked for null in the first line, before the synchronization block.

The reason the double-checked locking idiom is broken (per section 16.2.4 of Java Concurrency in Practice) is that it's possible for a thread running this method to see a non-null but improperly initialized reference to "row", before entering the synchronized block (unless "dao" provides proper synchronization). If your method were doing anything with "row" other than checking that it's null or not, it would be broken. As it stands, it is probably okay but very fragile - personally I wouldn't be comfortable committing this code if I thought there were even a remote chance that some other developer at some later time might modify the method without understanding the subtleties of DCL.