Guava cache considering older key

Question

I am facing an issue with Guava Caches. When I have only one element in cache, things are fine. But when I load a second element, Its trying to pick with key of earlier entry

private static LoadingCache<String, MyClass> cache = null;
....
public MyClass method(final String id1, final long id2)  {
    log.error("inside with "+id1);
    final String cacheKey = id1+"-"+id2;
    if(cache == null){
        cache = CacheBuilder.newBuilder()
       .maximumSize(1000)
       .build(
            new CacheLoader<String, MyClass>() {
                @Override
                public MyClass load(String key) {
                    return getValue(cacheKey);
                }
           }
        );
    }
    try {
        return cache.get(cacheKey);
    } catch (ExecutionException ex) {
        log.error("EEE missing entry",ex);
    }
}

private MyClass getValue(String cacheKey){
    log.error("not from cache "+cacheKey);
    ...

}

The log says:

inside with 129890038707408035563943963861595603358
not from cache 1663659699-315839912047403113610285801857400882820 // This is key for the earlier entry

For eg, When I call method("1", 2), it loads the value in cache and I am able to get it from cache subsequently. Now I call method ("3", 4), this is not in cache, so getValue() is called and the log prints out the key for method("1", 2)

Where am I going wrong?

Answer 1

Your problem is related to how you create your CacheLoader, if you check well you will see that you initialize it with a given cache key (the value of the local variable cacheKey at the time the cache is lazily initialized) while it should be more generic and rely on the key provided as parameter to the method load of your CacheLoader otherwise it will load the cache by calling getValue(key) with the same key.

It should be this:

new CacheLoader<String, MyClass>() {
    @Override
    public MyClass load(String key) {
        return getValue(key); // instead of return getValue(cacheKey);
    }
}

---

NB: The way you initialize your cache is not thread safe, indeed if it has not been initialized and your method method is called by several threads concurrently it will be created several times instead of one.

One way could be to use the double-checked locking idiom as next:

private static volatile LoadingCache<String, MyClass> cache = null;
public MyClass method(final String id1, final long id2)  {
    ...
    if(cache == null){
        synchronized (MyClass.class) {
            if(cache == null){
                cache = ...
            }
        }
    }

---

NB: Do not initialize a static cache with a CacheLoader based on a non static method, it is much too error prone. Make them both non static or static but don't mix them.

Assuming that you can make both static, your cache initialization will be very simply, it would simply be:

private static final LoadingCache<String, MyClass> cache = CacheBuilder.newBuilder()...

No need to initialize it lazily which will also simplify a lot the code of your method as it will simply be reduce to:

public MyClass method(final String id1, final long id2)  {
    log.error("inside with "+id1);
    final String cacheKey = id1+"-"+id2;
    try {
        return cache.get(cacheKey);
    } catch (ExecutionException ex) {
        log.error("EEE missing entry",ex);
    }
}