ZOOKEEPER-3531: Synchronization on ACLCache cause cluster to hang whe…

…n network/disk issues happen during datatree serialization

Author: Mcfatealan <louchang_new@163.com>

Reviewers: fangmin@apache.org, andor@apache.org

Closes #1077 from mcfatealan/ZOOKEEPER-3531 and squashes the following commits:

60109185e [Mcfatealan] minor
951dd5cac [Mcfatealan] fix checkstyle issue
96ea4a066 [Mcfatealan] finer-synchronization for deserialize
b79446a6f [Mcfatealan] minor fix for checkstyle
d5f7d04ab [Mcfatealan] ZOOKEEPER-3531: Synchronization on ACLCache cause cluster to hang when network/disk issues happen during datatree serialization

zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java

@@ -22,9 +22,9 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 import org.apache.jute.Index;
 import org.apache.jute.InputArchive;
@@ -99,14 +99,14 @@ public class ReferenceCountedACLCache {
         return ++aclIndex;
     }
 
-    public synchronized void deserialize(InputArchive ia) throws IOException {
+    public void deserialize(InputArchive ia) throws IOException {
         clear();
         int i = ia.readInt("map");
+
+        LinkedHashMap<Long, List<ACL>> deserializedMap = new LinkedHashMap<>();
+        // keep read operations out of synchronization block
         while (i > 0) {
             Long val = ia.readLong("long");
-            if (aclIndex < val) {
-                aclIndex = val;
-            }
             List<ACL> aclList = new ArrayList<ACL>();
             Index j = ia.startVector("acls");
             if (j == null) {
@@ -118,17 +118,33 @@ public class ReferenceCountedACLCache {
                 aclList.add(acl);
                 j.incr();
             }
-            longKeyMap.put(val, aclList);
-            aclKeyMap.put(aclList, val);
-            referenceCounter.put(val, new AtomicLongWithEquals(0));
+
+            deserializedMap.put(val, aclList);
             i--;
         }
+
+        synchronized (this) {
+            for (Map.Entry<Long, List<ACL>> entry : deserializedMap.entrySet()) {
+                Long val = entry.getKey();
+                List<ACL> aclList = entry.getValue();
+                if (aclIndex < val) {
+                    aclIndex = val;
+                }
+
+                longKeyMap.put(val, aclList);
+                aclKeyMap.put(aclList, val);
+                referenceCounter.put(val, new AtomicLongWithEquals(0));
+            }
+        }
     }
 
-    public synchronized void serialize(OutputArchive oa) throws IOException {
-        oa.writeInt(longKeyMap.size(), "map");
-        Set<Map.Entry<Long, List<ACL>>> set = longKeyMap.entrySet();
-        for (Map.Entry<Long, List<ACL>> val : set) {
+    public void serialize(OutputArchive oa) throws IOException {
+        Map<Long, List<ACL>> clonedLongKeyMap;
+        synchronized (this) {
+            clonedLongKeyMap = new HashMap<>(longKeyMap);
+        }
+        oa.writeInt(clonedLongKeyMap.size(), "map");
+        for (Map.Entry<Long, List<ACL>> val : clonedLongKeyMap.entrySet()) {
             oa.writeLong(val.getKey(), "long");
             List<ACL> aclList = val.getValue();
             oa.startVector(aclList, "acls");

zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java

@@ -24,6 +24,7 @@ import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.DataInputStream;
 import java.io.DataOutputStream;
 import java.io.IOException;
 import java.io.PrintWriter;
@@ -253,6 +254,106 @@ public class DataTreeTest extends ZKTestCase {
         assertEquals("/bug is still in pTrie", "/", pTrie.findMaxPrefix("/bug"));
     }
 
+
+    /* ZOOKEEPER-3531 - org.apache.zookeeper.server.DataTree#serialize calls the aclCache.serialize when doing
+     * dataree serialization, however, org.apache.zookeeper.server.ReferenceCountedACLCache#serialize
+     * could get stuck at OutputArchieve.writeInt due to potential network/disk issues.
+     * This can cause the system experiences hanging issues similar to ZooKeeper-2201.
+     * This test verifies the fix that we should not hold ACL cache during dumping aclcache to snapshots
+    */
+    @Test(timeout = 60000)
+    public void testSerializeDoesntLockACLCacheWhileWriting() throws Exception {
+        DataTree tree = new DataTree();
+        tree.createNode("/marker", new byte[]{42}, null, -1, 1, 1, 1);
+        final AtomicBoolean ranTestCase = new AtomicBoolean();
+        DataOutputStream out = new DataOutputStream(new ByteArrayOutputStream());
+        BinaryOutputArchive oa = new BinaryOutputArchive(out) {
+            @Override
+            public void writeInt(int size, String tag) throws IOException {
+                final Semaphore semaphore = new Semaphore(0);
+
+                new Thread(new Runnable() {
+                    @Override
+                    public void run() {
+
+                        synchronized (tree.getReferenceCountedAclCache()) {
+                            //When we lock ACLCache, allow writeRecord to continue
+                            semaphore.release();
+                        }
+                    }
+                }).start();
+
+                try {
+                    boolean acquired = semaphore.tryAcquire(30, TimeUnit.SECONDS);
+                    //This is the real assertion - could another thread lock
+                    //the ACLCache
+                    assertTrue("Couldn't acquire a lock on the ACLCache while we were calling tree.serialize", acquired);
+                } catch (InterruptedException e1) {
+                    throw new RuntimeException(e1);
+                }
+                ranTestCase.set(true);
+
+                super.writeInt(size, tag);
+            }
+        };
+
+        tree.serialize(oa, "test");
+
+        //Let's make sure that we hit the code that ran the real assertion above
+        assertTrue("Didn't find the expected node", ranTestCase.get());
+    }
+
+    /* ZOOKEEPER-3531 - similarly for aclCache.deserialize, we should not hold lock either
+    */
+    @Test(timeout = 60000)
+    public void testDeserializeDoesntLockACLCacheWhileReading() throws Exception {
+        DataTree tree = new DataTree();
+        tree.createNode("/marker", new byte[]{42}, null, -1, 1, 1, 1);
+        final AtomicBoolean ranTestCase = new AtomicBoolean();
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        DataOutputStream out = new DataOutputStream(baos);
+        BinaryOutputArchive oa = new BinaryOutputArchive(out);
+
+        tree.serialize(oa, "test");
+
+        DataTree tree2 = new DataTree();
+        DataInputStream in = new DataInputStream(new ByteArrayInputStream(baos.toByteArray()));
+        BinaryInputArchive ia = new BinaryInputArchive(in) {
+            @Override
+            public long readLong(String tag) throws IOException {
+                final Semaphore semaphore = new Semaphore(0);
+
+                new Thread(new Runnable() {
+                    @Override
+                    public void run() {
+
+                        synchronized (tree2.getReferenceCountedAclCache()) {
+                            //When we lock ACLCache, allow readLong to continue
+                            semaphore.release();
+                        }
+                    }
+                }).start();
+
+                try {
+                    boolean acquired = semaphore.tryAcquire(30, TimeUnit.SECONDS);
+                    //This is the real assertion - could another thread lock
+                    //the ACLCache
+                    assertTrue("Couldn't acquire a lock on the ACLCache while we were calling tree.deserialize", acquired);
+                } catch (InterruptedException e1) {
+                    throw new RuntimeException(e1);
+                }
+                ranTestCase.set(true);
+
+                return super.readLong(tag);
+            }
+        };
+
+        tree2.deserialize(ia, "test");
+
+        //Let's make sure that we hit the code that ran the real assertion above
+        assertTrue("Didn't find the expected node", ranTestCase.get());
+    }
+
     /*
      * ZOOKEEPER-2201 - OutputArchive.writeRecord can block for long periods of
      * time, we must call it outside of the node lock.