diff --git a/src/main/java/run/halo/app/extension/DefaultSchemeManager.java b/src/main/java/run/halo/app/extension/DefaultSchemeManager.java index b21685700..85e03b2fe 100644 --- a/src/main/java/run/halo/app/extension/DefaultSchemeManager.java +++ b/src/main/java/run/halo/app/extension/DefaultSchemeManager.java @@ -1,9 +1,9 @@ package run.halo.app.extension; import java.util.Collections; -import java.util.LinkedList; import java.util.List; import java.util.Optional; +import java.util.concurrent.CopyOnWriteArrayList; import org.springframework.lang.NonNull; import org.springframework.lang.Nullable; import run.halo.app.extension.SchemeWatcherManager.SchemeRegistered; @@ -18,7 +18,9 @@ public class DefaultSchemeManager implements SchemeManager { public DefaultSchemeManager(@Nullable SchemeWatcherManager watcherManager) { this.watcherManager = watcherManager; - schemes = new LinkedList<>(); + // we have to use CopyOnWriteArrayList at here to prevent concurrent modification between + // registering and listing. + schemes = new CopyOnWriteArrayList<>(); } @Override diff --git a/src/main/java/run/halo/app/extension/DefaultSchemeWatcherManager.java b/src/main/java/run/halo/app/extension/DefaultSchemeWatcherManager.java index 2ec86edd1..485217065 100644 --- a/src/main/java/run/halo/app/extension/DefaultSchemeWatcherManager.java +++ b/src/main/java/run/halo/app/extension/DefaultSchemeWatcherManager.java @@ -1,7 +1,8 @@ package run.halo.app.extension; -import java.util.LinkedList; +import java.util.Collections; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import org.springframework.lang.NonNull; import org.springframework.util.Assert; @@ -10,7 +11,7 @@ public class DefaultSchemeWatcherManager implements SchemeWatcherManager { private final List watchers; public DefaultSchemeWatcherManager() { - watchers = new LinkedList<>(); + watchers = new CopyOnWriteArrayList<>(); } @Override @@ -28,6 +29,6 @@ public class DefaultSchemeWatcherManager implements SchemeWatcherManager { @Override public List watchers() { // we have to copy the watchers entirely to prevent concurrent modification. - return List.copyOf(watchers); + return Collections.unmodifiableList(watchers); } } diff --git a/src/main/java/run/halo/app/extension/controller/DefaultController.java b/src/main/java/run/halo/app/extension/controller/DefaultController.java index 5882d9b1b..724806a2c 100644 --- a/src/main/java/run/halo/app/extension/controller/DefaultController.java +++ b/src/main/java/run/halo/app/extension/controller/DefaultController.java @@ -1,12 +1,15 @@ package run.halo.app.extension.controller; +import static java.util.concurrent.Executors.newSingleThreadExecutor; + import java.time.Duration; import java.time.Instant; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.function.Supplier; import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.concurrent.BasicThreadFactory; import org.springframework.util.StopWatch; import run.halo.app.extension.controller.RequestQueue.DelayedEntry; @@ -68,7 +71,16 @@ public class DefaultController implements Controller { Duration minDelay, Duration maxDelay) { this(name, reconciler, queue, synchronizer, nowSupplier, minDelay, maxDelay, - Executors.newSingleThreadExecutor()); + newSingleThreadExecutor(threadFactory())); + } + + private static ThreadFactory threadFactory() { + return new BasicThreadFactory.Builder() + .namingPattern("reconciler-thread-%d") + .daemon(false) + .uncaughtExceptionHandler((t, e) -> + log.error("Controller " + t.getName() + " encountered an error unexpectedly", e)) + .build(); } @Override diff --git a/src/test/java/run/halo/app/extension/DefaultSchemeManagerTest.java b/src/test/java/run/halo/app/extension/DefaultSchemeManagerTest.java index bb6a90d2a..f00f58c5f 100644 --- a/src/test/java/run/halo/app/extension/DefaultSchemeManagerTest.java +++ b/src/test/java/run/halo/app/extension/DefaultSchemeManagerTest.java @@ -116,5 +116,20 @@ class DefaultSchemeManagerTest { schemeManager.unregister(schemeManager.get(FakeExtension.class)); assertEquals(0, schemeManager.size()); } + + @Test + void shouldReturnCopyOnWriteList() { + schemeManager.register(FakeExtension.class); + var schemes = schemeManager.schemes(); + schemes.forEach(scheme -> { + // make sure concurrent modification won't happen + schemeManager.register(FooExtension.class); + }); + } + + @GVK(group = "fake.halo.run", version = "v1alpha1", kind = "Foo", + plural = "foos", singular = "foo") + static class FooExtension extends AbstractExtension { + } } diff --git a/src/test/java/run/halo/app/extension/DefaultSchemeWatcherManagerTest.java b/src/test/java/run/halo/app/extension/DefaultSchemeWatcherManagerTest.java index 8016d2661..8e3ad08fe 100644 --- a/src/test/java/run/halo/app/extension/DefaultSchemeWatcherManagerTest.java +++ b/src/test/java/run/halo/app/extension/DefaultSchemeWatcherManagerTest.java @@ -49,15 +49,11 @@ class DefaultSchemeWatcherManagerTest { @Test void shouldReturnCopyOfWatchers() { - var watcher = mock(SchemeWatcher.class); - watcherManager.register(watcher); - assertEquals(List.of(watcher), watcherManager.watchers()); + var firstWatcher = mock(SchemeWatcher.class); + var secondWatcher = mock(SchemeWatcher.class); + watcherManager.register(firstWatcher); - var watchersBeforeRegister = watcherManager.watchers(); - watcherManager.unregister(watcher); - - // watchers are not changed even if unregistered - assertEquals(List.of(watcher), watchersBeforeRegister); - assertEquals(Collections.emptyList(), watcherManager.watchers()); + var watchers = watcherManager.watchers(); + watchers.forEach(watcher -> watcherManager.register(secondWatcher)); } } \ No newline at end of file