From 336a5ba35ad2fd88c7acdf9a0ac13afb4a525e31 Mon Sep 17 00:00:00 2001 From: Decebal Suiu Date: Mon, 19 Feb 2024 18:43:24 +0200 Subject: [PATCH] Add strategy for handling the recovery of a plugin that could not be resolved (#564) --- .../java/org/pf4j/AbstractPluginManager.java | 197 ++++++++++++------ .../java/org/pf4j/DependencyResolver.java | 29 ++- .../org/pf4j/PluginNotFoundException.java | 37 ++++ .../org/pf4j/AbstractPluginManagerTest.java | 6 +- .../org/pf4j/DefaultPluginManagerTest.java | 118 ++++++++++- 5 files changed, 306 insertions(+), 81 deletions(-) create mode 100644 pf4j/src/main/java/org/pf4j/PluginNotFoundException.java diff --git a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java index 2f0d210..f3a7231 100644 --- a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java +++ b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java @@ -31,6 +31,7 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -109,6 +110,7 @@ public abstract class AbstractPluginManager implements PluginManager { protected boolean exactVersionAllowed = false; protected VersionManager versionManager; + protected ResolveRecoveryStrategy resolveRecoveryStrategy; /** * The plugins roots are supplied as comma-separated list by {@code System.getProperty("pf4j.pluginsDir", "plugins")}. @@ -280,56 +282,55 @@ public abstract class AbstractPluginManager implements PluginManager { * @return true if the plugin was unloaded, otherwise false */ protected boolean unloadPlugin(String pluginId, boolean unloadDependents) { - try { - if (unloadDependents) { - List dependents = dependencyResolver.getDependents(pluginId); - while (!dependents.isEmpty()) { - String dependent = dependents.remove(0); - unloadPlugin(dependent, false); - dependents.addAll(0, dependencyResolver.getDependents(dependent)); - } + if (unloadDependents) { + List dependents = dependencyResolver.getDependents(pluginId); + while (!dependents.isEmpty()) { + String dependent = dependents.remove(0); + unloadPlugin(dependent, false); + dependents.addAll(0, dependencyResolver.getDependents(dependent)); } - PluginWrapper pluginWrapper = getPlugin(pluginId); - PluginState pluginState; - try { - pluginState = stopPlugin(pluginId, false); - if (PluginState.STARTED == pluginState) { - return false; - } + } - log.info("Unload plugin '{}'", getPluginLabel(pluginWrapper.getDescriptor())); - } catch (Exception e) { - if (pluginWrapper == null) { - return false; - } - pluginState = PluginState.FAILED; + if (!plugins.containsKey(pluginId)) { + // nothing to do + return false; + } + + PluginWrapper pluginWrapper = getPlugin(pluginId); + PluginState pluginState; + try { + pluginState = stopPlugin(pluginId, false); + if (PluginState.STARTED == pluginState) { + return false; } - // remove the plugin - plugins.remove(pluginId); - getResolvedPlugins().remove(pluginWrapper); - - firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState)); - - // remove the classloader - Map pluginClassLoaders = getPluginClassLoaders(); - if (pluginClassLoaders.containsKey(pluginId)) { - ClassLoader classLoader = pluginClassLoaders.remove(pluginId); - if (classLoader instanceof Closeable) { - try { - ((Closeable) classLoader).close(); - } catch (IOException e) { - throw new PluginRuntimeException(e, "Cannot close classloader"); - } + log.info("Unload plugin '{}'", getPluginLabel(pluginWrapper.getDescriptor())); + } catch (Exception e) { + log.error("Cannot stop plugin '{}'", getPluginLabel(pluginWrapper.getDescriptor()), e); + pluginState = PluginState.FAILED; + } + + // remove the plugin + plugins.remove(pluginId); + getResolvedPlugins().remove(pluginWrapper); + getUnresolvedPlugins().remove(pluginWrapper); + + firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState)); + + // remove the classloader + Map pluginClassLoaders = getPluginClassLoaders(); + if (pluginClassLoaders.containsKey(pluginId)) { + ClassLoader classLoader = pluginClassLoaders.remove(pluginId); + if (classLoader instanceof Closeable) { + try { + ((Closeable) classLoader).close(); + } catch (IOException e) { + throw new PluginRuntimeException(e, "Cannot close classloader"); } } - - return true; - } catch (IllegalArgumentException e) { - // ignore not found exceptions because this method is recursive } - return false; + return true; } @Override @@ -506,11 +507,11 @@ public abstract class AbstractPluginManager implements PluginManager { * Check if the plugin exists in the list of plugins. * * @param pluginId the pluginId to check - * @throws IllegalArgumentException if the plugin does not exist + * @throws PluginNotFoundException if the plugin does not exist */ protected void checkPluginId(String pluginId) { if (!plugins.containsKey(pluginId)) { - throw new IllegalArgumentException(String.format("Unknown pluginId %s", pluginId)); + throw new PluginNotFoundException(pluginId); } } @@ -749,6 +750,7 @@ public abstract class AbstractPluginManager implements PluginManager { versionManager = createVersionManager(); dependencyResolver = new DependencyResolver(versionManager); + resolveRecoveryStrategy = ResolveRecoveryStrategy.THROW_EXCEPTION; } /** @@ -814,27 +816,7 @@ public abstract class AbstractPluginManager implements PluginManager { * @throws PluginRuntimeException if something goes wrong */ protected void resolvePlugins() { - // retrieves the plugins descriptors - List descriptors = plugins.values().stream() - .map(PluginWrapper::getDescriptor) - .collect(Collectors.toList()); - - DependencyResolver.Result result = dependencyResolver.resolve(descriptors); - - if (result.hasCyclicDependency()) { - throw new DependencyResolver.CyclicDependencyException(); - } - - List notFoundDependencies = result.getNotFoundDependencies(); - if (!notFoundDependencies.isEmpty()) { - throw new DependencyResolver.DependenciesNotFoundException(notFoundDependencies); - } - - List wrongVersionDependencies = result.getWrongVersionDependencies(); - if (!wrongVersionDependencies.isEmpty()) { - throw new DependencyResolver.DependenciesWrongVersionException(wrongVersionDependencies); - } - + DependencyResolver.Result result = resolveDependencies(); List sortedPlugins = result.getSortedPlugins(); // move plugins from "unresolved" to "resolved" @@ -1063,4 +1045,89 @@ public abstract class AbstractPluginManager implements PluginManager { return extensions; } + protected DependencyResolver.Result resolveDependencies() { + // retrieves the plugins descriptors + List descriptors = plugins.values().stream() + .map(PluginWrapper::getDescriptor) + .collect(Collectors.toList()); + + DependencyResolver.Result result = dependencyResolver.resolve(descriptors); + + if (result.isOK()) { + return result; + } + + if (result.hasCyclicDependency()) { + // cannot recover from cyclic dependency + throw new DependencyResolver.CyclicDependencyException(); + } + + List notFoundDependencies = result.getNotFoundDependencies(); + if (result.hasNotFoundDependencies() && resolveRecoveryStrategy.equals(ResolveRecoveryStrategy.THROW_EXCEPTION)) { + throw new DependencyResolver.DependenciesNotFoundException(notFoundDependencies); + } + + List wrongVersionDependencies = result.getWrongVersionDependencies(); + if (result.hasWrongVersionDependencies() && resolveRecoveryStrategy.equals(ResolveRecoveryStrategy.THROW_EXCEPTION)) { + throw new DependencyResolver.DependenciesWrongVersionException(wrongVersionDependencies); + } + + List resolvedDescriptors = new ArrayList<>(descriptors); + + for (String notFoundDependency : notFoundDependencies) { + List dependents = dependencyResolver.getDependents(notFoundDependency); + dependents.forEach(dependent -> resolvedDescriptors.removeIf(descriptor -> descriptor.getPluginId().equals(dependent))); + } + + for (DependencyResolver.WrongDependencyVersion wrongVersionDependency : wrongVersionDependencies) { + resolvedDescriptors.removeIf(descriptor -> descriptor.getPluginId().equals(wrongVersionDependency.getDependencyId())); + } + + List unresolvedDescriptors = new ArrayList<>(descriptors); + unresolvedDescriptors.removeAll(resolvedDescriptors); + + for (PluginDescriptor unresolvedDescriptor : unresolvedDescriptors) { + unloadPlugin(unresolvedDescriptor.getPluginId(), false); + } + + return resolveDependencies(); + } + + /** + * Retrieve the strategy for handling the recovery of a plugin resolve (load) failure. + * Default is {@link ResolveRecoveryStrategy#THROW_EXCEPTION}. + * + * @return the strategy + */ + protected final ResolveRecoveryStrategy getResolveRecoveryStrategy() { + return resolveRecoveryStrategy; + } + + /** + * Set the strategy for handling the recovery of a plugin resolve (load) failure. + * + * @param resolveRecoveryStrategy the strategy + */ + protected void setResolveRecoveryStrategy(ResolveRecoveryStrategy resolveRecoveryStrategy) { + Objects.requireNonNull(resolveRecoveryStrategy, "resolveRecoveryStrategy cannot be null"); + this.resolveRecoveryStrategy = resolveRecoveryStrategy; + } + + /** + * Strategy for handling the recovery of a plugin that could not be resolved + * (loaded) due to a dependency problem. + */ + public enum ResolveRecoveryStrategy { + + /** + * Throw an exception when a resolve (load) failure occurs. + */ + THROW_EXCEPTION, + /** + * Ignore the plugin with the resolve (load) failure and continue. + * The plugin with problems will be removed/unloaded from the plugins list. + */ + IGNORE_PLUGIN_AND_CONTINUE + } + } diff --git a/pf4j/src/main/java/org/pf4j/DependencyResolver.java b/pf4j/src/main/java/org/pf4j/DependencyResolver.java index 092b58a..abe9d29 100644 --- a/pf4j/src/main/java/org/pf4j/DependencyResolver.java +++ b/pf4j/src/main/java/org/pf4j/DependencyResolver.java @@ -217,6 +217,15 @@ public class DependencyResolver { return cyclicDependency; } + /** + * Returns true if there are dependencies required that were not found. + * + * @return true if there are dependencies required that were not found + */ + public boolean hasNotFoundDependencies() { + return !notFoundDependencies.isEmpty(); + } + /** * Returns a list with dependencies required that were not found. * @@ -226,6 +235,15 @@ public class DependencyResolver { return notFoundDependencies; } + /** + * Returns true if there are dependencies with wrong version. + * + * @return true if there are dependencies with wrong version + */ + public boolean hasWrongVersionDependencies() { + return !wrongVersionDependencies.isEmpty(); + } + /** * Returns a list with dependencies with wrong version. * @@ -235,6 +253,15 @@ public class DependencyResolver { return wrongVersionDependencies; } + /** + * Returns true if the result is OK (no cyclic dependency, no not found dependencies, no wrong version dependencies). + * + * @return true if the result is OK + */ + public boolean isOK() { + return !hasCyclicDependency() && !hasNotFoundDependencies() && !hasWrongVersionDependencies(); + } + /** * Get the list of plugins in dependency sorted order. * @@ -333,7 +360,7 @@ public class DependencyResolver { */ public static class DependenciesWrongVersionException extends PluginRuntimeException { - private List dependencies; + private final List dependencies; public DependenciesWrongVersionException(List dependencies) { super("Dependencies '{}' have wrong version", dependencies); diff --git a/pf4j/src/main/java/org/pf4j/PluginNotFoundException.java b/pf4j/src/main/java/org/pf4j/PluginNotFoundException.java new file mode 100644 index 0000000..003aaf6 --- /dev/null +++ b/pf4j/src/main/java/org/pf4j/PluginNotFoundException.java @@ -0,0 +1,37 @@ +/* + * Copyright (C) 2012-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.pf4j; + +/** + * Thrown when a plugin is not found. + * + * @author Decebal Suiu + */ +public class PluginNotFoundException extends PluginRuntimeException { + + private final String pluginId; + + public PluginNotFoundException(String pluginId) { + super("Plugin '{}' not found", pluginId); + + this.pluginId = pluginId; + } + + public String getPluginId() { + return pluginId; + } + +} diff --git a/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java b/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java index 70a9312..07a083b 100644 --- a/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java +++ b/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java @@ -18,12 +18,10 @@ package org.pf4j; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; import org.pf4j.test.TestExtension; import org.pf4j.test.TestExtensionPoint; import java.io.IOException; -import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; @@ -118,7 +116,7 @@ public class AbstractPluginManagerTest { @Test void checkNotExistedPluginId() { - assertThrows(IllegalArgumentException.class, () -> pluginManager.checkPluginId("plugin1")); + assertThrows(PluginNotFoundException.class, () -> pluginManager.checkPluginId("plugin1")); } private PluginWrapper createPluginWrapper(String pluginId) { @@ -127,7 +125,7 @@ public class AbstractPluginManagerTest { pluginDescriptor.setPluginVersion("1.2.3"); PluginWrapper pluginWrapper = new PluginWrapper(pluginManager, pluginDescriptor, Paths.get("plugin1"), getClass().getClassLoader()); - Plugin plugin= mock(Plugin.class); + Plugin plugin = mock(Plugin.class); pluginWrapper.setPluginFactory(wrapper -> plugin); return pluginWrapper; diff --git a/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java b/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java index 87c5e3d..2e78ad2 100644 --- a/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java +++ b/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java @@ -28,8 +28,6 @@ import java.nio.file.Path; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -194,24 +192,122 @@ public class DefaultPluginManagerTest { @Test public void loadedPluginWithMissingDependencyCanBeUnloaded() throws IOException { + pluginManager.setResolveRecoveryStrategy(AbstractPluginManager.ResolveRecoveryStrategy.IGNORE_PLUGIN_AND_CONTINUE); + PluginZip pluginZip = new PluginZip.Builder(pluginsPath.resolve("my-plugin-1.1.1.zip"), "myPlugin") .pluginVersion("1.1.1") .pluginDependencies("myBasePlugin") .build(); - try { - pluginManager.loadPlugin(pluginZip.path()); - } catch (DependencyResolver.DependenciesNotFoundException e) { - // expected - } + // try to load the plugin with a missing dependency + pluginManager.loadPlugin(pluginZip.path()); + + // the plugin is unloaded automatically + assertTrue(pluginManager.getPlugins().isEmpty()); + + // start plugins + pluginManager.startPlugins(); + + assertTrue(pluginManager.getStartedPlugins().isEmpty()); + assertTrue(pluginManager.getUnresolvedPlugins().isEmpty()); + assertTrue(pluginManager.getResolvedPlugins().isEmpty()); + assertTrue(pluginManager.getPlugins().isEmpty()); + } + + @Test + void loadingPluginWithMissingDependencyDoesNotBreakOtherPlugins() throws IOException { + pluginManager.setResolveRecoveryStrategy(AbstractPluginManager.ResolveRecoveryStrategy.IGNORE_PLUGIN_AND_CONTINUE); + + // Load 2 plugins, one with a dependency that is missing and one without any dependencies. + PluginZip pluginZip1 = new PluginZip.Builder(pluginsPath.resolve("my-first-plugin-1.1.1.zip"), "myPlugin1") + .pluginVersion("1.1.1") + .pluginDependencies("myBasePlugin") + .build(); + + PluginZip pluginZip2 = new PluginZip.Builder(pluginsPath.resolve("my-second-plugin-2.2.2.zip"), "myPlugin2") + .pluginVersion("2.2.2") + .build(); + pluginManager.loadPlugins(); pluginManager.startPlugins(); - PluginWrapper myPlugin = pluginManager.getPlugin(pluginZip.pluginId()); - assertNotNull(myPlugin); - assertNotEquals(PluginState.STARTED, myPlugin.getPluginState()); + // myPlugin2 should have been started at this point. + assertEquals(PluginState.STARTED, pluginManager.getPlugin(pluginZip2.pluginId()).getPluginState()); + + pluginManager.unloadPlugin(pluginZip1.pluginId()); + + // No traces should remain of myPlugin1. + assertTrue( + pluginManager.getUnresolvedPlugins().stream() + .noneMatch(pluginWrapper -> pluginWrapper.getPluginId().equals(pluginZip1.pluginId())) + ); + + pluginManager.unloadPlugin(pluginZip2.pluginId()); + + // Load the missing dependency, everything should start working. + PluginZip pluginZipBase = new PluginZip.Builder(pluginsPath.resolve("my-base-plugin-3.0.0.zip"), "myBasePlugin") + .pluginVersion("3.0.0") + .build(); + + pluginManager.loadPlugins(); + pluginManager.startPlugins(); + + assertEquals(PluginState.STARTED, pluginManager.getPlugin(pluginZip1.pluginId()).getPluginState()); + assertEquals(PluginState.STARTED, pluginManager.getPlugin(pluginZip2.pluginId()).getPluginState()); + assertEquals(PluginState.STARTED, pluginManager.getPlugin(pluginZipBase.pluginId()).getPluginState()); + + assertTrue(pluginManager.getUnresolvedPlugins().isEmpty()); + } + + @Test + void loadingPluginWithMissingDependencyFails() throws IOException { + pluginManager.setResolveRecoveryStrategy(AbstractPluginManager.ResolveRecoveryStrategy.THROW_EXCEPTION); + + PluginZip pluginZip = new PluginZip.Builder(pluginsPath.resolve("my-plugin-1.1.1.zip"), "myPlugin") + .pluginVersion("1.1.1") + .pluginDependencies("myBasePlugin") + .build(); + + // try to load the plugin with a missing dependency + Path pluginPath = pluginZip.path(); + assertThrows(DependencyResolver.DependenciesNotFoundException.class, () -> pluginManager.loadPlugin(pluginPath)); + } + + @Test + void loadingPluginWithWrongDependencyVersionFails() throws IOException { + pluginManager.setResolveRecoveryStrategy(AbstractPluginManager.ResolveRecoveryStrategy.THROW_EXCEPTION); + + PluginZip pluginZip1 = new PluginZip.Builder(pluginsPath.resolve("my-first-plugin-1.1.1.zip"), "myPlugin1") + .pluginVersion("1.1.1") + .pluginDependencies("myPlugin2@3.0.0") + .build(); + + PluginZip pluginZip2 = new PluginZip.Builder(pluginsPath.resolve("my-second-plugin-2.2.2.zip"), "myPlugin2") + .pluginVersion("2.2.2") + .build(); + + // try to load the plugins with cyclic dependencies + Path pluginPath1 = pluginZip1.path(); + Path pluginPath2 = pluginZip2.path(); + assertThrows(DependencyResolver.DependenciesWrongVersionException.class, () -> pluginManager.loadPlugins()); + } + + @Test + void loadingPluginsWithCyclicDependenciesFails() throws IOException { + PluginZip pluginZip1 = new PluginZip.Builder(pluginsPath.resolve("my-first-plugin-1.1.1.zip"), "myPlugin1") + .pluginVersion("1.1.1") + .pluginDependencies("myPlugin2") + .build(); + + PluginZip pluginZip2 = new PluginZip.Builder(pluginsPath.resolve("my-second-plugin-2.2.2.zip"), "myPlugin2") + .pluginVersion("2.2.2") + .pluginDependencies("myPlugin1") + .build(); - assertTrue(pluginManager.unloadPlugin(pluginZip.pluginId())); + // try to load the plugins with cyclic dependencies + Path pluginPath1 = pluginZip1.path(); + Path pluginPath2 = pluginZip2.path(); + assertThrows(DependencyResolver.CyclicDependencyException.class, () -> pluginManager.loadPlugins()); } } -- 2.39.5