ACS-1264: Content Model changes dynamically updated to node caches across a cluster deadlock (#289)

Re enabling changes disabled in ACS-936 we can see deadlock in asynchronouslyRefreshedCacheThreadPool (AbstractAsynchronouslyRefreshedCache).

There should be no reason to be calling the dictionary destroy method before the doCall() finishes...and it is the use of the destroy method that carries the risk of deadlock.

Proposed fix: the liveLock is used for the doCall() method, this will stop deadlock from external calls to dictionary destroy() while doCall is in progress.

Removed invalidating cache fix (e.g. fix the issue where cluster nodes could have null values and other nodes had default value… so no properly invalidated on node startup). This fix was moved in ClusteringBootstrap as the initial one was causing issues.

core/src/main/java/org/alfresco/util/cache/AbstractAsynchronouslyRefreshedCache.java

@@ -424,6 +424,7 @@ public abstract class AbstractAsynchronouslyRefreshedCache<T>
         @Override
         public Void call()
         {
+            liveLock.writeLock().lock();
             try
             {
                 doCall();
@@ -444,6 +445,10 @@ public abstract class AbstractAsynchronouslyRefreshedCache<T>
                 }
                 return null;
             }
+            finally
+            {
+                liveLock.writeLock().unlock();
+            }
         }
 
         private void doCall() throws Exception

repository/src/main/java/org/alfresco/repo/cache/DefaultSimpleCache.java

@@ -150,24 +150,10 @@ public final class DefaultSimpleCache<K extends Serializable, V extends Object>
      * @return <code>true</code> if the put resulted in a change in value, <code>false</code> otherwise.
      */
     public boolean putAndCheckUpdate(K key, V value)
-    {
-        return putAndCheckUpdate(key, value, false);
-    }
-
-    /**
-     * <code>put</code> method that may be used to check for updates in a thread-safe manner.
-     *
-     * @param includeNewCheck if true then we include the new value in the check
-     * @return <code>true</code> if the put resulted in a change in value, 
-     *     or if includeNewCheck is true and the put resulted in a new value, 
-     *     <code>false</code> otherwise.
-     */
-    public boolean putAndCheckUpdate(K key, V value, boolean includeNewCheck)
     {
         AbstractMap.SimpleImmutableEntry<K, V> kvp = new AbstractMap.SimpleImmutableEntry<K, V>(key, value);
         AbstractMap.SimpleImmutableEntry<K, V> priorKVP = cache.asMap().put(key, kvp);
-
+        return (priorKVP != null && (!priorKVP.equals(kvp)));
-        return (includeNewCheck && priorKVP == null) || (priorKVP != null && (!priorKVP.equals(kvp)));
     }
     
     @Override

repository/src/main/resources/alfresco/tx-cache-context.xml

@@ -136,7 +136,6 @@
       <property name="tenantAware" value="false" />
       <property name="cacheStats" ref="cacheStatistics"/>
       <property name="cacheStatsEnabled" value="${cache.immutableEntitySharedCache.tx.statsEnabled}"/>
-      <property name="allowEqualsChecks" value="true" />
    </bean>
    
    

repository/src/test/java/org/alfresco/repo/cache/DefaultSimpleCacheTest.java

@@ -131,45 +131,6 @@ public class DefaultSimpleCacheTest extends SimpleCacheTestBase<DefaultSimpleCac
         assertEquals(false, cache.putAndCheckUpdate(104, "104"));
         assertEquals(true, cache.putAndCheckUpdate(104, null));
     }
-
-    @Test
-    public void putAndCheckUpdateIncludeNewCheck()
-    {
-        // Put an initial value
-        cache.put(101, "101");
-        // Update it
-        assertEquals(true, cache.putAndCheckUpdate(101, "99101", true));
-        // Check the value really was updated
-        assertEquals("99101", cache.get(101));
-
-        // Precondition: no value for key 102
-        assertFalse(cache.contains(102));
-        // Put a value - and test the return
-        assertEquals(true, cache.putAndCheckUpdate(102, "102", true));
-        assertEquals("102", cache.get(102));
-
-        cache.put(103, null);
-        assertEquals(true, cache.putAndCheckUpdate(103, "103", true));
-        // Repeat the put, this should not be an update
-        assertEquals(false, cache.putAndCheckUpdate(103, "103", true));
-
-        assertFalse(cache.contains(104));
-        assertEquals(true, cache.putAndCheckUpdate(104, null, true));
-        // Repeat putting null - still not an update, as we had that value a moment ago.
-        assertEquals(false, cache.putAndCheckUpdate(104, null, true));
-        // Now an update
-        assertEquals(true, cache.putAndCheckUpdate(104, "104", true));
-        // Another update
-        assertEquals(true, cache.putAndCheckUpdate(104, "99104", true));
-        // Another update, back to null
-        assertEquals(true, cache.putAndCheckUpdate(104, null, true));
-        // Not an update - still null
-        assertEquals(false, cache.putAndCheckUpdate(104, null, true));
-
-        cache.remove(104);
-        assertEquals(true, cache.putAndCheckUpdate(104, "104", true));
-        assertEquals(true, cache.putAndCheckUpdate(104, null, true));
-    }
     
     // TODO: Timer-based tests are not ideal. An alternative approach is to factor out the CacheBuilder.newBuilder()
     // call to a protected method, override that in this test class to return a mock and use the mock to check