[Fix] Common 修复潜在的并发问题, 修复测试用例不严谨的问题;

[Fix] CacheStoreBuilder 修复潜在的因并发锁错误导致的重载崩溃的问题;
[Fix] CacheStoreBuilderTest 调整测试用例, 以确保多线程重载获取的问题不会被捕获;

ContentGrabbingJi-common/src/main/java/net/lamgc/cgj/bot/cache/CacheStoreBuilder.java

@@ -42,10 +42,12 @@ public final class CacheStoreBuilder {
     private final static Logger log = LoggerFactory.getLogger(CacheStoreBuilder.class);
 
     private volatile List<CacheStoreFactory> factoryList;
-    private final Map<CacheStoreFactory, FactoryInfo> factoryInfoMap = new Hashtable<>();
+    private volatile Map<CacheStoreFactory, FactoryInfo> factoryInfoMap;
     private final ServiceLoader<CacheStoreFactory> factoryLoader = ServiceLoader.load(CacheStoreFactory.class);
     private final File dataDirectory;
 
+    private final Object getFactoryLock = new Object();
+
     /**
      * 获取 CacheStoreBuilder 实例.
      * <p> 该方法仅提供给 ContentGrabbingJiBot 调用, 请勿通过该方法私自创建 CacheStoreBuilder.
@@ -83,22 +85,22 @@ public final class CacheStoreBuilder {
     private synchronized void loadFactory() {
         factoryLoader.reload();
         List<CacheStoreFactory> newFactoryList = new ArrayList<>();
+        Map<CacheStoreFactory, FactoryInfo> newFactoryInfoMap = new HashMap<>(8);
         try {
             for (CacheStoreFactory factory : factoryLoader) {
                 FactoryInfo info;
                 try {
                     info = new FactoryInfo(factory.getClass());
-                    if (factoryInfoMap.containsValue(info)) {
+                    if (newFactoryInfoMap.containsValue(info)) {
                         log.warn("发现 Name 重复的 Factory, 已跳过. (被拒绝的实现: {})", factory.getClass().getName());
                         continue;
                     }
-                    factoryInfoMap.put(factory, info);
+                    newFactoryInfoMap.put(factory, info);
                 } catch (IllegalArgumentException e) {
                     log.warn("Factory {} 加载失败: {}", factory.getClass().getName(), e.getMessage());
                     continue;
                 }
 
-
                 if (!initialFactory(factory, info)) {
                     log.warn("Factory {} 初始化失败.", info.getFactoryName());
                     continue;
@@ -109,9 +111,11 @@ public final class CacheStoreBuilder {
                         info.getFactoryPriority(),
                         factory.getClass().getName());
             }
-            newFactoryList.sort(new PriorityComparator());
+            newFactoryList.sort(new PriorityComparator(newFactoryInfoMap));
-            factoryList = newFactoryList;
+            synchronized (getFactoryLock) {
-            optimizeFactoryInfoMap();
+                this.factoryList = newFactoryList;
+                this.factoryInfoMap = newFactoryInfoMap;
+            }
         } catch (Error error) {
             // 防止发生 Error 又不输出到日志导致玄学问题难以排查.
             log.error("加载 CacheStoreFactory 时发生严重错误.", error);
@@ -119,13 +123,6 @@ public final class CacheStoreBuilder {
         }
     }
 
-    /**
-     * 清除无效的 {@link FactoryInfo}
-     */
-    private void optimizeFactoryInfoMap() {
-        factoryInfoMap.keySet().removeIf(factory -> !factoryList.contains(factory));
-    }
-
     /**
      * 初始化 Factory.
      * @param factory Factory 对象.
@@ -151,7 +148,14 @@ public final class CacheStoreBuilder {
     /**
      * 优先级排序器.
      */
-    private final class PriorityComparator implements Comparator<CacheStoreFactory> {
+    private static final class PriorityComparator implements Comparator<CacheStoreFactory> {
+
+        private final Map<CacheStoreFactory, FactoryInfo> factoryInfoMap;
+
+        private PriorityComparator(Map<CacheStoreFactory, FactoryInfo> factoryInfoMap) {
+            this.factoryInfoMap = factoryInfoMap;
+        }
+
         @Override
         public int compare(CacheStoreFactory o1, CacheStoreFactory o2) {
             FactoryInfo info1 = Objects.requireNonNull(factoryInfoMap.get(o1));
@@ -167,10 +171,15 @@ public final class CacheStoreBuilder {
     private <R extends CacheStore<?>> R getFactory(CacheStoreSource storeSource,
                                                    Function<CacheStoreFactory, R> function)
     throws NoSuchFactoryException {
-        Iterator<CacheStoreFactory> iterator = factoryList.iterator();
+        Map<CacheStoreFactory, FactoryInfo> localFactoryInfoMap;
+        Iterator<CacheStoreFactory> iterator;
+        synchronized (getFactoryLock) {
+            localFactoryInfoMap = this.factoryInfoMap;
+            iterator = factoryList.iterator();
+        }
         while (iterator.hasNext()) {
             CacheStoreFactory factory = iterator.next();
-            FactoryInfo info = factoryInfoMap.get(factory);
+            FactoryInfo info = localFactoryInfoMap.get(factory);
             if (storeSource != null && info.getStoreSource() != storeSource) {
                 continue;
             }

ContentGrabbingJi-common/src/test/java/net/lamgc/cgj/bot/cache/CacheStoreBuilderTest.java

@@ -17,7 +17,6 @@
 
 package net.lamgc.cgj.bot.cache;
 
-import com.google.common.base.Throwables;
 import net.lamgc.cgj.bot.cache.convert.StringConverter;
 import net.lamgc.cgj.bot.cache.convert.StringToStringConverter;
 import net.lamgc.cgj.bot.cache.exception.GetCacheStoreException;
@@ -38,6 +37,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Random;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * @see CacheStoreBuilder
@@ -48,25 +48,15 @@ public class CacheStoreBuilderTest {
     private final static Logger log = LoggerFactory.getLogger(CacheStoreBuilderTest.class);
 
     @BeforeClass
-    public static void beforeAction() {
+    public static void beforeAction() throws IOException {
-        try {
         tempDirectory.create();
-        } catch (IOException e) {
-            Assert.fail(Throwables.getStackTraceAsString(e));
-        }
     }
 
     @Test
-    public void getCacheStoreTest() {
+    public void getCacheStoreTest() throws IOException {
         final String identify = "test";
         final StringConverter<String> converter = new StringToStringConverter();
-        CacheStoreBuilder cacheStoreBuilder;
+        CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        try {
-            cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        } catch (IOException e) {
-            Assert.fail(Throwables.getStackTraceAsString(e));
-            return;
-        }
 
         SingleCacheStore<String> singleCacheStore = cacheStoreBuilder.newSingleCacheStore(CacheStoreSource.REMOTE, identify, converter);
         Assert.assertNotNull(singleCacheStore);
@@ -86,14 +76,8 @@ public class CacheStoreBuilderTest {
     }
 
     @Test
-    public void loadFailureTest() throws IllegalAccessException, NoSuchFieldException {
+    public void loadFailureTest() throws IllegalAccessException, NoSuchFieldException, IOException {
-        CacheStoreBuilder cacheStoreBuilder;
+        CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        try {
-            cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        } catch (IOException e) {
-            Assert.fail(Throwables.getStackTraceAsString(e));
-            return;
-        }
 
         Field factoryListField;
         factoryListField = CacheStoreBuilder.class.getDeclaredField("factoryList");
@@ -129,12 +113,14 @@ public class CacheStoreBuilderTest {
         final String identify = "test";
         final StringConverter<String> converter = new StringToStringConverter();
         final CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
+        final AtomicBoolean uncaughtExceptionFlag = new AtomicBoolean();
+        final int totalCount = 100000;
 
         final Method loadFactoryMethod = CacheStoreBuilder.class.getDeclaredMethod("loadFactory");
         loadFactoryMethod.setAccessible(true);
 
-        Thread accessThread = new Thread(() -> {
+        Thread accessThreadA = new Thread(() -> {
-            for (int i = 0; i < 100000; i++) {
+            for (int i = 0; i < totalCount; i++) {
                 SingleCacheStore<String> singleCacheStore = cacheStoreBuilder.newSingleCacheStore(CacheStoreSource.REMOTE, identify, converter);
                 Assert.assertNotNull(singleCacheStore);
                 Assert.assertEquals(RemoteCacheFactory.RemoteSingleCacheFactory.class, singleCacheStore.getClass());
@@ -151,11 +137,11 @@ public class CacheStoreBuilderTest {
                 Assert.assertNotNull(setCacheStore);
                 Assert.assertEquals(SetCacheStoreFactory.OnlySetCacheStore.class, setCacheStore.getClass());
             }
-        }, "Thread-AccessBuilder");
+        }, "Thread-AccessBuilderA");
-        Thread reloadThread = new Thread(() -> {
+        Thread reloadThreadA = new Thread(() -> {
             int count = 0;
             final Random random = new Random();
-            while(count++ < 100000) {
+            while(count++ < totalCount) {
                 if (random.nextInt() % 2 == 0) {
                     try {
                         loadFactoryMethod.invoke(cacheStoreBuilder);
@@ -167,26 +153,77 @@ public class CacheStoreBuilderTest {
                     }
                 }
             }
-        }, "Thread-ReloadBuilder");
+        }, "Thread-ReloadBuilderA");
+        Thread accessThreadB = new Thread(() -> {
+            for (int i = 0; i < totalCount; i++) {
+                SingleCacheStore<String> singleCacheStore = cacheStoreBuilder.newSingleCacheStore(CacheStoreSource.REMOTE, identify, converter);
+                Assert.assertNotNull(singleCacheStore);
+                Assert.assertEquals(RemoteCacheFactory.RemoteSingleCacheFactory.class, singleCacheStore.getClass());
 
-        accessThread.start();
+                ListCacheStore<String> listCacheStore = cacheStoreBuilder.newListCacheStore(CacheStoreSource.MEMORY, identify, converter);
-        reloadThread.start();
+                Assert.assertNotNull(listCacheStore);
+                Assert.assertEquals(MemoryFactory.MemoryListCacheStore.class, listCacheStore.getClass());
 
-        accessThread.join();
+                MapCacheStore<String> mapCacheStore = cacheStoreBuilder.newMapCacheStore(CacheStoreSource.LOCAL, identify, converter);
-        reloadThread.join();
+                Assert.assertNotNull(mapCacheStore);
+                Assert.assertEquals(LocalFactory.LocalMapCacheStore.class, mapCacheStore.getClass());
+
+                SetCacheStore<String> setCacheStore = cacheStoreBuilder.newSetCacheStore(identify, converter);
+                Assert.assertNotNull(setCacheStore);
+                Assert.assertEquals(SetCacheStoreFactory.OnlySetCacheStore.class, setCacheStore.getClass());
+            }
+        }, "Thread-AccessBuilderB");
+        Thread reloadThreadB = new Thread(() -> {
+            int count = 0;
+            final Random random = new Random();
+            while(count++ < totalCount) {
+                if (random.nextInt() % 2 == 0) {
+                    try {
+                        loadFactoryMethod.invoke(cacheStoreBuilder);
+                    } catch (IllegalAccessException | InvocationTargetException e) {
+                        log.error("重载 Builder 时发生异常.",
+                                e instanceof InvocationTargetException ?
+                                        ((InvocationTargetException) e).getTargetException() :
+                                        e);
+                    }
+                }
+            }
+        }, "Thread-ReloadBuilderB");
+
+        class TestUncaughtExceptionHandler implements Thread.UncaughtExceptionHandler {
+
+            @Override
+            public void uncaughtException(Thread t, Throwable e) {
+                log.error("An uncapped exception occurred in thread " + t.getName(), e);
+                uncaughtExceptionFlag.set(true);
+            }
+        }
+
+        accessThreadA.setUncaughtExceptionHandler(new TestUncaughtExceptionHandler());
+        reloadThreadA.setUncaughtExceptionHandler(new TestUncaughtExceptionHandler());
+        accessThreadB.setUncaughtExceptionHandler(new TestUncaughtExceptionHandler());
+        reloadThreadB.setUncaughtExceptionHandler(new TestUncaughtExceptionHandler());
+
+        accessThreadA.start();
+        reloadThreadA.start();
+        accessThreadB.start();
+        reloadThreadB.start();
+
+        accessThreadA.join();
+        reloadThreadA.join();
+        accessThreadB.join();
+        reloadThreadB.join();
+
+        if (uncaughtExceptionFlag.get()) {
+            Assert.fail("Exception occurred while multithreading reload");
+        }
     }
 
     @Test
-    public void noSpecifiedGetCacheStoreTest() {
+    public void noSpecifiedGetCacheStoreTest() throws IOException {
         final String identify = "test";
         final StringConverter<String> converter = new StringToStringConverter();
-        CacheStoreBuilder cacheStoreBuilder;
+        CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        try {
-            cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        } catch (IOException e) {
-            Assert.fail(Throwables.getStackTraceAsString(e));
-            return;
-        }
 
         SingleCacheStore<String> singleCacheStore = cacheStoreBuilder.newSingleCacheStore(identify, converter);
         Assert.assertNotNull(singleCacheStore);
@@ -202,16 +239,10 @@ public class CacheStoreBuilderTest {
     }
 
     @Test
-    public void noSuchFactoryExceptionThrowTest() throws NoSuchFieldException, IllegalAccessException {
+    public void noSuchFactoryExceptionThrowTest() throws NoSuchFieldException, IllegalAccessException, IOException {
         final String identify = "test";
         final StringConverter<String> converter = new StringToStringConverter();
-        CacheStoreBuilder cacheStoreBuilder;
+        CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        try {
-            cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        } catch (IOException e) {
-            Assert.fail(Throwables.getStackTraceAsString(e));
-            return;
-        }
 
         Field factoryListField;
         factoryListField = CacheStoreBuilder.class.getDeclaredField("factoryList");
@@ -275,16 +306,10 @@ public class CacheStoreBuilderTest {
     }
 
     @Test
-    public void lastFactoryThrowExceptionTest() throws NoSuchFieldException, IllegalAccessException {
+    public void lastFactoryThrowExceptionTest() throws NoSuchFieldException, IllegalAccessException, IOException {
         final String identify = "test";
         final StringConverter<String> converter = new StringToStringConverter();
-        CacheStoreBuilder cacheStoreBuilder;
+        CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        try {
-            cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot());
-        } catch (IOException e) {
-            Assert.fail(Throwables.getStackTraceAsString(e));
-            return;
-        }
 
         Field factoryListField;
         factoryListField = CacheStoreBuilder.class.getDeclaredField("factoryList");
@@ -330,6 +355,4 @@ public class CacheStoreBuilderTest {
         }
     }
 
-
-
 }