From cee63b31f6d98c08eabc3f51461ae137bd9f8a32 Mon Sep 17 00:00:00 2001
From: Cristian Turlica <cristian.turlica@ness.com>
Date: Fri, 19 Feb 2021 09:46:36 +0200
Subject: [PATCH] ACS-1264: Content Model changes dynamically updated to node
 caches across a cluster deadlock (#289)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../AbstractAsynchronouslyRefreshedCache.java |  5 +++
 .../repo/cache/DefaultSimpleCache.java        | 16 +-------
 .../resources/alfresco/tx-cache-context.xml   |  1 -
 .../repo/cache/DefaultSimpleCacheTest.java    | 39 -------------------
 4 files changed, 6 insertions(+), 55 deletions(-)

diff --git a/core/src/main/java/org/alfresco/util/cache/AbstractAsynchronouslyRefreshedCache.java b/core/src/main/java/org/alfresco/util/cache/AbstractAsynchronouslyRefreshedCache.java
index 9f3749af32..b4aabb7db7 100644
--- a/core/src/main/java/org/alfresco/util/cache/AbstractAsynchronouslyRefreshedCache.java
+++ b/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
diff --git a/repository/src/main/java/org/alfresco/repo/cache/DefaultSimpleCache.java b/repository/src/main/java/org/alfresco/repo/cache/DefaultSimpleCache.java
index 376330e12e..977bf90458 100644
--- a/repository/src/main/java/org/alfresco/repo/cache/DefaultSimpleCache.java
+++ b/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 (includeNewCheck && priorKVP == null) || (priorKVP != null && (!priorKVP.equals(kvp)));
+        return (priorKVP != null && (!priorKVP.equals(kvp)));
     }
     
     @Override
diff --git a/repository/src/main/resources/alfresco/tx-cache-context.xml b/repository/src/main/resources/alfresco/tx-cache-context.xml
index 6b47973f2c..8ab850ec58 100644
--- a/repository/src/main/resources/alfresco/tx-cache-context.xml
+++ b/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>
    
    
diff --git a/repository/src/test/java/org/alfresco/repo/cache/DefaultSimpleCacheTest.java b/repository/src/test/java/org/alfresco/repo/cache/DefaultSimpleCacheTest.java
index 43d97ed8f6..7e18fbdf61 100644
--- a/repository/src/test/java/org/alfresco/repo/cache/DefaultSimpleCacheTest.java
+++ b/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