4

I am implementing a simple cache with the cache stored as an AtomicReference.

private AtomicReference<Map<String, String>> cacheData;

The cache object should be populated (lazily) from a database table.

I provide a method to return the cache data to a caller, but if the data is null (ie. not loaded), then the code needs to load the data from the database. To avoid synchronized I thought of using the compareAndSet() method:

public Object getCacheData() {
  cacheData.compareAndSet(null, getDataFromDatabase()); // atomic reload only if data not set!
  return Collections.unmodifiableMap(cacheData.get());
}

Is it ok to use compareAndSet in this way ie. to involve a database call as part of the atomic action? Is it any better/worse than just synchronizing the method?

Many thanks for any advice..

MandyW
  • 1,117
  • 3
  • 14
  • 23

1 Answers1

5

You do not achieve expected behaviour. This expression:

cacheData.compareAndSet(null, getDataFromDatabase())

will always call getDataFromDatabase() first. This means that it doesn't matter if the data was cached or not. If it was, you still call the database, but discard the results. The cache is working, but the performance is equally poor.

Consider this instead:

if(cacheData.get() == null) {
    cacheData.compareAndSet(null, unmodifiableMap(getDataFromDatabase()));
}
return cacheData.get());

It's not perfect (still getDataFromDatabase() can be called multiple times at the beginning), but will work later as expected. Also I moved Collections.unmodifiableMap() earlier so that you don't have to wrap the same map over and over again.

Which brings us to even simpler implementation (no synchronized or AtomicReference needed):

private volatile Map<String, String> cacheData;

if(cacheData == null) {
  cacheData = unmodifiableMap(getDataFromDatabase());
}
return cacheData;
Tomasz Nurkiewicz
  • 334,321
  • 69
  • 703
  • 674
  • Thanks for the very fast reply Tomasz, I really appreciate it! OK great I understand now that is not a good pattern! Just out of interest, how did you know that getDataFromDatabase() would get called before the null check? I couldn't see this in the Javadoc for the method.. – MandyW Oct 20 '12 at 20:47
  • @MandyW: it's how Java works. Parameters are passed by value (always!) and they must be evaluated *before* we enter the method. In Scala you have *pass-by-name* which avoids this problem. – Tomasz Nurkiewicz Oct 20 '12 at 20:49
  • thanks for the alternative.. is this preferable to making getData() synchronized? I can't really see much performance advantage.. – MandyW Oct 20 '12 at 20:58
  • @MandyW: by saying you don't see performance advantage, have you measured it? `volatile` should normally be much faster than `synchronized`, especially under heavy concurrent load – Tomasz Nurkiewicz Oct 20 '12 at 21:03
  • so really I was thinking more about avoiding blocking all my calling threads if say the call to the database has some problem eg. waits around for a minute or so.. I assumed that making the whole method synchronized would mean no thread could make any progress.. by using your code snippet for the Atomic compareAndSet am I in any better position? Do the other threads get blocked waiting for the Atomic write to complete or do they all individually keep calling getDataFromDatabase() and then eventually (assuming db returned to normal) one thread will manage to set the cacheData successfully? – MandyW Oct 20 '12 at 21:19