• Bug
  • Status: Closed
  • 2 Major
  • Resolution: Fixed
  • ehcache-core
  • hsingh
  • Reporter: mads1980
  • May 25, 2010
  • 0
  • Watchers: 1
  • January 17, 2013
  • May 25, 2010

Attachments

Description

The following methods invoke freeBlock(diskElement):

  • expireElements()
  • evictLfuDiskElements()
  • remove()
  • removeOldEntryIfAny()

Both remove() and removeOldEntryIfAny() process DiskElements correctly by invoking diskElements.remove(key) which locks the ConcurrentHashMap’s segment for the key in question.

The other two methods introduce problems when there is high concurrency of removal / expiration / eviction. For instance, expireElements() iterates through all diskElements using ConcurrentHashMap’s weakly consistent iterator. By the time the iteration finds a DiskElement that needs to be expired, the DiskElement may have been already removed / evicted by another thread (remember that these iterators may or may not reflect some or all of the changes in the underlying map). So if the diskElement had already been removed, we would be calling freeBlock() twice on the same block.

Another consistency problem may occur when there’s a configured expiration cacheEventListener, since loadElementFromDiskElement() may even return an Element completely different from the original one, as the block may have been freed and reclaimed by another thread while the iteration took place.

The proposed fix for expireElements() is to use diskElements.remove(key) instead of iterator.remove(), so that we can atomically use its return value in order to determine whether the entry has already been removed by another thread. The relevant code fragment follows:

        if (now >= diskElement.expiryTime) {
            // An expired element
            if (LOG.isDebugEnabled()) {
                LOG.debug(name + "Cache: Removing expired spool element " + entry.getKey() + " from Disk Store");
            }
            // This ensures the DiskElement is only removed and processed once
            diskElement = (DiskElement) diskElements.remove(entry.getKey());
            if (diskElement != null) {
                // only load the element from the file if there is a listener interested in hearing about its expiration
                if (listeners.hasCacheEventListeners()) {
                    try {
                        Element element = loadElementFromDiskElement(diskElement);
                        notifyExpiryListeners(element);
                    } catch (Exception exception) {
                        LOG.error(name + "Cache: Could not remove disk store entry for " + entry.getKey()
                                + ". Error was " + exception.getMessage(), exception);
                    }
                }
                freeBlock(diskElement);
            }
        }

evictLfuDiskElements() similarly may be subject to the same issue. The proposed fix is similar to that for expireElements():

private void evictLfuDiskElements(int count) {
    for (int i = 0; i < count; i++) {
        DiskElement diskElement = findRelativelyUnused();
        // This ensures the DiskElement is only removed and processed once
        diskElement = (DiskElement) diskElements.remove(diskElement.key);
        if (diskElement != null) {
            notifyEvictionListeners(diskElement);
            freeBlock(diskElement);
        }
    }
}

Applying these changes we ensure that freeBlocks() is only invoked once for each block after removal / expiration / eviction, even when these events may happen concurrently.

Comments

Manuel Dominguez Sarmiento 2010-05-25

DiskStore patch including mods for EHC-721, EHC-722, EHC-723, EHC-725, and EHC-726

Manuel Dominguez Sarmiento 2010-05-25

EHC-726 is related to this issue.

Himadri Singh 2010-06-27

Tweaked patch has been checked in.