Race in LookPool may cause used Resources to be evicted

Author: ZanderXu

dora/core/common/src/main/java/alluxio/collections/LockPool.java

@@ -16,6 +16,7 @@
 import alluxio.resource.LockResource;
 import alluxio.resource.RWLockResource;
 import alluxio.resource.RefCountLockResource;
+import alluxio.util.AlluxioFaultInjector;
 import alluxio.util.ThreadFactoryUtils;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -165,8 +166,12 @@ private void awaitAndEvict() throws InterruptedException {
             candidate.mIsAccessed = false;
           } else {
             if (candidate.mRefCount.compareAndSet(0, Integer.MIN_VALUE)) {
-              mIterator.remove();
-              numToEvict--;
+              AlluxioFaultInjector.get().blockUtilAllocatedNewResource();
+              Resource tmpResource = mPool.compute(candidateMapEntry.getKey(),
+                  (k, v) -> v == candidate ? null : v);
+              if (tmpResource == null) {
+                numToEvict--;
+              }
             }
           }
         }

dora/core/common/src/main/java/alluxio/util/AlluxioFaultInjector.java

@@ -0,0 +1,38 @@
+/*
+ * The Alluxio Open Foundation licenses this work under the Apache License, version 2.0
+ * (the "License"). You may not use this work except in compliance with the License, which is
+ * available at www.apache.org/licenses/LICENSE-2.0
+ *
+ * This software is distributed on an "AS IS" basis, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
+ * either express or implied, as more fully set forth in the License.
+ *
+ * See the NOTICE file distributed with this work for information regarding copyright ownership.
+ */
+
+package alluxio.util;
+
+import com.google.common.annotations.VisibleForTesting;
+
+/**
+ * Used for injecting faults in Alluxio tests.
+ * Calls into this are a no-op in production code.
+ */
+@VisibleForTesting
+public class AlluxioFaultInjector {
+  private static AlluxioFaultInjector instance = new AlluxioFaultInjector();
+
+  public static AlluxioFaultInjector get() {
+    return instance;
+  }
+
+  public static void set(AlluxioFaultInjector injector) {
+    instance = injector;
+  }
+
+  /**
+   * Used as a hook to inject intercept when evicting unused resource from pool.
+   */
+  public void blockUtilAllocatedNewResource() {
+
+  }
+}

dora/core/common/src/test/java/alluxio/collections/LockPoolTest.java

@@ -16,12 +16,16 @@
 
 import alluxio.concurrent.LockMode;
 import alluxio.resource.LockResource;
+import alluxio.util.AlluxioFaultInjector;
 import alluxio.util.CommonUtils;
 
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
@@ -141,4 +145,53 @@ public void referencedLockTest() {
     assertTrue(lock1.hasSameLock(mPool.get(50, LockMode.READ)));
     assertTrue(lock2.hasSameLock(mPool.get(100, LockMode.READ)));
   }
+
+  @Test(timeout = 10000)
+  public void evictorTest() throws InterruptedException {
+    List<LockResource> unClosedLocks = new ArrayList<>();
+    CountDownLatch mainThreadDownLatch = new CountDownLatch(1);
+    CountDownLatch evictorDownLatch = new CountDownLatch(1);
+    AlluxioFaultInjector.set(new AlluxioFaultInjector() {
+      public void blockUtilAllocatedNewResource() {
+        try {
+          mainThreadDownLatch.countDown();
+          evictorDownLatch.await();
+        } catch (InterruptedException e) {
+          throw new RuntimeException(e);
+        }
+      }
+    });
+
+    // Acquire 16 locks and just release the lock-3.
+    for (int i = 1; i <= 16; i++) {
+      LockResource lock = mPool.get(i, LockMode.READ);
+      if (i == 3) {
+        lock.close();
+      } else {
+        unClosedLocks.add(lock);
+      }
+    }
+    assertEquals(16, mPool.size());
+
+    // Acquire a new Lock to trigger evictor. And the Evictor will release the
+    // unused Resource-3, but it will be blocked by evictorDownLatch.
+    LockResource lock17 = mPool.get(17, LockMode.READ);
+    unClosedLocks.add(lock17);
+
+    // Acquire a resource for key 3 again, currently the Evictor is releasing
+    // the old Resource-3, but it is blocked right now.
+    mainThreadDownLatch.await();
+    LockResource lock3 = mPool.get(3, LockMode.READ);
+    unClosedLocks.add(lock3);
+
+    // Allow the evictor to continue.
+    evictorDownLatch.countDown();
+
+    // Sleep some millis to wait for the Evictor.
+    Thread.sleep(100);
+
+    // The Resource 3 should be still in the mPool.
+    assertEquals(17, mPool.size());
+    unClosedLocks.forEach(k -> k.close());
+  }
 }