Java Cache design question

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

}

i'd prefer System.currentTimeMillis() over new Date().getTime() — guido
– guido, Commented Jul 29, 2011 at 22:39
If you haven't already done so, you may want to post this to Code Review, which is geared towards these sort of questions. — ig0774
– ig0774, Commented Jul 29, 2011 at 23:02
Not really an answer, but .... If this isn't homework you really shouldn't do this by hand. Caches are a solved problem with many well tested implementations out there that are free and open source. I truly hope this is just an engineering exercise. Otherwise, you are just asking for trouble. — rfeak
– rfeak, Commented Jul 29, 2011 at 23:13
You should not use synchronized(store) but define some dedicated Object lock. Things can get really messed up (deadlock) if store uses itself as a synchronization lock too (by defining a synchronized method in ConcurrentHashMap for example). BTW I checked the code and in the implementation of Oracle Java 7 ConcurrentHashMap does not synchronize on itself. — toto2
– toto2, Commented Jul 30, 2011 at 1:15

Ryan Stewart · Accepted Answer · 2011-07-29 22:47:10Z

1

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?

answered Jul 29, 2011 at 22:47

Ryan Stewart

130k22 gold badges183 silver badges201 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Bhaskar Over a year ago

Assume for a moment - usage of external libraries is restricted. I did some elementary Junit testing though - and it passed those.

MRAB · Accepted Answer · 2011-07-29 22:55:28Z

0

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

answered Jul 29, 2011 at 22:55

MRAB

20.7k6 gold badges44 silver badges34 bronze badges

4 Comments

MRAB Over a year ago

@Bhaskar: getItemFromSource isn't in a sync block.

Bhaskar Over a year ago

The point of using a sync block is not around the remove or replace , but in making sure that between the time check and the actual modification - no other thread can modify the collection.

Dave Costa Over a year ago

Two threads could both get the same expired item, re-fetch it from the source, and replace it. I don't think would cause wrong results, but more work than necessary, especially if getItemFromSource() is resource-intensive, which I assume is the reason for the cache.

MRAB Over a year ago

@Dave: True, although whether that's a problem it depends on what the cost actually is. Is the cost high enough, and does duplicated fetching occur often enough, to justify a more complicated solution?

Collectives™ on Stack Overflow

Java Cache design question

2 Answers 2

1 Comment

4 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

4 Comments

Your Answer

Sign up or log in

Post as a guest

Related