• Bug
  • Status: Resolved
  • 2 Major
  • Resolution: Won't Fix
  • alexsnaps
  • Reporter: alexsnaps
  • May 20, 2010
  • 2
  • Watchers: 6
  • January 07, 2014
  • January 07, 2014

Description

Not sure whether that serves a purpose or not… Seems to be like that since 2006. It was posted on the forum and I had a quick mail exchange with Greg on the subject:

Greg, Maybe you can have a look at this: http://forums.terracotta.org/forums/posts/list/3695.page I’m not sure why BlockingCache.put is implemented as such… Certainly would be interested in some background though. Or is this something we might change?

Alex, Really no good reason. If I ever had a reason it would be on the javadoc. The thing to bear in mind is that you need to support backward compatibility with the web cache and anybody using the BlockingCache now. If you can do that I am happy with the change.

Comments

Alexander Snaps 2010-10-06

I have no clue why BlockingCache has that behavior, but many tests are actually expecting it. Greg, care to comment ?

Fiona OShea 2011-02-22

No updates from Greg, but his first comments seem to indicate that we can change. Can we just fix it? or does it require a discussion?

Alexander Snaps 2011-02-23

I haven’t looked much further in why this change triggered so much test failures (it isn’t always easy to understand why a test makes certain assertions). I’ll have deeper look and will see why this behavior might or not be an issue…

Ian Jones 2011-11-01

This is quite a problem because it breaks the Liskov substitution principle, as implementations of {{Ehcache}} are not substitutable.

In my [cache-aside|http://ehcache.org/documentation/user-guide/concepts#cache-aside] case, I’m using Spring to create my Ehcache and I can change the the implementation from {{Cache}} to {{BlockingCache}} just by setting blocking=true in my Spring configuration file. However, due to the differences in {{put()}} behavior I can’t do this. * If I’m using a {{BlockingCache}}, I need to catch exceptions when calling {{get()}} and and call {{put(new Element(key, null))}} to release the lock as [the Javadoc|http://ehcache.org/apidocs/net/sf/ehcache/constructs/blocking/BlockingCache.html#get%28java.lang.Object%29] says. This has the desired behavior and removes from the cache and allows awaiting threads to continue, miss the cache, and go and retry getting the data from the SOR. If no threads are waiting, a subsequent call will do the same. * If however, I’m using a standard {{Cache}} this code will actually put a null value in the cache and a subsequent call will return a null value, not miss the cache and get the data from the SOR.

It seems as though {{BlockingCache}} should allow a call to {{remove()}} to release the lock, and remove the entry. This would then provide consistent behavior between {{Ehcache}} implementations. My code could safely call {{remove()}} in both of the above situations.

Adnan Memon 2012-07-15

I believe the notion of null value was used to release the write lock by just calling put method on BlockingCache and not to deal with locks or any other methods.

Alexander Snaps 2013-09-03

Haven’t looked further in this.

Alexander Snaps 2014-01-06

Still completely confused about this… Just re-ran the whole test suite w/o the removal to null valued elements and here’s the list of failing tests:

Failed tests: 
  BlockingCacheTest.testInlineEviction:501 null
  BlockingCacheTest.testClear:477 expected:<3> but was:<0>
  BlockingCacheTest.testRemoveEntry:451 expected:<[ key = key, value=value, version=1, hitCount=0, CreationTime = 1389048383241, LastAccessTime = 0 ]> but was:<null>
  BlockingCacheTest.testSupportsStatsCorrectly:132 null
  BlockingCacheTest.testSecondThreadActuallyBlocks:255 expected:<[ key = key, value=value, version=1, hitCount=0, CreationTime = 1389048383316, LastAccessTime = 0 ]> but was:<null>
  BlockingCacheTest.testAddEntry:160 expected:<1> but was:<0>
  BlockingCacheTest.testAddMissingEntry:220 expected:<1> but was:<0>
  SelfPopulatingCacheTest.testRefreshNoisily:714 expected:<2> but was:<0>
  SelfPopulatingCacheTest.testRefreshAbsentElement:639 expected:<2> but was:<0>
  SelfPopulatingCacheTest.testCacheEntryFactoryReturningElementRefresh:547 expected:<2> but was:<0>
  SelfPopulatingCacheTest.testRefreshElement:590 expected:<2> but was:<0>
  SelfPopulatingCacheTest.testCacheEntryFactoryReturningElementMake:522 expected:<2> but was:<0>
  SelfPopulatingCacheTest.testRefreshWithException:231 This should have exploded!
  SelfPopulatingCacheTest.testCreateOnce:187 expected:<1> but was:<2>
  SelfPopulatingCacheTest.testRefresh:206 expected:<2> but was:<1>
  SelfPopulatingCacheTest.testDiscardLittleUsed:283 expected:<2> but was:<0>
  SelfPopulatingCacheTest.testSelfPopulatingBlocksWithoutTimeoutSetNonNull:441 The wrong number of cacheAccessorThreads tried to create selfPopulatingCache entry for key1 expected:<1> but was:<11>
  SelfPopulatingCacheTest.testRefreshQuietly:670 expected:<2> but was:<0>

Alexander Snaps 2014-01-07

Finally found the root cause for this:

It turns out, it’s a leakage of concern. SelfPopulatingCache, which builds (extends more precisely) BlockingCache, calls into get and then checks if this was a miss. On miss it populate the Cache by putting the new Element(key, dataReturnedByTheFactory) into the cache. This not only populates it, but also effectively releases the write lock owned by the thread that had the cache miss.

Now there is the case where the factory throws (say the link to the DB is down). Obviously the lock for that key needs to be released, yet we don’t have a reference to it. The way to release it, is by putting to the cache. Yet putting new Element(key, null) in this particular case wouldn’t be correct (i.e. caching the absence of a value associated with this particular key from the underlying SoR). So null has been chosen as the sentinel value for that particular issue, and on seeing Element.value == null, blocking cache removes the entry.

Now obviously this is very disappointing. But I fear, mainly because of backwards compatibility, there isn’t much we can do about it. Even the tests specify that Element instances with a null value will be removed. This is pretty much spec’ed down at this stage… Even though, yes, this defeats LSP (the one behind cache decorators to begin with…)