ReadWriteLock decorator, is this code thread safe?

Question

We're building cache stores in our app backed by memory, file, and remote services. Want to avoid explicit synchronization to keep the stores simple while using decorators for behavioral concerns like blocking.

Here's a simple cache, this is just an example!

import java.util.HashMap;

public class SimpleCache {

   private HashMap<String,Object> store;  
   private final BlockingCacheDecorator decorator; 

   public SimpleCache(){  
      store = new HashMap<String,Object>();
      decorator = new BlockingCacheDecorator(this);
   }

   //is NOT called directly, always uses decorator
   public Object get(String key){
      return store.get(key);
   }

   //is NOT called directly, always uses decorator 
   public void set(String key, Object value){
      store.put(key, value);
   }

   //is NOT called directly, always uses decorator
   public boolean isKeyStale(String key){
      return !(store.containsKey(key));
   }

   //is NOT called directly, always uses decorator
   public void refreshKey(String key){
      store.put(key, new Object());
   }

   public BlockingCacheDecorator getDecorator(){
      return decorator;
   }
}

getDecorator() returns a decorator providing synchronization for get() and set(), while isKeyStale() and refreshKey() allows the decorator to check if a key should be refreshed without knowing why or how. I got the idea for a synchronizing decorator from here.

import java.util.concurrent.locks.ReentrantReadWriteLock;

public class BlockingCacheDecorator {

   private SimpleCache delegate;
   private final ReentrantReadWriteLock lock;

   public BlockingCacheDecorator(SimpleCache cache){
      delegate = cache;
      lock = new ReentrantReadWriteLock();
   }

   public Object get(String key){      
      validateKey(key);         
      lockForReading();
      try{
         return delegate.get(key);
      }finally{ readUnlocked(); }
   }

   public void setKey(String key, Object value){
      lockForWriting();
      try{
         delegate.set(key,value);
      }finally{ writeUnlocked(); }      
   }

   protected void validateKey(String key){
      if(delegate.isKeyStale(key)){         
         try{
            lockForWriting();
            if(delegate.isKeyStale(key))
               delegate.refreshKey(key);
         }finally{ writeUnlocked(); }
      }
   }

   protected void lockForReading(){
      lock.readLock().lock();
   }   
   protected void readUnlocked(){
      lock.readLock().unlock();
   }   
   protected void lockForWriting(){
      lock.writeLock().lock();
   }   
   protected void writeUnlocked(){
      lock.writeLock().unlock();
   }  
}

Questions:

• Assuming SimpleCache is only ever used via its decorator, is the code thread-safe?
• Is it bad practice for ReadWriteLock to be declared outside the class being synchronized? SimpleCache.getDecorator() ensures a 1-to-1 mapping between cache and decorator instances, so I'm assuming this is ok.

Answer 1

• Is this code thread-safe?

Yes. Assuming that the instance of the decorated SimpleCache is not passed about.

• Is it bad practice for ReadWriteLock to be declared outside the class being synchronized? SimpleCache.getDecorator() ensures a 1-to-1 mapping between cache and decorator instances, so I'm assuming this is ok.

No. Although it is also worth noting that as discussed in comments, BlockingCacheDecorator would usually implement a Cache interface.

Answer 2

In its current form the code is trivially non-threadsafe, as there's nothing preventing a caller from calling methods of SimpleCache directly, or indeed pass the same SimpleCache instance to multiple decorators, causing even more mayhem.

If you promise never to do that, it is technically thread-safe, but we all know how much those promises are worth.

If the aim is to be able to use different implementations of underlying caches, I'd create a CacheFactory interface:

interface CacheFactory {
  Cache newCache();
}

A sample implementation of the factory:

class SimpleCacheFactory implements CacheFactory {
  private final String cacheName; //example cache parameter

  public SimpleCacheFactory( String cacheName ) {
    this.cacheName = cacheName;
  }

  public Cache newCache() {
    return new SimpleCache( cacheName );
  }
}

And finally your delegate class:

public class BlockingCacheDecorator {

   private final Cache delegate;
   private final ReentrantReadWriteLock lock;

   public BlockingCacheDecorator(CacheFactory factory){
     delegate = factory.newCache();
     lock = new ReentrantReadWriteLock();
   }

   //rest of the code stays the same
}

This way there's a much stronger guarantee that your Cache instances won't be inadvertently reused or accessed by an external agent. (That is, unless the factory is deliberately mis-implemented, but at least your intention not to reuse Cache instances is clear.)

Note: you can also use an anonymous inner class (or possibly a closure) to provide the factory implementation.