• Bug
  • Status: Closed
  • 2 Major
  • Resolution: Fixed
  • ehcache-core
  • hsingh
  • Reporter: lima
  • November 25, 2009
  • 0
  • Watchers: 0
  • January 17, 2013
  • February 11, 2010

Description

This is a report from a client:

Hi Li,

How would you explain the fact that get() method of BlockingCache class listed below acquires write lock on the key unconditionally, prior to attempting to get an Element from cache? Isn’t there a contradiction with whatever is claimed in javadocs as a promise of “concurrent read access to elements already in the cache”?

Here is the get() method I’m talking about:

/** * Looks up an entry. Blocks if the entry is null until a call to {@link #put} is done * to put an Element in. * <p/> * If a put is not done, the lock is never released. * <p/> * If this method throws an exception, it is the responsibility of the caller to catch that exception and call * put(new Element(key, null)); to release the lock acquired. See {@link net.sf.ehcache.constructs.blocking.SelfPopulatingCache} * for an example. * <p/> * Note. If a LockTimeoutException is thrown while doing a get it means the lock was never acquired, * therefore it is a threading error to call {@link #put} * * @throws LockTimeoutException if timeout millis is non zero and this method has been unable to * acquire a lock in that time * @throws RuntimeException if thrown the lock will not released. Catch and callput(new Element(key, null)); * to release the lock acquired. */ public Element get(final Object key) throws RuntimeException, LockTimeoutException {

   Sync lock = getLockForKey(key);
   Element element; //        boolean tcClustered = cache.getCacheConfiguration().isTerracottaClustered(); //        if (!tcClustered) \{ //            acquiredLockForKey(key, lock, LockType.READ); //        \} //        element = cache.get(key); //        if (!tcClustered) \{ //            lock.unlock(LockType.READ); //        \} //        if (element == null) \{
       acquiredLockForKey(key, lock, LockType.WRITE);
       element = cache.get(key);
       if (element != null) {
           lock.unlock(LockType.WRITE);
       } //        \}
   return element;    \}

Comments

Alexander Snaps 2009-11-25

I’m responsible for that one. The issue is already explained here: https://jira.terracotta.org/jira/browse/EHC-420 It was a conscious decision to have it behave as before, where it would acquire the lock (mutex) right away. Indeed, we could have done bet with this impl. but it would have meant we’d have lost performance with TC in the play. (Think we discussed that in El Capitan w/ Alex M & Chris while we were all in SF). Not sure how that contradicts the javadoc (it says _Blocks_, not “Locks if the entry is null”, right?)

Here’s the previous implementation:

public Element get(final Object key) throws RuntimeException, LockTimeoutException { Mutex lock = getLockForKey(key); try { if (timeoutMillis == 0) { lock.acquire(); } else { boolean acquired = lock.attempt(timeoutMillis); if (!acquired) { StringBuffer message = new StringBuffer(“Lock timeout. Waited more than “) .append(timeoutMillis) .append(“ms to acquire lock for key “) .append(key).append(“ on blocking cache “).append(cache.getName()); throw new LockTimeoutException(message.toString()); } } final Element element = cache.get(key); if (element != null) { //ok let the other threads in lock.release(); return element; } else { //don’t release the read lock until we put return null; } } catch (InterruptedException e) { throw new CacheException(“Get interrupted for key “ + key + “. Message was: “ + e.getMessage()); } }

Alexander Snaps 2009-11-25

Talking w/ Abhishek we realized we weren’t aligned. I was talking about the method’s doc while you guys probably were referencing the class’ Indeed this bit looks like an overstatement:

* It allows concurrent read access to elements already in the cache. If the element is null, other * reads will block until an element with the same key is put into the cache.

Abhishek proposes to put it more like “concurrent read access to different element keys already in the cache”. Again if we fix EHC-420 we’d get to real concurrent reads on any elements.

Fiona OShea 2010-01-26

Believe this may already be resolved. can you verify?

Alexander Snaps 2010-01-28

I’ll check with Li Ma to see what happened there… Code was never commited, patches were only submitted here and were for Li Ma to test at the customer’s site.

Alexander Snaps 2010-02-11

See EHC-420