0

I have a cache that gets loaded upfront with a large amount of data (by a background thread) and is unusable until full (it will also get reloaded every so often and be unusable during that load). I want the classes that use it to check a flag isLoaded() before accesses. I use a ReentrantReadWriteLock (I omit this in the code for simplicity) for access control like this:

public class Cache {

   private volatile boolean loaded = false; //starts false

   private static String[] cache;

   private static Lock readLock;
   private static Lock writeLock;

   public Object get(Object key) {
       if (!readLock.tryLock()) throw IllegalStateException(...);
       try {
           ... do some work
       } finally {
           readLock.unlock();
       }
   }

   // called by background thread
   private void loadFull() {
      loaded = false;
      writeLock.lock()
      try {
          cache = new String[];
          ... fill cache
      } finally {
          writeLock.unlock();
          loaded = true;
      }
   }
....
}

Now in my other class I have a block like this:

if (cache.isLoaded()) {
    try {
      Object x = cache.get(y);
    } catch (IllegalStateException iex) {
      // goto database for object
    }
} else {
    // goto database for object
}

Do I really need the try/catch? Is it ever possible that the flag will be set to false and the readLock try() will fail? Should I even bother with the flag and jut catch the Exception (since I basically do the same code if the Exception is thrown as if the flag is false). I just feel like I am doing something slightly wrong but I can't put my finger on it. Thanks.

Gandalf
  • 9,648
  • 8
  • 53
  • 88
  • You need to implement [Cache](http://en.wikipedia.org/wiki/Cache) and I didn't get we you are trying to solve [Producer/Consumer problem](http://en.wikipedia.org/wiki/Producer-consumer_problem). BTW, `IllegalStateException` isn't the appropriate one to be used. AFAI understand, empty cache is an ordinary thing in your application. – khachik Dec 07 '10 at 18:42
  • Is there any particular reason you can't use existing lightweight caching solutions? – Sanjay T. Sharma Dec 07 '10 at 18:47
  • A separate read and write lock? And your String[] cache gets created with new int[] :) – extraneon Dec 07 '10 at 18:52
  • @extraneon There is a new interface its called ReadWriteLock that has two methods readLock() and writeLock() a ReentrantReadWriteLock implements that interface – John Vint Dec 07 '10 at 18:54
  • @Gandalf This is separate to your question (why I am commenting) but there is no need to do the volatile write outside of the writeLock. You will get a performance hit rather then putting it write before you unlock the writeLock (the same memory guarantees would be in place) – John Vint Dec 07 '10 at 18:56
  • @extraneon: I guess that's done to improve concurrency in case you have a lot of reader threads and a few writer threads. You create a single `ReentrantReadWriteLock` and acquire separate read/write locks from the same object to synchronize your reads/writes. More at: http://download.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html – Sanjay T. Sharma Dec 07 '10 at 18:59

3 Answers3

2

Do I really need the try/catch? Is it ever possible that the flag will be set to false and the readLock try() will fail?

Yes, you need it. Between the time cache.isLoaded() and cache.get() are called, a writer can come in and get the write lock - in which case cache.isLoaded() will return true, but cache.get() will throw the exception.

Should I even bother with the flag and jut catch the Exception (since I basically do the same code if the Exception is thrown as if the flag is false).

From the code you have shown, the exception is thrown only in cases where the get fails to acquire the read lock. Acquisition of the read lock fails only if there is a concurrent writer at the time. isLoaded also returns false in precisely this scenario. So just relying on the exception would suffice. Also, consider creating a specialized CacheStaleException.

Binil Thomas
  • 13,699
  • 10
  • 57
  • 70
1

The tryLock will fail if some other thread has already acquired that lock. This typically means that an exception would be thrown if a client fails to acquire a lock due to high contention (multiple clients accessing the same cache). Is there any fallback strategy you have implemented in your client layer which deals with such situations?

Also, why static locks? I think that even though your cache is typically used in the application as a singleton, there is no need to limit its usability by making Locks static.

Sanjay T. Sharma
  • 22,857
  • 4
  • 59
  • 71
0

No, but to be honest your paradigm is confusing. Presumably it is expensive to go to the actual database and that is the purpose of the cache. In the case that the cache is being reloaded, is it not better to just wait until it is?

Assuming you really do want to go to the database if the read lock is not immediately available, I would do this:

   public Object get(Object key) {
       Object returnValue;
       if (readLock.tryLock()) {
           try {
               ... do some work
               returnValue = ...
           } finally {
               readLock.unlock();
           }
       } else {
           //go to database
           returnValue = ...
       }
       return returnValue;
   }
Tim Bender
  • 20,112
  • 2
  • 49
  • 58
  • I think the intent of the OP is to make the `Cache` class ignorant of what it is caching - hence the logic to fetch the data is kept outside of the `Cache` class. Nonetheless, I agree with Tim that keeping the retry logic at one place is better - probably by another class wrapping the `Cache`, and clients using that class. – Binil Thomas Dec 07 '10 at 19:18
  • That logic is handled outside the cache - I say "in another class" for the second block of code. The String[] cache is loaded all at once and takes a while, so if the readLock call fails that means the cache is empty and I don't load on demand. – Gandalf Dec 17 '10 at 19:42