Using EHCache the right way

Question

I use EHCache to maintain a mostly read-only cache of results from database queries. It works perfectly for over a dozen queries.

However, there is one table which needs an odd access method that is causing me grief. I hope someone with a knowledgeable eye could see what I am doing wrong, if anything.

Note that these classes are marked static because I have torn them out of live code and put them all into a single Test class - just for you. :)

Here is the Cache object which wraps an ehcache object. It works fine in all case except my special case I will post further down.

// My Cache object which wraps the EHCache object into a simple API.
public static class Cache<K, V> {

    // The cache.
    private final Ehcache cache;

    private Cache(final Ehcache cache, final String name) {
        // Plain ordinary.
        this.cache = cache;
    }

    public Cache(final String name) {
        this(CacheManager.getInstance().addCacheIfAbsent(name), name);
    }

    public Cache(final String name, CacheEntryFactory factory) {
        // Wrap it with a factory.
        this(new SelfPopulatingCache(CacheManager.getInstance().addCacheIfAbsent(name), factory), name);
    }

    public void put(final K key, final V value) {
        cache.put(new Element(key, value));
    }

    @SuppressWarnings("unchecked")
    public V get(final K key) {
        final Element element = cache.get(key);
        if (element != null) {
            return (V) element.getObjectValue();
        }
        return null;
    }

    public void remove(final K key) {
        cache.remove(key);
    }

    public void clear() {
        cache.removeAll();
    }

    public boolean expired(final K key) {
        boolean expired = true;
        final Element element = cache.get(key);
        if (element != null) {
            expired = cache.isExpired(element);
        }
        return expired;
    }

    public Ehcache getCache() {
        return cache;
    }

}

Note that this is cut down from the real one but only to remove features such as auto-configuration.

The use-case that fails occasionally for me runs like this. This special table can be updated by other parties so customer needs a regular full refresh of the cache. To achieve this I use a load method that always checks to see if the cache has expired and if it has, rebuilds it.

To detect a timeout I insert a pill into the cache under an ID that will never be accessed otherwise.

To ensure no clash with other threads I also use a ReadWriteLock to forestall other threads trying to read the cache while it is being rebuilt.

// The thing that is cached.
static class Thing {
}

// Sample element to detect cache stale.
private static final Thing Sample = new Thing();
// Cache timer checker - inserted in the cache at startup and checked for expiry regularly.
private static final String CachePill = "";
// Cache for the things - use a factory as well as a cache pill.
private final Cache<String, Thing> thingCache = new Cache<String, Thing>("Things");

// Locks to allow both read and write access.
private static final ReadWriteLock cacheLock = new ReentrantReadWriteLock();
private static final Lock cacheReadLock = cacheLock.readLock();
private static final Lock cacheWriteLock = cacheLock.writeLock();

private void loadCache() throws Exception {
    // Expiry.
    while (thingCache.expired(CachePill)) {
        /**
         * Allow only one in - all others will wait for 5 seconds before checking again.
         *
         * Eventually the one that got in will finish loading, refresh the Cache pill and let all the waiting ones out.
         *
         * Also waits until all read locks have been released - not sure if that might cause problems under busy conditions.
         */
        if (cacheWriteLock.tryLock(5, TimeUnit.SECONDS)) {
            try {
                // Got a lock! Start the rebuild if still out of date.
                if (thingCache.expired(CachePill)) {
                    rebuildCache();
                }
            } finally {
                cacheWriteLock.unlock();
            }
        }
    }
}

// Only ever called by loadCache.
private void rebuildCache() throws Exception {
    // Clear down everything.
    thingCache.clear();
    // Run a query that does a whole bunch of
    {
        // ...
        thingCache.put(id, thing);
        // ...
    }
    // Add my sample element to the cache last so everyone waiting on me can go.
    thingCache.put(CachePill, Sample);
}

public Thing load(String id) throws Exception {
    // Make sure the cache is fresh.
    loadCache();
    Thing thing;
    try {
        // Establish a read lock so we do not attempt a read while teh cache is being updated.
        cacheReadLock.lock();
        thing = thingCache.get(id);
    } finally {
        // Make sure the lock is cleared.
        cacheReadLock.unlock();
    }
    return thing;
}

My problem is that sometimes (unpredictably but after at least 1 hour of idle time - which is how long the cache pill takes to expire) the code passes through this load mechanism, does not rebuild the cache but retrieves a null. I know that the thing was added to the cache in the last rebuild so my logic follows that something unexpected is happening but I cannot think what.

Any ideas?

Notes

• I do see log entries that indicate the cache is being rebuilt - at the expected times.
• I can see signs that other requests are being held off while the cache is being rebuilt.
• When this goes wrong it goes wrong badly i.e. almost all things come back null from lookups (but only almost, some seem to still be there).

My conclusions - this code is almost right but I have missed a situation that can leave the cache almost empty even though I have added all items to it.

Answer 1

I believe I have finally found the problem.

Spot the stupid mistake:

public boolean expired(final K key) {
    boolean expired = true;
    final Element element = cache.get(key);
    if (element != null) {
        expired = cache.isExpired(element);
    }
    return expired;
}

Should be:

public boolean expired(final K key) {
    boolean expired = true;
    // Do a quiet get so we don't change the last access time.
    final Element element = cache.getQuiet(key);
    if (element != null) {
        expired = cache.isExpired(element);
    }
    return expired;
}

Note the use of cache.getQuiet(key) instead of cache.get(key).

What was happening was that every time I checked to see if the cache had expired I was resetting the access time of the pill - and therefore not expiring the cache.