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? :)

Wrand

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.

Be

The assumption that DAO is properly synchronized is CRITICAL. – Runnel

The problem with this assumption is that the DAO has no idea what you expect from it and the next code chance might ruin your hopes ... Therefore, I think this answer is wrong and dangerous! – Askins

Do you mean to say that the DAO should be expected to return object instances that are not initialized? Why would it matter if there was a lock if it can return objects in a bad state anyway? – Be

@Dustin, it's not that the DAO would purposely return uninitialized objects, but if the DAO does not provide proper synchronization then it's possible that a returned object could APPEAR uninitialized from the POV of other threads. – Mutual

Only one thread can see the object returned by the DAO as it's happening within that thread on the stack. I don't see how there could be a bug here that wouldn't be here without the sync. – Be

@Be we may be talking about different things. I posted my own answer below where I elaborated on what I'm cautioning about. The DAO could return a value that appears partially initialized, but since the example is only checking "row" for null I think it will work as is. – Mutual

@Be in your previous comment you're assuming that a different instance is returned by dao.fetch() each time. Maybe that's a sensible assumption, but we don't know from the info given. It is possible the DAO is not thread-safe and perhaps caches row instances for a period of time. – Mutual

Scott, that's a valid point, but that still wouldn't be a bug in this code, and this code couldn't really defend against it. This code might suggest that the DAO needs review, but it's not something broken here. – Be

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.

Pirouette

Yeah, the code is running in an EJB-container so I know I'm not actually allowed to use synchronization since it will fail when clustered. I will check to see if DB2 has any "insert or update"-function I can use. – Wrand

So then is the worst case that the synchronization fails and there are two attempts at an insert (and one fails)? Perhaps it's best just to not even bother with the synchronization and just catch the insertion error when the race occurs. – Be

Yes, that would be a simple solution. The problem is that our internal frameworks catches and logs the SQLException before my application has the opportunity to do so. So I can't supress the exception from being logged and the logs are monitored by surveillance software. – Wrand

Hmm. I'd be concerned about have systems in place that cause code to become more complex and buggy. Sometimes exceptions...aren't. – Be

Answer 3

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).

Askins

Answer 4

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.

Mutual