From 08ff7e6e0016950ce9ce65a27ef0b475994903c1 Mon Sep 17 00:00:00 2001 From: John Niang Date: Wed, 29 May 2024 14:35:10 +0800 Subject: [PATCH] Fix the problem where extension point might not be obtained when the plugin was started (#6006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### What type of PR is this? /kind bug /area plugin /area core #### What this PR does / why we need it: This PR refactors SpringComponentsFinder to prevent unexpected cleanup, which might lead to not be able to obtain plugin extension when the plugin was started. The real reason is that entries is initialized by AttachmentReconciler during initialization of some plugins. Please note the problem cannot be reproduced stably. Refs: - https://github.com/pf4j/pf4j/blob/e3125f2998197d6f866abd1eb77a922103c1cace/pf4j/src/main/java/org/pf4j/AbstractExtensionFinder.java#L229-L249 - https://github.com/pf4j/pf4j/blob/e3125f2998197d6f866abd1eb77a922103c1cace/pf4j/src/main/java/org/pf4j/AbstractExtensionFinder.java#L312-L316 #### Which issue(s) this PR fixes: Fixes https://github.com/halo-dev/halo/issues/5999 #### Does this PR introduce a user-facing change? ```release-note 修复插件启动成功但仍然可能无法获取扩展点导致页面无法访问的问题 ``` --- .../app/plugin/SpringComponentsFinder.java | 81 +++++++----------- .../plugin/SpringComponentsFinderTest.java | 84 ++++++++++++++----- .../META-INF/plugin-components.idx | 2 + .../plugin/test-plugin-components.idx | 2 - 4 files changed, 96 insertions(+), 73 deletions(-) create mode 100644 application/src/test/resources/plugin/plugin-for-finder/META-INF/plugin-components.idx delete mode 100644 application/src/test/resources/plugin/test-plugin-components.idx diff --git a/application/src/main/java/run/halo/app/plugin/SpringComponentsFinder.java b/application/src/main/java/run/halo/app/plugin/SpringComponentsFinder.java index afd3c0e64..ff9f449b5 100644 --- a/application/src/main/java/run/halo/app/plugin/SpringComponentsFinder.java +++ b/application/src/main/java/run/halo/app/plugin/SpringComponentsFinder.java @@ -5,19 +5,17 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.io.Reader; import java.nio.charset.StandardCharsets; -import java.util.Collections; import java.util.HashSet; -import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.locks.StampedLock; +import java.util.concurrent.ConcurrentHashMap; import lombok.extern.slf4j.Slf4j; import org.pf4j.AbstractExtensionFinder; import org.pf4j.PluginManager; +import org.pf4j.PluginState; +import org.pf4j.PluginStateEvent; import org.pf4j.PluginWrapper; import org.pf4j.processor.ExtensionStorage; -import org.springframework.util.Assert; /** *

The spring component finder. it will read {@code META-INF/plugin-components.idx} file in @@ -31,53 +29,41 @@ import org.springframework.util.Assert; @Slf4j public class SpringComponentsFinder extends AbstractExtensionFinder { public static final String EXTENSIONS_RESOURCE = "META-INF/plugin-components.idx"; - private final StampedLock entryStampedLock = new StampedLock(); public SpringComponentsFinder(PluginManager pluginManager) { super(pluginManager); + entries = new ConcurrentHashMap<>(); } @Override public Map> readClasspathStorages() { - log.debug("Reading components storages from classpath"); - return Collections.emptyMap(); + throw new UnsupportedOperationException(); } @Override public Map> readPluginsStorages() { - // We have to copy the source code from `org.pf4j.LegacyExtensionFinder.readPluginsStorages` - // because we have to adapt to the new extensions resource location - // `META-INF/plugin-components.idx`. - log.debug("Reading components storages from plugins"); - Map> result = new LinkedHashMap<>(); + throw new UnsupportedOperationException(); + } - List plugins = pluginManager.getPlugins(); - for (PluginWrapper plugin : plugins) { - String pluginId = plugin.getDescriptor().getPluginId(); - log.debug("Reading extensions storage from plugin '{}'", pluginId); - Set bucket = new HashSet<>(); - - try { - log.debug("Read '{}'", EXTENSIONS_RESOURCE); - ClassLoader pluginClassLoader = plugin.getPluginClassLoader(); - try (var resourceStream = - pluginClassLoader.getResourceAsStream(EXTENSIONS_RESOURCE)) { - if (resourceStream == null) { - log.debug("Cannot find '{}'", EXTENSIONS_RESOURCE); - } else { - collectExtensions(resourceStream, bucket); - } + private Set readPluginStorage(PluginWrapper pluginWrapper) { + var pluginId = pluginWrapper.getPluginId(); + log.debug("Reading extensions storage from plugin '{}'", pluginId); + var bucket = new HashSet(); + try { + log.debug("Read '{}'", EXTENSIONS_RESOURCE); + var classLoader = pluginWrapper.getPluginClassLoader(); + try (var resourceStream = classLoader.getResourceAsStream(EXTENSIONS_RESOURCE)) { + if (resourceStream == null) { + log.debug("Cannot find '{}'", EXTENSIONS_RESOURCE); + } else { + collectExtensions(resourceStream, bucket); } - - debugExtensions(bucket); - - result.put(pluginId, bucket); - } catch (IOException e) { - log.error("Failed to read components from " + EXTENSIONS_RESOURCE, e); } + debugExtensions(bucket); + } catch (IOException e) { + log.error("Failed to read components from " + EXTENSIONS_RESOURCE, e); } - - return result; + return bucket; } private void collectExtensions(InputStream inputStream, Set bucket) throws IOException { @@ -86,20 +72,15 @@ public class SpringComponentsFinder extends AbstractExtensionFinder { } } - protected boolean containsComponentsStorage(String pluginId) { - Assert.notNull(pluginId, "The pluginId cannot be null"); - long stamp = entryStampedLock.tryOptimisticRead(); - boolean contains = super.entries != null && super.entries.containsKey(pluginId); - if (!entryStampedLock.validate(stamp)) { - stamp = entryStampedLock.readLock(); - try { - return super.entries != null && entries.containsKey(pluginId); - } finally { - entryStampedLock.unlockRead(stamp); - } + @Override + public void pluginStateChanged(PluginStateEvent event) { + var pluginState = event.getPluginState(); + String pluginId = event.getPlugin().getPluginId(); + if (pluginState == PluginState.UNLOADED) { + entries.remove(pluginId); + } else if (pluginState == PluginState.CREATED || pluginState == PluginState.RESOLVED) { + entries.computeIfAbsent(pluginId, id -> readPluginStorage(event.getPlugin())); } - return contains; } } - diff --git a/application/src/test/java/run/halo/app/plugin/SpringComponentsFinderTest.java b/application/src/test/java/run/halo/app/plugin/SpringComponentsFinderTest.java index a0d546333..11c86ada4 100644 --- a/application/src/test/java/run/halo/app/plugin/SpringComponentsFinderTest.java +++ b/application/src/test/java/run/halo/app/plugin/SpringComponentsFinderTest.java @@ -1,18 +1,25 @@ package run.halo.app.plugin; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; -import java.io.File; import java.io.FileNotFoundException; -import org.junit.jupiter.api.BeforeEach; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.Set; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import org.pf4j.PluginClassLoader; +import org.pf4j.PluginManager; +import org.pf4j.PluginState; +import org.pf4j.PluginStateEvent; import org.pf4j.PluginWrapper; import org.springframework.util.ResourceUtils; @@ -25,29 +32,64 @@ import org.springframework.util.ResourceUtils; @ExtendWith(MockitoExtension.class) class SpringComponentsFinderTest { - private File testFile; @Mock - private HaloPluginManager pluginManager; - @Mock - private PluginWrapper pluginWrapper; - @Mock - private PluginClassLoader pluginClassLoader; + private PluginManager pluginManager; @InjectMocks - private SpringComponentsFinder springComponentsFinder; + private SpringComponentsFinder finder; - @BeforeEach - void setUp() throws FileNotFoundException { - testFile = ResourceUtils.getFile("classpath:plugin/test-plugin-components.idx"); + @Test + void shouldNotInvokeReadClasspathStorages() { + assertThrows(UnsupportedOperationException.class, + () -> finder.readClasspathStorages() + ); } @Test - void containsPlugin() { - boolean exist = springComponentsFinder.containsComponentsStorage("NotExist"); - assertThat(exist).isFalse(); - - assertThatThrownBy(() -> springComponentsFinder.containsComponentsStorage(null)) - .hasMessage("The pluginId cannot be null"); + void shouldNotInvokeReadPluginsStorages() { + assertThrows(UnsupportedOperationException.class, + () -> finder.readPluginsStorages() + ); } + @Test + void shouldPutEntryIfPluginCreated() throws FileNotFoundException { + var pluginWrapper = mockPluginWrapper(); + when(pluginWrapper.getPluginState()).thenReturn(PluginState.CREATED); + + var event = new PluginStateEvent(pluginManager, pluginWrapper, null); + finder.pluginStateChanged(event); + + var classNames = finder.findClassNames("fake-plugin"); + assertEquals(Set.of("run.halo.fake.FakePlugin"), classNames); + } + + @Test + void shouldRemoveEntryIfPluginUnloaded() throws FileNotFoundException { + var pluginWrapper = mockPluginWrapper(); + when(pluginWrapper.getPluginState()).thenReturn(PluginState.CREATED); + + var event = new PluginStateEvent(pluginManager, pluginWrapper, null); + finder.pluginStateChanged(event); + + var classNames = finder.findClassNames("fake-plugin"); + assertFalse(classNames.isEmpty()); + + when(pluginWrapper.getPluginState()).thenReturn(PluginState.UNLOADED); + event = new PluginStateEvent(pluginManager, pluginWrapper, null); + finder.pluginStateChanged(event); + + classNames = finder.findClassNames("fake-plugin"); + assertTrue(classNames.isEmpty()); + } + + private PluginWrapper mockPluginWrapper() throws FileNotFoundException { + var pluginWrapper = mock(PluginWrapper.class); + when(pluginWrapper.getPluginId()).thenReturn("fake-plugin"); + + var pluginRootUrl = ResourceUtils.getURL("classpath:plugin/plugin-for-finder/"); + var classLoader = new URLClassLoader(new URL[] {pluginRootUrl}); + when(pluginWrapper.getPluginClassLoader()).thenReturn(classLoader); + return pluginWrapper; + } } \ No newline at end of file diff --git a/application/src/test/resources/plugin/plugin-for-finder/META-INF/plugin-components.idx b/application/src/test/resources/plugin/plugin-for-finder/META-INF/plugin-components.idx new file mode 100644 index 000000000..a40f507e4 --- /dev/null +++ b/application/src/test/resources/plugin/plugin-for-finder/META-INF/plugin-components.idx @@ -0,0 +1,2 @@ +# Generated by Halo +run.halo.fake.FakePlugin diff --git a/application/src/test/resources/plugin/test-plugin-components.idx b/application/src/test/resources/plugin/test-plugin-components.idx deleted file mode 100644 index 1835b4bbf..000000000 --- a/application/src/test/resources/plugin/test-plugin-components.idx +++ /dev/null @@ -1,2 +0,0 @@ -# Generated by Halo -run.halo.links.LinkPlugin