From 76371b3257ede0a1f63de69927f7a518ee75dc1b Mon Sep 17 00:00:00 2001 From: LamGC Date: Thu, 19 Nov 2020 22:01:37 +0800 Subject: [PATCH] =?UTF-8?q?[Fix]=20Common=20=E4=BF=AE=E5=A4=8D=E6=BD=9C?= =?UTF-8?q?=E5=9C=A8=E7=9A=84=E5=B9=B6=E5=8F=91=E9=97=AE=E9=A2=98,=20?= =?UTF-8?q?=E4=BF=AE=E5=A4=8D=E6=B5=8B=E8=AF=95=E7=94=A8=E4=BE=8B=E4=B8=8D?= =?UTF-8?q?=E4=B8=A5=E8=B0=A8=E7=9A=84=E9=97=AE=E9=A2=98;?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [Fix] CacheStoreBuilder 修复潜在的因并发锁错误导致的重载崩溃的问题; [Fix] CacheStoreBuilderTest 调整测试用例, 以确保多线程重载获取的问题不会被捕获; --- .../cgj/bot/cache/CacheStoreBuilder.java | 43 +++--- .../cgj/bot/cache/CacheStoreBuilderTest.java | 141 ++++++++++-------- 2 files changed, 108 insertions(+), 76 deletions(-) diff --git a/ContentGrabbingJi-common/src/main/java/net/lamgc/cgj/bot/cache/CacheStoreBuilder.java b/ContentGrabbingJi-common/src/main/java/net/lamgc/cgj/bot/cache/CacheStoreBuilder.java index cdbbffd..a51229b 100644 --- a/ContentGrabbingJi-common/src/main/java/net/lamgc/cgj/bot/cache/CacheStoreBuilder.java +++ b/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 factoryList; - private final Map factoryInfoMap = new Hashtable<>(); + private volatile Map factoryInfoMap; private final ServiceLoader factoryLoader = ServiceLoader.load(CacheStoreFactory.class); private final File dataDirectory; + private final Object getFactoryLock = new Object(); + /** * 获取 CacheStoreBuilder 实例. *

该方法仅提供给 ContentGrabbingJiBot 调用, 请勿通过该方法私自创建 CacheStoreBuilder. @@ -83,22 +85,22 @@ public final class CacheStoreBuilder { private synchronized void loadFactory() { factoryLoader.reload(); List newFactoryList = new ArrayList<>(); + Map 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()); - factoryList = newFactoryList; - optimizeFactoryInfoMap(); + newFactoryList.sort(new PriorityComparator(newFactoryInfoMap)); + synchronized (getFactoryLock) { + 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 { + private static final class PriorityComparator implements Comparator { + + private final Map factoryInfoMap; + + private PriorityComparator(Map 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 getFactory(CacheStoreSource storeSource, Function function) throws NoSuchFactoryException { - Iterator iterator = factoryList.iterator(); + Map localFactoryInfoMap; + Iterator 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; } diff --git a/ContentGrabbingJi-common/src/test/java/net/lamgc/cgj/bot/cache/CacheStoreBuilderTest.java b/ContentGrabbingJi-common/src/test/java/net/lamgc/cgj/bot/cache/CacheStoreBuilderTest.java index 3cbc801..ad38f6a 100644 --- a/ContentGrabbingJi-common/src/test/java/net/lamgc/cgj/bot/cache/CacheStoreBuilderTest.java +++ b/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() { - try { - tempDirectory.create(); - } catch (IOException e) { - Assert.fail(Throwables.getStackTraceAsString(e)); - } + public static void beforeAction() throws IOException { + tempDirectory.create(); } @Test - public void getCacheStoreTest() { + public void getCacheStoreTest() throws IOException { final String identify = "test"; final StringConverter converter = new StringToStringConverter(); - CacheStoreBuilder cacheStoreBuilder; - try { - cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); - } catch (IOException e) { - Assert.fail(Throwables.getStackTraceAsString(e)); - return; - } + CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); SingleCacheStore singleCacheStore = cacheStoreBuilder.newSingleCacheStore(CacheStoreSource.REMOTE, identify, converter); Assert.assertNotNull(singleCacheStore); @@ -86,14 +76,8 @@ public class CacheStoreBuilderTest { } @Test - public void loadFailureTest() throws IllegalAccessException, NoSuchFieldException { - CacheStoreBuilder cacheStoreBuilder; - try { - cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); - } catch (IOException e) { - Assert.fail(Throwables.getStackTraceAsString(e)); - return; - } + public void loadFailureTest() throws IllegalAccessException, NoSuchFieldException, IOException { + CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); Field factoryListField; factoryListField = CacheStoreBuilder.class.getDeclaredField("factoryList"); @@ -129,12 +113,14 @@ public class CacheStoreBuilderTest { final String identify = "test"; final StringConverter 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(() -> { - for (int i = 0; i < 100000; i++) { + Thread accessThreadA = new Thread(() -> { + for (int i = 0; i < totalCount; i++) { SingleCacheStore 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 reloadThread = new Thread(() -> { + }, "Thread-AccessBuilderA"); + 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 singleCacheStore = cacheStoreBuilder.newSingleCacheStore(CacheStoreSource.REMOTE, identify, converter); + Assert.assertNotNull(singleCacheStore); + Assert.assertEquals(RemoteCacheFactory.RemoteSingleCacheFactory.class, singleCacheStore.getClass()); - accessThread.start(); - reloadThread.start(); + ListCacheStore listCacheStore = cacheStoreBuilder.newListCacheStore(CacheStoreSource.MEMORY, identify, converter); + Assert.assertNotNull(listCacheStore); + Assert.assertEquals(MemoryFactory.MemoryListCacheStore.class, listCacheStore.getClass()); - accessThread.join(); - reloadThread.join(); + MapCacheStore mapCacheStore = cacheStoreBuilder.newMapCacheStore(CacheStoreSource.LOCAL, identify, converter); + Assert.assertNotNull(mapCacheStore); + Assert.assertEquals(LocalFactory.LocalMapCacheStore.class, mapCacheStore.getClass()); + + SetCacheStore 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 converter = new StringToStringConverter(); - CacheStoreBuilder cacheStoreBuilder; - try { - cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); - } catch (IOException e) { - Assert.fail(Throwables.getStackTraceAsString(e)); - return; - } + CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); SingleCacheStore 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 converter = new StringToStringConverter(); - CacheStoreBuilder cacheStoreBuilder; - try { - cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); - } catch (IOException e) { - Assert.fail(Throwables.getStackTraceAsString(e)); - return; - } + CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); 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 converter = new StringToStringConverter(); - CacheStoreBuilder cacheStoreBuilder; - try { - cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); - } catch (IOException e) { - Assert.fail(Throwables.getStackTraceAsString(e)); - return; - } + CacheStoreBuilder cacheStoreBuilder = CacheStoreBuilder.getInstance(tempDirectory.getRoot()); Field factoryListField; factoryListField = CacheStoreBuilder.class.getDeclaredField("factoryList"); @@ -330,6 +355,4 @@ public class CacheStoreBuilderTest { } } - - }