HashMap is broken/ performance issues

Currently I have Has开发者_如何转开发hMap implemented which

private static Map<String, Item> cached = new HashMap<String, Item>();

and Item is a object with properties Date expirationTime , and byte[] data

This map is used when multiple threads concurrently start hitting this. The check I do is

1.

public static final byte[] getCachedData(HttpServletRequest request) throws ServletException
{
    String url = getFullURL(request);
    Map<String, Item> cache = getCache(request);  // this chec
    Item item = null;

    synchronized (cache)
    {
        item = cache.get(url);
        if (null == item)
            return null;

        // Make sure that it is not over an hour old.
        if (item.expirationTime.getTime() < System.currentTimeMillis())
        {
            cache.remove(url);
            item = null;
        }
    }

    if (null == item)
    {
        log.info("Expiring Item: " + url);
        return null;
    }

    return item.data;
}

2. If data is returned null, then we create and data and cache it in hashMap

public static void cacheDataX(HttpServletRequest request, byte[] data, Integer minutes) throws ServletException
{
    Item item = new Item(data);
    String url = getFullURL(request);
    Map<String, Item> cache = getCache(request);

    log.info("Caching Item: " + url + " - Bytes: " + data.length);
    synchronized (cache)
    {
        Calendar cal = Calendar.getInstance();
        cal.add(Calendar.MINUTE, minutes);
        item.expirationTime = cal.getTime();
        cache.put(url, item);
    }
}

Seems like if multiple threads access the say key (url in this case) , then data gets added to cache more than once at same key location [ as getCacheData will return null for multiple threads since hashmap has not finished writing data for first thread ]

Any suggestions as how to solve the issue ?

In cacheDataX add a check for the existence of the item before you add (inside of the synchronized block).

synchronized (cache)
    {
        if (cache.get(url) == null) {
            Calendar cal = Calendar.getInstance();
            cal.add(Calendar.MINUTE, minutes);
            item.expirationTime = cal.getTime();
            cache.put(url, item);
        }
    }

This will ensure that multiple threads that have already done a lookup and returned null cannot all add the same data to the cache. One will add it and others threads will silently ignore since the cache has already been updated.

You need one synchronize block, to cover both getting something from the cache plus inserting into the cache. As the code stands you have a race condition: multiple threads can execute step 1 before anybody executes step 2.