Ehcache Core
  1. Ehcache Core
  2. EHC-715

BlockingCache.put removes entry if Element.value == null

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: 2 Major 2 Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.0.1
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Terracotta Target:
      Vicente_Holding

      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.

        Issue Links

          Activity

          Hide
          Alexander Snaps added a comment -

          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...

          Show
          Alexander Snaps added a comment - 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...
          Hide
          Ian Jones added a comment -

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

          In my 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 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.

          Show
          Ian Jones added a comment - This is quite a problem because it breaks the Liskov substitution principle, as implementations of Ehcache are not substitutable. In my 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 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.
          Hide
          Adnan Memon added a comment -

          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.

          Show
          Adnan Memon added a comment - 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.
          Hide
          Alexander Snaps added a comment -

          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>
          
          Show
          Alexander Snaps added a comment - 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>
          Hide
          Alexander Snaps added a comment -

          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...)

          Show
          Alexander Snaps added a comment - 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...)

            People

            • Assignee:
              Alexander Snaps
              Reporter:
              Alexander Snaps
            • Votes:
              2 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: