Use CopyOnWriteArrayList on SchemeManager (#2612)

#### What type of PR is this?

/kind bug
/area core
/milestone 2.0

#### What this PR does / why we need it:

Use CopyOnWriteArrayList on SchemeManager to prevent concurrent modification when link plugin is installed.

#### How to test?

1. Install link plugin
2. Restart Halo
3. Delete any extensions
4. Check the result

#### Does this PR introduce a user-facing change?

```release-note
修复数据一直处于删除中的错误
```
pull/2613/head^2
John Niang 2022-10-21 11:18:11 +08:00 committed by GitHub
parent e93f028a25
commit d765c2c329
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 42 additions and 16 deletions

View File

@ -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

View File

@ -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<SchemeWatcher> watchers;
public DefaultSchemeWatcherManager() {
watchers = new LinkedList<>();
watchers = new CopyOnWriteArrayList<>();
}
@Override
@ -28,6 +29,6 @@ public class DefaultSchemeWatcherManager implements SchemeWatcherManager {
@Override
public List<SchemeWatcher> watchers() {
// we have to copy the watchers entirely to prevent concurrent modification.
return List.copyOf(watchers);
return Collections.unmodifiableList(watchers);
}
}

View File

@ -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<R> 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

View File

@ -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 {
}
}

View File

@ -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));
}
}