CODEWITHSUNDEEP
javaclass
Bhaskar
Bhaskar

Reputation: 7523

Java Cache design question

I was writing a cache implementation - it would expire a stored item if it has been in the store for more than say , 5 mins. In this case it should be refreshed from a source , otherwise the cached copy should be returned.

Below is what I wrote - Are there any design flaws with it ? In particular , the get part ?

public class Cache<K,V> {
     private final ConcurrentMap<K,TimedItem> store ;
     private final long expiryInMilliSec ;


    Cache(){
        store = new ConcurrentHashMap<K, TimedItem>(16);
         expiryInMilliSec = 5 * 60 * 1000; // 5 mins
     }

    Cache(int minsToExpire){
        store = new ConcurrentHashMap<K, TimedItem>(16);
        expiryInMilliSec = minsToExpire * 60 * 1000; 
     }

// Internal class to hold item and its 'Timestamp' together
private class TimedItem {
    private long timeStored ;
    private V item ;

    TimedItem(V v) {
        item = v;
        timeStored = new Date().getTime();
    }

    long getTimeStored(){
        return timeStored;
    }

    V getItem(){
        return item;
    }

    public String toString(){
        return item.toString();
    }
}

// sync on the store object - its a way to ensure that it does not interfere
// with the get method's logic below
public void put(K key, V item){
    synchronized(store){
        store.putIfAbsent(key, new TimedItem(item));
    }
}

// Lookup the item, check if its timestamp is earlier than current time 
// allowing for the expiry duration
public V get(K key){
    TimedItem ti = null;
    K keyLocal = key;
    boolean refreshRequired = false;

    synchronized(store){
        ti = store.get(keyLocal);
        if(ti == null)
            return null;
        long currentTime = new Date().getTime();
        if( (currentTime - ti.getTimeStored()) > expiryInMilliSec ){
            store.remove(keyLocal);
            refreshRequired = true;
        }
    }
    // even though this is not a part of the sync block , this should not be a problem
    // from a concurrency point of view
    if(refreshRequired){
        ti = store.putIfAbsent(keyLocal, new TimedItem(getItemFromSource(keyLocal)) );
    }
    return ti.getItem();
}

private V getItemFromSource(K key){
    // implement logic for refreshing item from source 
    return null ;  
}

public String toString(){
    return store.toString();
}

}

Upvotes: 0

Views: 852

Answers (2)

MRAB
MRAB

Reputation: 20654

The documentation says that replace is atomic, so I'd do something like this:

public V get(K key){
    TimedItem ti;

    ti = store.get(key);
    if(ti == null)
        return null;

    long currentTime = new Date().getTime();
    if((currentTime - ti.getTimeStored()) > expiryInMilliSec){
        ti = new TimedItem(getItemFromSource(key));
        store.replace(key, ti);
    }

    return ti.getItem();
}

Upvotes: 0

Ryan Stewart
Ryan Stewart

Reputation: 128829

Given that you're trying to synchronize things manually and (at a guess) you seem not to have tested it very thoroughly, I'd say there's about a 98% chance that you have a bug in there. Is there a good reason you're not using functionality provided by an established caching library, like Ehcache's SelfPopulatingCache?

Upvotes: 1

Related Questions