5
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ In rebuildCache(), after clearing there is no need to continuously hold a write lock, you could drop it between puts. Then the rebuild thread would look like any other reader, racing to hit hot items before production threads hit them. If it loses the race, that's OK. Where it wins, production threads enjoy lower latency. BTW, be sure to sort by popularity so it brings in popular items first. \$\endgroup\$ – J_H Aug 25 '17 at 16:13
6
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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