Java thread safe caching, and return old cach if getting new is in progress

Question

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

My 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

From SO and some other user help I have this ATM:

** Edited with Sandeep's and Kayaman's advice **

public enum MyClass
{
    INSTANCE;

    // caching field
    private CachedObject cached = null;

    private AtomicLong lastVisistToDB = new AtomicLong();
    private long refreshInterval = 1000 * 60 * 5;

    private CachedObject createCachedObject()
    {
        return new CachedObject();
    }

    public CachedObject getCachedObject()
    {
        if( ( System.currentTimeMillis() - this.lastVisistToDB.get() ) > this.refreshInterval)
        {
            synchronized( this.cached )
            {
                if( ( System.currentTimeMillis() - this.lastVisistToDB.get() ) > this.refreshInterval)
                {
                    this.refreshCachedObject();
                }
            }
        }

        return this.cached;
    }

    public void refreshCachedObject()
    {
        // This is to prevent threads waiting on synchronized from re-refreshing the object     
        this.lastVisistToDB.set(System.currentTimeMillis());

        new Thread() 
        {
            public void run() 
            {
                createCachedObject();
                // Update the actual refresh time
                lastVisistToDB.set(System.currentTimeMillis());  
            }
        }.start();
    }
}

In my opinion my code does all of the above written requirements. (but I'm not sure)

With code soon going to third party analysis, I really would appreciate any input on code performance and blind spots

Thx for your help.

EDIT : VanOekel's answer IS the solution, because my code ( Edited with Sandeep's and Kayaman's advice ), doesn't account for impact of user-triggered refresh() in this multi-threading enviroment

Answer 1

Instead of DCL as proposed by Sandeep, I'd use the enum Singleton pattern, as it's the best way for lazy-init singletons these days (and looks nicer than DCL).

There's a lot of unnecessary variables and code being used, I'd simplify it a lot.

private static Object cachedObject;
private AtomicLong lastTime = new AtomicLong();
private long refreshPeriod = 1000;

public Object get() {

    if(System.currentTimeMillis() - lastTime.get() > refreshPeriod) {
        synchronized(cachedObject) {
            if(System.currentTimeMillis() - lastTime.get() > refreshPeriod) {
                lastTime.set(System.currentTimeMillis());    // This is to prevent threads waiting on synchronized from re-refreshing the object
                new Thread() {
                    public void run() {
                        cachedObject = refreshObject();  // Get from DB
                        lastTime.set(System.currentTimeMillis());  // Update the actual refresh time
                    }
                }.start();
            }
        }
    }
    return cachedObject;
}

Speedwise that could still be improved a bit, but a lot of unnecessary complexity is reduced. Repeated calls to System.currentTimeMillis() could be removed, as well as setting lastTime twice. But, let's start off with this.

Answer 2

You should put in double checked locking in getInstance().

Also, you might want to keep just one volatile cache object, and in getAndRefreshCashedObject(), and where-ever it's refreshed, you could calculate the new data, and assign it in a syncronized way to the cache object you have. This way, the code might look smaller, and you don't need to maintain loadInProgress, oldCached variables

Answer 3

I arrive at a somewhat different solution when taking into account the "random" refresh triggered by a user. Also, I think the first fetch should wait for the cache to be filled (i.e. wait for first cached object to be created). And, finally, there should be some (unit) tests to verify the cache works as intended and is thread-safe.

First the cache implementation:

import java.util.concurrent.*;
import java.util.concurrent.atomic.*;

// http://stackoverflow.com/q/31338509/3080094
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; // make this an AtomicLong if it can be updated

    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;
    }

}

Second the cached object with a main-method that tests the cache implementation:

import java.util.concurrent.*;
import java.util.concurrent.atomic.*;

public class CachedObject {

    private static final AtomicInteger createCount = new AtomicInteger();
    static final long createTimeMs = 100L;

    private final int instanceNumber = createCount.incrementAndGet();

    public CachedObject() {
        println("Creating cached object " + instanceNumber);
        try {
            Thread.sleep(createTimeMs);
        } catch (Exception ignored) {}
        println("Cached object " + instanceNumber + " created");
    }

    public int getInstanceNumber() {
        return instanceNumber;
    }

    @Override
    public String toString() {
        return getClass().getSimpleName() + "-" + getInstanceNumber();
    }

    private static final long startTime = System.currentTimeMillis();

    /**
     * Test the use of DbCachedObject.
     */
    public static void main(String[] args) {

        ThreadPoolExecutor tp = (ThreadPoolExecutor) Executors.newCachedThreadPool();
        final int tcount = 2; // amount of tasks running in paralllel
        final long threadStartGracePeriodMs = 50L; // starting runnables takes time
        try {
            // verify first calls wait for initialization of first cached object
            fetchCacheTasks(tp, tcount, createTimeMs + threadStartGracePeriodMs);
            // verify immediate return of cached object
            CachedObject o = DbCachedObject.INSTANCE.get();
            println("Cached: " + o);

            // wait for refresh-period
            Thread.sleep(DbCachedObject.INSTANCE.getCachePeriodMs() + 1);
            // trigger update
            o = DbCachedObject.INSTANCE.get();
            println("Triggered update for " + o);
            // wait for update to complete
            Thread.sleep(createTimeMs + 1);
            // verify updated cached object is returned
            fetchCacheTasks(tp, tcount, threadStartGracePeriodMs);

            // trigger update
            DbCachedObject.INSTANCE.refresh();
            // wait for update to complete
            Thread.sleep(createTimeMs + 1);
            println("Refreshed: " + DbCachedObject.INSTANCE.get());

        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            tp.shutdownNow();
        }
    }

    private static void fetchCacheTasks(ThreadPoolExecutor tp, int tasks, long doneWaitTimeMs) throws Exception {

        final CountDownLatch fetchStart = new CountDownLatch(tasks);
        final CountDownLatch fetchDone = new CountDownLatch(tasks);
        // println("Starting " + tasks + " tasks");
        for (int i = 0; i < tasks; i++) {
            final int r = i;
            tp.execute(new Runnable() {
                @Override public void run() {
                    fetchStart.countDown();
                    try { fetchStart.await();} catch (Exception ignored) {}
                    CachedObject o = DbCachedObject.INSTANCE.get();
                    println("Task " + r + " got " + o);
                    fetchDone.countDown();
                }
            });
        }
        println("Awaiting " + tasks + " tasks");
        if (!fetchDone.await(doneWaitTimeMs, TimeUnit.MILLISECONDS)) {
            throw new RuntimeException("Fetch cached object tasks incomplete.");
        }
    }

    private static void println(String msg) {
        System.out.println((System.currentTimeMillis() - startTime) + " "  + msg);
    }
}

The tests in the main-method need human eyes to verify the results, but they should provide sufficient input for unit tests. Once the unit tests are more refined, the cache implementation will probably need some finishing touches as well.