Caching large object in multithreading environment

I'm having to dabble with caching and multithreading (thread per request), and I am an absolute beginner in that area, so any help would be appreciated.

My project requirements are:

• Cache one single large object that has ether interval refresh or refresh from user

• Because retrieving object data is very time consuming make it thread-safe

• When retrieving object data return "old data" until new data is available

• Optimize it

```
public enum DbCachedObject {

INSTANCE;

private final CountDownLatch initLock = new CountDownLatch(1);
private final Object refreshLock = new Object();
private final AtomicReference cachedInstance = new AtomicReference();
private final AtomicLong lastUpdate = new AtomicLong();
private volatile boolean refreshing;
private long cachePeriodMs = 1000L;

public CachedObject get() {

CachedObject o = cachedInstance.get();
if (o == null || isCacheOutdated()) {
updateCache();
if (o == null) {
awaitInit();
o = cachedInstance.get();
}
}
return o;
}

public void refresh() {
updateCache();
}

private boolean isCacheOutdated() {
return (System.currentTimeMillis() - lastUpdate.get() > cachePeriodMs);
}

private void updateCache() {

synchronized (refreshLock) {
// prevent users from refreshing while an update is already in progress
if (refreshing) {
return;
}
refreshing = true;
// prevent other threads from calling this method again
lastUpdate.set(System.currentTimeMillis());
}
new Thread() {
@Override
public void run() {
try {
cachedInstance.set(getFromDb());
// set the 'real' last update time
lastUpdate.set(System.currentTimeMillis());
initLock.countDown();
} finally {
// make sure refreshing is set to false, even in case of error
refreshing = false;
}

I surely dislike your mixing of various mean synchronization. Using synchronized and Atomic* and also volatile together looks very strange. However, it looks like you got it right (a deeper analysis would be needed).

private volatile boolean refreshing;

You need no volatile if you access it inside the synchronized block only.

private long cachePeriodMs = 1000L;

I'd suggest cachePeriodMillis. It's only slightly longer and there's a precedent.

I guess, I'd drop the latch, define a non-anonymous Runnable for getting from the DB and let the first interested thread execute it directly. The others would wait on the corresponding synchronized block's entry (and then leave immediately).

I'd probably also aggregate CachedObject and lastUpdate into one object as they belong together. This might be a problem w.r.t "prevent other threads from calling this method again", but it should be possible to eliminate the need for this.

This way I might get a simpler design which I could trust more.

StackExchange Code Review Q#97060, answer score: 3