current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

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<CachedObject> cachedInstance = new AtomicReference<CachedObject>();
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;
            }
        }
    }.start();
}

private boolean awaitInit() {

    boolean initialized = false;
    try {
        // assume cache-period is longer as the time it takes to create the cached object 
        initialized = initLock.await(cachePeriodMs, TimeUnit.MILLISECONDS);
    } catch (Exception e) {
        e.printStackTrace();
    }
    return initialized;
}

private CachedObject getFromDb() {
    // dummy call, no db is involved
    return new CachedObject();
}

public long getCachePeriodMs() {
    return cachePeriodMs;
}
}
share|improve this question

2 Answers 2

up vote 3 down vote accepted

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.

share|improve this answer
up vote 2 down vote

Maaartinus is right in his analysis of your code, the three-style locking is a problem. Volatile is always a red-flag for me. Inevitably when I see it used, it is a problem. There are very few places where volatile is actually reliable, and in each case volatile is OK, it can be replaced with an easier-to-understand alternative that is almost as fast. Unless you are a real expert (and I don't consider myself to be expert enough), you are probably don't need volatile.

Now, you have three states in your code:

  1. you can get the previously accessed instance from the cache
  2. there is no previously cached instance
  3. you want to refresh the instance

Now, this can all be solved with simple synchronization, and nothing more. I would wager that it will work just fine, and you will not notice any performance problems. In the very unlikely even that your circumstances are exceptionally high-frequency/volume calls from many threads, then maybe you will need to implement a more complicated mix of atomic and synchronization strategies, or perhaps even a lock-free strategy, but I would seriously, seriously doubt that.

OK, so, about a simple cache.... how about:

public enum DbCachedObject {

    INSTANCE;

    public static final class CachedObject {
    }

    private final Object refreshLock = new Object();

    private long expireCache = 0L;
    private CachedObject cachedInstance = null;
    private boolean refreshing;
    private final long cachePeriodMs = 1000L;

    public CachedObject get() throws InterruptedException {

        synchronized (refreshLock) {
            while (cachedInstance == null) {
                updateCache();
                refreshLock.wait();
            }
            if (System.currentTimeMillis() > expireCache) {
                updateCache();
            }
            return cachedInstance;
        }

    }

    public void refresh() {
        updateCache();
    }

    private void updateCache() {

        synchronized (refreshLock) {
            if (refreshing) {
                return;
            }
            refreshing = true;
        }

        Thread refresher = new Thread(() -> backgroundUpdate());
        refresher.setDaemon(true);
        refresher.start();
    }

    private void backgroundUpdate() {

        CachedObject co = null;
        try {

            co = getFromDb();

        } finally {
            synchronized(refreshLock) {
                cachedInstance = co;
                refreshing = false;
                expireCache = System.currentTimeMillis() + cachePeriodMs;
                refreshLock.notifyAll();
            }
        }

    }

    private CachedObject getFromDb() {
        // dummy call, no db is involved
        return new CachedObject();
    }

    public long getCachePeriodMs() {
        return cachePeriodMs;
    }
}

Here are features in there that I believe are improvements:

  1. The thread you start is a daemon thread (it will not hold up the shutdown of the system.
  2. the cache duration is set with an expire-time, not a value-set-at time. The expire-time means there is no math needed at check-time, so the cache invalidation is simpler.
  3. There is no complicated logic for the initialization delay. The get() thread simply waits until there's data. If the data is invalid, it starts an update.
  4. I use a Java-8 call to a functional interface to set the thread's Runnable.
  5. Do not extend Thread - extend Runnable, and add the Runnable to a thread.
  6. only 1 mechanism used for memory-consistency management - synchronization. No more latches, atomics, or volatile
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.