Fix #520

3 files changed, 152 insertions, 36 deletions

diff --git a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
index beac7c8..c3e8c0f 100644
--- a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
+++ b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
@@ -91,7 +91,7 @@ public abstract class AbstractPluginManager implements PluginManager {
 
     /**
      * Cache value for the runtime mode.
-     * No need to re-read it because it wont change at runtime.
+     * No need to re-read it because it won't change at runtime.
      */
     protected RuntimeMode runtimeMode;
 
@@ -455,6 +455,9 @@ public abstract class AbstractPluginManager implements PluginManager {
                 } catch (PluginRuntimeException e) {
                     log.error(e.getMessage(), e);
                 }
+            } else {
+                // do nothing
+                log.debug("Plugin '{}' is not started, nothing to stop", getPluginLabel(pluginWrapper.getDescriptor()));
             }
         }
     }
@@ -477,18 +480,11 @@ public abstract class AbstractPluginManager implements PluginManager {
     protected PluginState stopPlugin(String pluginId, boolean stopDependents) {
         checkPluginId(pluginId);
 
-        PluginWrapper pluginWrapper = getPlugin(pluginId);
-        PluginDescriptor pluginDescriptor = pluginWrapper.getDescriptor();
-        PluginState pluginState = pluginWrapper.getPluginState();
-        if (PluginState.STOPPED == pluginState) {
-            log.debug("Already stopped plugin '{}'", getPluginLabel(pluginDescriptor));
-            return PluginState.STOPPED;
-        }
-
-        // test for disabled plugin
-        if (PluginState.DISABLED == pluginState) {
+        // test for started plugin
+        if (!checkPluginState(pluginId, PluginState.STARTED)) {
             // do nothing
-            return pluginState;
+            log.debug("Plugin '{}' is not started, nothing to stop", getPluginLabel(pluginId));
+            return getPlugin(pluginId).getPluginState();
         }
 
         if (stopDependents) {
@@ -500,14 +496,15 @@ public abstract class AbstractPluginManager implements PluginManager {
             }
         }
 
-        log.info("Stop plugin '{}'", getPluginLabel(pluginDescriptor));
+        log.info("Stop plugin '{}'", getPluginLabel(pluginId));
+        PluginWrapper pluginWrapper = getPlugin(pluginId);
         pluginWrapper.getPlugin().stop();
         pluginWrapper.setPluginState(PluginState.STOPPED);
-        startedPlugins.remove(pluginWrapper);
+        getStartedPlugins().remove(pluginWrapper);
 
-        firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState));
+        firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, PluginState.STOPPED));
 
-        return pluginWrapper.getPluginState();
+        return PluginState.STOPPED;
     }
 
     /**
@@ -522,6 +519,16 @@ public abstract class AbstractPluginManager implements PluginManager {
         }
     }
 
+    /**
+     * Check if the plugin state is equals with the value passed for parameter {@code pluginState}.
+     *
+     * @param pluginId the pluginId to check
+     * @return {@code true} if the plugin state is equals with the value passed for parameter {@code pluginState}, otherwise {@code false}
+     */
+    protected boolean checkPluginState(String pluginId, PluginState pluginState) {
+        return getPlugin(pluginId).getPluginState() == pluginState;
+    }
+
     @Override
     public boolean disablePlugin(String pluginId) {
         checkPluginId(pluginId);
@@ -532,20 +539,21 @@ public abstract class AbstractPluginManager implements PluginManager {
         if (PluginState.DISABLED == pluginState) {
             log.debug("Already disabled plugin '{}'", getPluginLabel(pluginDescriptor));
             return true;
+        } else if (PluginState.STARTED == pluginState) {
+            if (PluginState.STOPPED == stopPlugin(pluginId)) {
+                log.error("Failed to stop plugin '{}' on disable", getPluginLabel(pluginDescriptor));
+                return false;
+            }
         }
 
-        if (PluginState.STOPPED == stopPlugin(pluginId)) {
-            pluginWrapper.setPluginState(PluginState.DISABLED);
+        pluginWrapper.setPluginState(PluginState.DISABLED);
 
-            firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, PluginState.STOPPED));
+        firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState));
 
-            pluginStatusProvider.disablePlugin(pluginId);
-            log.info("Disabled plugin '{}'", getPluginLabel(pluginDescriptor));
+        pluginStatusProvider.disablePlugin(pluginId);
+        log.info("Disabled plugin '{}'", getPluginLabel(pluginDescriptor));
 
-            return true;
-        }
-
-        return false;
+        return true;
     }
 
     @Override
@@ -812,10 +820,9 @@ public abstract class AbstractPluginManager implements PluginManager {
      */
     protected void resolvePlugins() {
         // retrieves the plugins descriptors
-        List<PluginDescriptor> descriptors = new ArrayList<>();
-        for (PluginWrapper plugin : plugins.values()) {
-            descriptors.add(plugin.getDescriptor());
-        }
+        List<PluginDescriptor> descriptors = plugins.values().stream()
+            .map(PluginWrapper::getDescriptor)
+            .collect(Collectors.toList());
 
         DependencyResolver.Result result = dependencyResolver.resolve(descriptors);
 
@@ -1022,6 +1029,16 @@ public abstract class AbstractPluginManager implements PluginManager {
         return pluginDescriptor.getPluginId() + "@" + pluginDescriptor.getVersion();
     }
 
+    /**
+     * Shortcut for {@code getPluginLabel(getPlugin(pluginId).getDescriptor())}.
+     *
+     * @param pluginId the pluginId
+     * @return the plugin label
+     */
+    protected String getPluginLabel(String pluginId) {
+        return getPluginLabel(getPlugin(pluginId).getDescriptor());
+    }
+
     @SuppressWarnings("unchecked")
     protected <T> List<Class<? extends T>> getExtensionClasses(List<ExtensionWrapper<T>> extensionsWrapper) {
         List<Class<? extends T>> extensionClasses = new ArrayList<>(extensionsWrapper.size());
diff --git a/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java b/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java
index c43bc63..70a9312 100644
--- a/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java
+++ b/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java
@@ -15,28 +15,54 @@
  */
 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;
 import java.util.List;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
 
 /**
  * @author Decebal Suiu
  */
 public class AbstractPluginManagerTest {
 
+    private AbstractPluginManager pluginManager;
+
+    @BeforeEach
+    void setUp() {
+        pluginManager = mock(AbstractPluginManager.class, withSettings()
+            .useConstructor()
+            .defaultAnswer(CALLS_REAL_METHODS));
+    }
+
+    @AfterEach
+    void tearDown() {
+        pluginManager = null;
+    }
+
     @Test
     public void getExtensionsByType() {
-        AbstractPluginManager pluginManager = mock(AbstractPluginManager.class, CALLS_REAL_METHODS);
-
         ExtensionFinder extensionFinder = mock(ExtensionFinder.class);
         List<ExtensionWrapper<TestExtensionPoint>> extensionList = new ArrayList<>(1);
         extensionList.add(new ExtensionWrapper<>(new ExtensionDescriptor(0, TestExtension.class), new DefaultExtensionFactory()));
@@ -46,11 +72,65 @@ public class AbstractPluginManagerTest {
         List<TestExtensionPoint> extensions = pluginManager.getExtensions(TestExtensionPoint.class);
         assertEquals(1, extensions.size());
     }
-    
+
     @Test
     public void getVersion() {
-        AbstractPluginManager pluginManager = mock(AbstractPluginManager.class, CALLS_REAL_METHODS);
         assertNotEquals("0.0.0", pluginManager.getVersion());
     }
 
+    @Test
+    void stopStartedPlugin() throws IOException {
+        String pluginId = "plugin1";
+        PluginWrapper pluginWrapper = createPluginWrapper(pluginId);
+        pluginWrapper.setPluginState(PluginState.STARTED);
+
+        doReturn(pluginWrapper).when(pluginManager).getPlugin(pluginId);
+        doNothing().when(pluginManager).checkPluginId(pluginId);
+        doReturn(new ArrayList<>(Arrays.asList(pluginWrapper))).when(pluginManager).getStartedPlugins();
+
+        PluginState pluginState = pluginManager.stopPlugin(pluginId, false);
+        verify(pluginWrapper.getPlugin()).stop();
+        assertSame(PluginState.STOPPED, pluginState);
+    }
+
+    @Test
+    void stopCreatedPlugin() {
+        String pluginId = "plugin1";
+        PluginWrapper pluginWrapper = createPluginWrapper(pluginId);
+
+        doReturn(pluginWrapper).when(pluginManager).getPlugin(pluginId);
+        doNothing().when(pluginManager).checkPluginId(pluginId);
+        doReturn(new ArrayList<>(Arrays.asList(pluginWrapper))).when(pluginManager).getStartedPlugins();
+
+        PluginState pluginState = pluginManager.stopPlugin(pluginId, false);
+        verify(pluginWrapper.getPlugin(), never()).stop();
+        assertSame(PluginState.CREATED, pluginState);
+    }
+
+    @Test
+    void checkExistedPluginId() {
+        String pluginId = "plugin1";
+        PluginWrapper pluginWrapper = createPluginWrapper(pluginId);
+
+        pluginManager.plugins.put("plugin1", pluginWrapper);
+        pluginManager.checkPluginId("plugin1");
+    }
+
+    @Test
+    void checkNotExistedPluginId() {
+        assertThrows(IllegalArgumentException.class, () -> pluginManager.checkPluginId("plugin1"));
+    }
+
+    private PluginWrapper createPluginWrapper(String pluginId) {
+        DefaultPluginDescriptor pluginDescriptor = new DefaultPluginDescriptor();
+        pluginDescriptor.setPluginId(pluginId);
+        pluginDescriptor.setPluginVersion("1.2.3");
+
+        PluginWrapper pluginWrapper = new PluginWrapper(pluginManager, pluginDescriptor, Paths.get("plugin1"), getClass().getClassLoader());
+        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 fab1a02..3edb1c5 100644
--- a/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java
+++ b/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java
@@ -28,6 +28,8 @@ 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;
@@ -90,7 +92,7 @@ public class DefaultPluginManagerTest {
 
     @Test
     public void isPluginValid() {
-        // By default accept all since system version not given
+        // By default, accept all since system version not given
         assertTrue(pluginManager.isPluginValid(pluginWrapper));
 
         pluginManager.setSystemVersion("1.0.0");
@@ -107,7 +109,7 @@ public class DefaultPluginManagerTest {
     public void isPluginValidAllowExact() {
         pluginManager.setExactVersionAllowed(true);
 
-        // By default accept all since system version not given
+        // By default, accept all since system version not given
         assertTrue(pluginManager.isPluginValid(pluginWrapper));
 
         pluginManager.setSystemVersion("1.0.0");
@@ -127,7 +129,7 @@ public class DefaultPluginManagerTest {
 
     /**
      * Test that a disabled plugin doesn't start.
-     * See https://github.com/pf4j/pf4j/issues/223.
+     * See <a href="https://github.com/pf4j/pf4j/issues/223">#223</a>.
      */
     @Test
     public void testPluginDisabledNoStart() throws IOException {
@@ -190,4 +192,21 @@ public class DefaultPluginManagerTest {
         assertFalse(pluginJar.file().exists());
     }
 
+    @Test
+    public void loadedPluginWithMissingDependencyCanBeUnloaded() throws IOException {
+        PluginZip pluginZip = new PluginZip.Builder(pluginsPath.resolve("my-plugin-1.1.1.zip"), "myPlugin")
+            .pluginVersion("1.1.1")
+            .pluginDependencies("myBasePlugin")
+            .build();
+
+        pluginManager.loadPlugins();
+        pluginManager.startPlugins();
+
+        PluginWrapper myPlugin = pluginManager.getPlugin(pluginZip.pluginId());
+        assertNotNull(myPlugin);
+        assertNotEquals(PluginState.STARTED, myPlugin.getPluginState());
+
+        assertTrue(pluginManager.unloadPlugin(pluginZip.pluginId()));
+    }
+
 }