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

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.