Read lock while Writing

Question

I need help in understanding the below code :

private Predicate composedPredicate = null;

public boolean evaluate(Task taskData) {
        boolean isReadLock = false;
        try{
            rwl.readLock().lock();
            isReadLock = true;
            if (composedPredicate == null) {
                rwl.readLock().unlock();
                isReadLock = false;
                rwl.writeLock().lock();
                if (composedPredicate == null) {
                    //write to the "composedPredicate" object
                }
            }
        }finally {
            if (isReadLock) {
                rwl.readLock().unlock();
            }else{
                rwl.writeLock().unlock();
            }
        }
        return composedPredicate.test(taskData);
    }

What will happen if we don't use Read Locks in the above code? Like :

public boolean evaluate(Task taskData) {
        //boolean isReadLock = false;
        try{
            //rwl.readLock().lock();
            //isReadLock = true;
            if (composedPredicate == null) {
                //rwl.readLock().unlock();
                //isReadLock = false;
                rwl.writeLock().lock();
                if (composedPredicate == null) {
                    //write to the "composedPredicate" object
                }
            }
        }finally {
            rwl.writeLock().unlock();
        }
        return composedPredicate.test(taskData);
    }

1. Do we really need Read locks while we are only writing the data?
2. What is the difference between the above two codes?
3. Should we use Read locks even for accessing the object(composedPredicate) for null check?

Answer 1

The first code that you posted is a correct implementation of the double-checked locking approach in Java using a read/write lock.

Your second implementation without a read-lock is broken. The memory model allows writes to be reordering from the perspective of another thread seeing the result of the writes to memory.

What could happen is that you could be using a not-fully initialized instance of Predicate in the thread that is reading it.

Example with your code:

We have thread A and B both running evaluate and composedPredicate is null initially.

1. A: sees composedPredicate is null
2. A: write-locks
3. A: creates an instance of an implementation of Predicate
4. A: initializes this instance in the constructor
5. A: assigns the instance to the the shared variable composedPredicate
6. A: unlocks the write lock

1. B: sees composedPredicate is not null
2. B: runs composedPredicate.test(taskData);
3. HOWEVER, the compiler, the JVM, or the hardware architecture of your system reordered steps 4 and 5 of thread A, and assigned the address of the Predicate instance of the shared field before it was initialized (this is allowed by the Java Memory model)
4. composedPredicate.test(taskData); is run using a not-fully initialized instance and your code has random unexpected errors in production resulting in great losses to your company (potentially that happens .. depends on the system that you're building)

Whether or not the reordering of step 4 and 5 happens depends on many factors. Maybe it only does under heavy system load. It may not happen at all on your OS, hardware, version of JVM, etc. (But on the next version of the JVM, your OS, or when you move your application to a different physical machine, it may suddenly start happening)

Bad idea.

Answer 2

This code is simular to an old 'Singleton-pattern' wich makes use of the synchronozed blocks. E.g.

class Singleton
{
    volatile Singleton s;

    public static Singleton instance()
    {
        if(s == null)
        {
             synchronized(Singleton.class)
             {
                  if(s == null)
                      s = new Singleton();
             }
         }
         return s;
    }
}

Notice the double 'null-check' where only the second one is synchronozed. The reason for doing the first 'null-check' is to prevent the blocking of threads if the instance() method is called (because when not null, it can proceed without synchronization).

Your first code is doing the same. First it checks if there is something assigned to composedPredicate. And if that isnt the case, only than will it aquire a writingLock (wich blocks all other Thread oposed to readLocks, which only blocks writeLocks).

The main difference with the 'singleton-pattern' and your code is that your value can be reassignes. This can only happen safly if it makes sure nobody is reading the value during modification. By removing the readLock you basically create a possibility that a thread may get undefined results (if not a crash) when accessing the composedPredicate while another Thread is modifying that same field.

So to answer your questions: 1. You dont need a readLock for writing, only a writeLock (wich will block all other Threads whonare trying to lock). But in this design-pattern you cannot leave it out. 2. & 3. See explanation above.

Hope this was enough to get a grasp of this pattern.

EDIT As commented by Erwin Bolwidt , the above pattern is considered broken (without the 'volatile' keyword) due to compiler/CPU code optimization (where read/write actions may happen out of order). In the linked blog there are examples for alternatives/fixes for this problem. It turns out the 'volatile' keyword creates a barier which disallows reordering of read and write operations by either the compiler or CPU optimization, and thus 'fixes' the 'double-checked-locking' pattern described above.