Improve #292

11 files changed, 60 insertions, 89 deletions

diff --git a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
index 3833588..4fa1d38 100644
--- a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
+++ b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
@@ -232,11 +232,11 @@ public abstract class AbstractPluginManager implements PluginManager {
      * Unload the specified plugin and it's dependents.
      */
     @Override
-    public boolean unloadPlugin(String pluginId) {
+    public boolean unloadPlugin(String pluginId) throws PluginException {
         return unloadPlugin(pluginId, true);
     }
 
-    private boolean unloadPlugin(String pluginId, boolean unloadDependents) {
+    private boolean unloadPlugin(String pluginId, boolean unloadDependents) throws PluginException {
         try {
             if (unloadDependents) {
                 List<String> dependents = dependencyResolver.getDependents(pluginId);
@@ -269,7 +269,7 @@ public abstract class AbstractPluginManager implements PluginManager {
                     try {
                         ((Closeable) classLoader).close();
                     } catch (IOException e) {
-                        log.error("Cannot close classloader", e);
+                        throw new PluginException("Cannot close classloader", e);
                     }
                 }
             }
@@ -283,7 +283,7 @@ public abstract class AbstractPluginManager implements PluginManager {
     }
 
     @Override
-    public boolean deletePlugin(String pluginId) {
+    public boolean deletePlugin(String pluginId) throws PluginException {
         checkPluginId(pluginId);
 
         PluginWrapper pluginWrapper = getPlugin(pluginId);
@@ -304,12 +304,7 @@ public abstract class AbstractPluginManager implements PluginManager {
         }
 
         // notify the plugin as it's deleted
-        try {
-            plugin.delete();
-        } catch (PluginException e) {
-            log.error(e.getMessage(), e);
-            return false;
-        }
+        plugin.delete();
 
         Path pluginPath = pluginWrapper.getPluginPath();
 
@@ -342,7 +337,7 @@ public abstract class AbstractPluginManager implements PluginManager {
      * Start the specified plugin and its dependencies.
      */
     @Override
-    public PluginState startPlugin(String pluginId) {
+    public PluginState startPlugin(String pluginId) throws PluginException {
         checkPluginId(pluginId);
 
         PluginWrapper pluginWrapper = getPlugin(pluginId);
@@ -369,16 +364,12 @@ public abstract class AbstractPluginManager implements PluginManager {
             startPlugin(dependency.getPluginId());
         }
 
-        try {
-            log.info("Start plugin '{}'", getPluginLabel(pluginDescriptor));
-            pluginWrapper.getPlugin().start();
-            pluginWrapper.setPluginState(PluginState.STARTED);
-            startedPlugins.add(pluginWrapper);
+        log.info("Start plugin '{}'", getPluginLabel(pluginDescriptor));
+        pluginWrapper.getPlugin().start();
+        pluginWrapper.setPluginState(PluginState.STARTED);
+        startedPlugins.add(pluginWrapper);
 
-            firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState));
-        } catch (PluginException e) {
-            log.error(e.getMessage(), e);
-        }
+        firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState));
 
         return pluginWrapper.getPluginState();
     }
@@ -413,11 +404,11 @@ public abstract class AbstractPluginManager implements PluginManager {
      * Stop the specified plugin and it's dependents.
      */
     @Override
-    public PluginState stopPlugin(String pluginId) {
+    public PluginState stopPlugin(String pluginId) throws PluginException {
         return stopPlugin(pluginId, true);
     }
 
-    private PluginState stopPlugin(String pluginId, boolean stopDependents) {
+    private PluginState stopPlugin(String pluginId, boolean stopDependents) throws PluginException {
         checkPluginId(pluginId);
 
         PluginWrapper pluginWrapper = getPlugin(pluginId);
@@ -443,16 +434,12 @@ public abstract class AbstractPluginManager implements PluginManager {
             }
         }
 
-        try {
-            log.info("Stop plugin '{}'", getPluginLabel(pluginDescriptor));
-            pluginWrapper.getPlugin().stop();
-            pluginWrapper.setPluginState(PluginState.STOPPED);
-            startedPlugins.remove(pluginWrapper);
+        log.info("Stop plugin '{}'", getPluginLabel(pluginDescriptor));
+        pluginWrapper.getPlugin().stop();
+        pluginWrapper.setPluginState(PluginState.STOPPED);
+        startedPlugins.remove(pluginWrapper);
 
-            firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState));
-        } catch (PluginException e) {
-            log.error(e.getMessage(), e);
-        }
+        firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState));
 
         return pluginWrapper.getPluginState();
     }
@@ -464,7 +451,7 @@ public abstract class AbstractPluginManager implements PluginManager {
     }
 
     @Override
-    public boolean disablePlugin(String pluginId) {
+    public boolean disablePlugin(String pluginId) throws PluginException {
         checkPluginId(pluginId);
 
         PluginWrapper pluginWrapper = getPlugin(pluginId);
@@ -480,10 +467,7 @@ public abstract class AbstractPluginManager implements PluginManager {
 
             firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, PluginState.STOPPED));
 
-            if (!pluginStatusProvider.disablePlugin(pluginId)) {
-                return false;
-            }
-
+            pluginStatusProvider.disablePlugin(pluginId);
             log.info("Disabled plugin '{}'", getPluginLabel(pluginDescriptor));
 
             return true;
@@ -493,7 +477,7 @@ public abstract class AbstractPluginManager implements PluginManager {
     }
 
     @Override
-    public boolean enablePlugin(String pluginId) {
+    public boolean enablePlugin(String pluginId) throws PluginException {
         checkPluginId(pluginId);
 
         PluginWrapper pluginWrapper = getPlugin(pluginId);
@@ -509,9 +493,7 @@ public abstract class AbstractPluginManager implements PluginManager {
             return true;
         }
 
-        if (!pluginStatusProvider.enablePlugin(pluginId)) {
-            return false;
-        }
+        pluginStatusProvider.enablePlugin(pluginId);
 
         pluginWrapper.setPluginState(PluginState.CREATED);
 
@@ -612,7 +594,6 @@ public abstract class AbstractPluginManager implements PluginManager {
         return extensionFactory;
     }
 
-    // TODO remove
     public PluginLoader getPluginLoader() {
         return pluginLoader;
     }
diff --git a/pf4j/src/main/java/org/pf4j/BasePluginRepository.java b/pf4j/src/main/java/org/pf4j/BasePluginRepository.java
index aa152be..b083994 100644
--- a/pf4j/src/main/java/org/pf4j/BasePluginRepository.java
+++ b/pf4j/src/main/java/org/pf4j/BasePluginRepository.java
@@ -86,7 +86,7 @@ public class BasePluginRepository implements PluginRepository {
     }
 
     @Override
-    public boolean deletePluginPath(Path pluginPath) {
+    public boolean deletePluginPath(Path pluginPath) throws PluginException {
         if (!filter.accept(pluginPath.toFile())) {
             return false;
         }
@@ -97,7 +97,7 @@ public class BasePluginRepository implements PluginRepository {
         } catch (NoSuchFileException e) {
             return false; // Return false on not found to be compatible with previous API (#135)
         } catch (IOException e) {
-            throw new RuntimeException(e);
+            throw new PluginException(e);
         }
     }
 
diff --git a/pf4j/src/main/java/org/pf4j/CompoundPluginRepository.java b/pf4j/src/main/java/org/pf4j/CompoundPluginRepository.java
index 705b912..c738191 100644
--- a/pf4j/src/main/java/org/pf4j/CompoundPluginRepository.java
+++ b/pf4j/src/main/java/org/pf4j/CompoundPluginRepository.java
@@ -48,7 +48,7 @@ public class CompoundPluginRepository implements PluginRepository {
     }
 
     @Override
-    public boolean deletePluginPath(Path pluginPath) {
+    public boolean deletePluginPath(Path pluginPath) throws PluginException {
         for (PluginRepository repository : repositories) {
             if (repository.deletePluginPath(pluginPath)) {
                 return true;
diff --git a/pf4j/src/main/java/org/pf4j/DefaultPluginRepository.java b/pf4j/src/main/java/org/pf4j/DefaultPluginRepository.java
index da456eb..1c031b6 100644
--- a/pf4j/src/main/java/org/pf4j/DefaultPluginRepository.java
+++ b/pf4j/src/main/java/org/pf4j/DefaultPluginRepository.java
@@ -54,7 +54,7 @@ public class DefaultPluginRepository extends BasePluginRepository {
     }
 
     @Override
-    public boolean deletePluginPath(Path pluginPath) {
+    public boolean deletePluginPath(Path pluginPath) throws PluginException {
         FileUtils.optimisticDelete(FileUtils.findWithEnding(pluginPath, ".zip", ".ZIP", ".Zip"));
         return super.deletePluginPath(pluginPath);
     }
diff --git a/pf4j/src/main/java/org/pf4j/DefaultPluginStatusProvider.java b/pf4j/src/main/java/org/pf4j/DefaultPluginStatusProvider.java
index ca85407..823a29c 100644
--- a/pf4j/src/main/java/org/pf4j/DefaultPluginStatusProvider.java
+++ b/pf4j/src/main/java/org/pf4j/DefaultPluginStatusProvider.java
@@ -66,31 +66,23 @@ public class DefaultPluginStatusProvider implements PluginStatusProvider {
     }
 
     @Override
-    public boolean disablePlugin(String pluginId) {
-        if (disabledPlugins.add(pluginId)) {
-            try {
-                FileUtils.writeLines(disabledPlugins, pluginsRoot.resolve("disabled.txt").toFile());
-            } catch (IOException e) {
-                log.error("Failed to disable plugin {}", pluginId, e);
-                return false;
-            }
+    public void disablePlugin(String pluginId) throws PluginException {
+        disabledPlugins.add(pluginId);
+        try {
+            FileUtils.writeLines(disabledPlugins, pluginsRoot.resolve("disabled.txt").toFile());
+        } catch (IOException e) {
+            throw new PluginException(e);
         }
-
-        return true;
     }
 
     @Override
-    public boolean enablePlugin(String pluginId) {
-        if (disabledPlugins.remove(pluginId)) {
-            try {
-                FileUtils.writeLines(disabledPlugins, pluginsRoot.resolve("disabled.txt").toFile());
-            } catch (IOException e) {
-                log.error("Failed to enable plugin {}", pluginId, e);
-                return false;
-            }
+    public void enablePlugin(String pluginId) throws PluginException {
+        disabledPlugins.remove(pluginId);
+        try {
+            FileUtils.writeLines(disabledPlugins, pluginsRoot.resolve("disabled.txt").toFile());
+        } catch (IOException e) {
+            throw new PluginException(e);
         }
-
-        return true;
     }
 
 }
diff --git a/pf4j/src/main/java/org/pf4j/PluginManager.java b/pf4j/src/main/java/org/pf4j/PluginManager.java
index 11d3eac..a283738 100644
--- a/pf4j/src/main/java/org/pf4j/PluginManager.java
+++ b/pf4j/src/main/java/org/pf4j/PluginManager.java
@@ -85,7 +85,7 @@ public interface PluginManager {
      *
      * @return the plugin state
      */
-    PluginState startPlugin(String pluginId);
+    PluginState startPlugin(String pluginId) throws PluginException;
 
     /**
      * Stop all active plugins.
@@ -97,7 +97,7 @@ public interface PluginManager {
      *
      * @return the plugin state
      */
-    PluginState stopPlugin(String pluginId);
+    PluginState stopPlugin(String pluginId) throws PluginException;
 
     /**
      * Unload a plugin.
@@ -105,7 +105,7 @@ public interface PluginManager {
      * @param pluginId the unique plugin identifier, specified in its metadata
      * @return true if the plugin was unloaded
      */
-    boolean unloadPlugin(String pluginId);
+    boolean unloadPlugin(String pluginId) throws PluginException;
 
     /**
      * Disables a plugin from being loaded.
@@ -113,7 +113,7 @@ public interface PluginManager {
      * @param pluginId the unique plugin identifier, specified in its metadata
      * @return true if plugin is disabled
      */
-    boolean disablePlugin(String pluginId);
+    boolean disablePlugin(String pluginId) throws PluginException;
 
     /**
      * Enables a plugin that has previously been disabled.
@@ -121,7 +121,7 @@ public interface PluginManager {
      * @param pluginId the unique plugin identifier, specified in its metadata
      * @return true if plugin is enabled
      */
-    boolean enablePlugin(String pluginId);
+    boolean enablePlugin(String pluginId) throws PluginException;
 
     /**
      * Deletes a plugin.
@@ -129,7 +129,7 @@ public interface PluginManager {
      * @param pluginId the unique plugin identifier, specified in its metadata
      * @return true if the plugin was deleted
      */
-    boolean deletePlugin(String pluginId);
+    boolean deletePlugin(String pluginId) throws PluginException;
 
     ClassLoader getPluginClassLoader(String pluginId);
 
diff --git a/pf4j/src/main/java/org/pf4j/PluginRepository.java b/pf4j/src/main/java/org/pf4j/PluginRepository.java
index d0b2125..941d68f 100644
--- a/pf4j/src/main/java/org/pf4j/PluginRepository.java
+++ b/pf4j/src/main/java/org/pf4j/PluginRepository.java
@@ -39,6 +39,6 @@ public interface PluginRepository {
      * @param pluginPath the plugin path
      * @return true if deleted
      */
-    boolean deletePluginPath(Path pluginPath);
+    boolean deletePluginPath(Path pluginPath) throws PluginException;
 
 }
diff --git a/pf4j/src/main/java/org/pf4j/PluginStatusProvider.java b/pf4j/src/main/java/org/pf4j/PluginStatusProvider.java
index dda5f82..228575f 100644
--- a/pf4j/src/main/java/org/pf4j/PluginStatusProvider.java
+++ b/pf4j/src/main/java/org/pf4j/PluginStatusProvider.java
@@ -33,16 +33,14 @@ public interface PluginStatusProvider {
      * Disables a plugin from being loaded.
      *
      * @param pluginId the unique plugin identifier, specified in its metadata
-     * @return true if plugin is disabled
      */
-    boolean disablePlugin(String pluginId);
+    void disablePlugin(String pluginId) throws PluginException;
 
     /**
      * Enables a plugin that has previously been disabled.
      *
      * @param pluginId the unique plugin identifier, specified in its metadata
-     * @return true if plugin is enabled
      */
-    boolean enablePlugin(String pluginId);
+    void enablePlugin(String pluginId) throws PluginException;
 
 }
diff --git a/pf4j/src/test/java/org/pf4j/DefaultPluginRepositoryTest.java b/pf4j/src/test/java/org/pf4j/DefaultPluginRepositoryTest.java
index 3aaba8f..3805c42 100644
--- a/pf4j/src/test/java/org/pf4j/DefaultPluginRepositoryTest.java
+++ b/pf4j/src/test/java/org/pf4j/DefaultPluginRepositoryTest.java
@@ -85,7 +85,7 @@ public class DefaultPluginRepositoryTest {
      * Test of {@link DefaultPluginRepository#deletePluginPath(Path)} method.
      */
     @Test
-    public void testDeletePluginPath() {
+    public void testDeletePluginPath() throws PluginException {
         PluginRepository repository = new DefaultPluginRepository(pluginsPath, false);
 
         assertTrue(Files.exists(pluginsPath.resolve("plugin-1.zip")));
diff --git a/pf4j/src/test/java/org/pf4j/DefaultPluginStatusProviderTest.java b/pf4j/src/test/java/org/pf4j/DefaultPluginStatusProviderTest.java
index 635e41c..e766735 100644
--- a/pf4j/src/test/java/org/pf4j/DefaultPluginStatusProviderTest.java
+++ b/pf4j/src/test/java/org/pf4j/DefaultPluginStatusProviderTest.java
@@ -60,58 +60,59 @@ public class DefaultPluginStatusProviderTest {
     }
 
     @Test
-    public void testDisablePlugin() throws IOException {
+    public void testDisablePlugin() throws Exception {
         createEnabledFile();
         createDisabledFile();
 
         PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath);
+        statusProvider.disablePlugin("plugin-1");
 
-        assertTrue(statusProvider.disablePlugin("plugin-1"));
         assertTrue(statusProvider.isPluginDisabled("plugin-1"));
         assertTrue(statusProvider.isPluginDisabled("plugin-2"));
         assertTrue(statusProvider.isPluginDisabled("plugin-3"));
     }
 
     @Test
-    public void testDisablePluginWithEnableEmpty() throws IOException {
+    public void testDisablePluginWithEnableEmpty() throws Exception {
         createDisabledFile();
 
         PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath);
+        statusProvider.disablePlugin("plugin-1");
 
-        assertTrue(statusProvider.disablePlugin("plugin-1"));
         assertTrue(statusProvider.isPluginDisabled("plugin-1"));
         assertTrue(statusProvider.isPluginDisabled("plugin-2"));
         assertFalse(statusProvider.isPluginDisabled("plugin-3"));
     }
 
     @Test
-    public void testEnablePlugin() throws IOException {
+    public void testEnablePlugin() throws Exception {
         createEnabledFile();
 
         PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath);
+        statusProvider.enablePlugin("plugin-2");
 
-        assertTrue(statusProvider.enablePlugin("plugin-2"));
         assertFalse(statusProvider.isPluginDisabled("plugin-1"));
         assertFalse(statusProvider.isPluginDisabled("plugin-2"));
         assertTrue(statusProvider.isPluginDisabled("plugin-3"));
     }
 
     @Test
-    public void testEnablePluginWithEnableEmpty() {
+    public void testEnablePluginWithEnableEmpty() throws Exception{
         PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath);
+        statusProvider.enablePlugin("plugin-2");
 
-        assertTrue(statusProvider.enablePlugin("plugin-2"));
         assertFalse(statusProvider.isPluginDisabled("plugin-1"));
         assertFalse(statusProvider.isPluginDisabled("plugin-2"));
         assertFalse(statusProvider.isPluginDisabled("plugin-3"));
     }
 
     @Test
-    public void testDisablePluginWithoutDisabledFile() {
+    public void testDisablePluginWithoutDisabledFile() throws Exception {
         PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath);
 
         assertFalse(statusProvider.isPluginDisabled("plugin-1"));
-        assertTrue(statusProvider.disablePlugin("plugin-1"));
+
+        statusProvider.disablePlugin("plugin-1");
         assertTrue(statusProvider.isPluginDisabled("plugin-1"));
     }
 
diff --git a/pf4j/src/test/java/org/pf4j/JarPluginRepositoryTest.java b/pf4j/src/test/java/org/pf4j/JarPluginRepositoryTest.java
index 2260680..02d2bcc 100644
--- a/pf4j/src/test/java/org/pf4j/JarPluginRepositoryTest.java
+++ b/pf4j/src/test/java/org/pf4j/JarPluginRepositoryTest.java
@@ -18,7 +18,6 @@ package org.pf4j;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 
-import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.List;
@@ -39,7 +38,7 @@ public class JarPluginRepositoryTest {
      * Test of {@link JarPluginRepository#deletePluginPath(Path)} method.
      */
     @Test
-    public void testDeletePluginPath() throws IOException {
+    public void testDeletePluginPath() throws Exception {
         PluginRepository repository = new JarPluginRepository(pluginsPath);
 
         Path plugin1Path = Files.createDirectory(pluginsPath.resolve("plugin-1"));