Changeset 15508 in josm

Timestamp:

2019-11-04T22:19:54+01:00 (4 years ago)

Author:

Don-vip

Message:

fix #18277 - Allow plugins to implement Destroyable if they want to allow restartless updates/removals (patch by taylor.smock)

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/preferences/plugin/PluginPreference.java (modified) (2 diffs)
• src/org/openstreetmap/josm/plugins/PluginHandler.java (modified) (4 diffs)
• test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java (modified) (8 diffs)

trunk/src/org/openstreetmap/josm/gui/preferences/plugin/PluginPreference.java

@@ -56 +56 @@
 import org.openstreetmap.josm.gui.widgets.FilterField;
 import org.openstreetmap.josm.plugins.PluginDownloadTask;
+import org.openstreetmap.josm.plugins.PluginHandler;
 import org.openstreetmap.josm.plugins.PluginInformation;
 import org.openstreetmap.josm.plugins.ReadLocalPluginInformationTask;
@@ -325 +326 @@
             Collections.sort(l);
             Config.getPref().putList("plugins", l);
-            if (!model.getNewlyDeactivatedPlugins().isEmpty())
-                return true;
+            List<PluginInformation> deactivatedPlugins = model.getNewlyDeactivatedPlugins();
+            if (!deactivatedPlugins.isEmpty()) {
+                boolean requiresRestart = PluginHandler.removePlugins(deactivatedPlugins);
+                if (requiresRestart)
+                    return requiresRestart;
+            }
             for (PluginInformation pi : model.getNewlyActivatedPlugins()) {
                 if (!pi.canloadatruntime)

trunk/src/org/openstreetmap/josm/plugins/PluginHandler.java

@@ -69 +69 @@
 import org.openstreetmap.josm.io.OnlineResource;
 import org.openstreetmap.josm.spi.preferences.Config;
+import org.openstreetmap.josm.tools.Destroyable;
 import org.openstreetmap.josm.tools.GBC;
 import org.openstreetmap.josm.tools.I18n;
@@ -1169 +1170 @@
                         tr("Update plugins")
                 );
-
                 try {
                     pluginDownloadTask.run();
@@ -1333 +1333 @@
                     pluginsToLoad.stream().filter(x -> x.libraries.contains(oldPluginURL)).forEach(
                             x -> Collections.replaceAll(x.libraries, oldPluginURL, newPluginURL));
+
+                    // Attempt to update loaded plugin (must implement Destroyable)
+                    PluginInformation tInfo = pluginsToLoad.parallelStream()
+                            .filter(x -> x.libraries.contains(newPluginURL)).findAny().orElse(null);
+                    if (tInfo != null) {
+                        Object tUpdatedPlugin = getPlugin(tInfo.name);
+                        if (tUpdatedPlugin instanceof Destroyable) {
+                            ((Destroyable) tUpdatedPlugin).destroy();
+                            PluginHandler.loadPlugins(getInfoPanel(), Collections.singleton(tInfo),
+                                    NullProgressMonitor.INSTANCE);
+                        }
+                    }
                 } catch (MalformedURLException e) {
                     Logging.warn(e);
@@ -1644 +1656 @@
         }
     }
+
+    /**
+     * Remove deactivated plugins, returning true if JOSM should restart
+     *
+     * @param deactivatedPlugins The plugins to deactivate
+     *
+     * @return true if there was a plugin that requires a restart
+     * @since 15508
+     */
+    public static boolean removePlugins(List<PluginInformation> deactivatedPlugins) {
+        List<Destroyable> noRestart = deactivatedPlugins.parallelStream()
+                .map(info -> PluginHandler.getPlugin(info.name)).filter(Destroyable.class::isInstance)
+                .map(Destroyable.class::cast).collect(Collectors.toList());
+        boolean restartNeeded;
+        try {
+            noRestart.forEach(Destroyable::destroy);
+            new ArrayList<>(pluginList).stream().filter(proxy -> noRestart.contains(proxy.getPlugin()))
+                    .forEach(pluginList::remove);
+            restartNeeded = deactivatedPlugins.size() != noRestart.size();
+        } catch (Exception e) {
+            Logging.error(e);
+            restartNeeded = true;
+        }
+        return restartNeeded;
+    }
 }

trunk/test/unit/org/openstreetmap/josm/plugins/PluginHandlerTestIT.java

@@ -8 +8 @@
 import java.awt.GraphicsEnvironment;
 import java.awt.HeadlessException;
+import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -14 +16 @@
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.function.Consumer;
@@ -19 +22 @@
 
 import org.apache.commons.lang3.exception.ExceptionUtils;
-import org.junit.Rule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
 import org.junit.Test;
+import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.Preferences;
 import org.openstreetmap.josm.data.gpx.GpxData;
@@ -31 +36 @@
 import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
+import org.openstreetmap.josm.tools.Destroyable;
 import org.openstreetmap.josm.tools.Utils;
 
@@ -40 +46 @@
 public class PluginHandlerTestIT {
 
+    private static List<String> errorsToIgnore = new ArrayList<>();
     /**
      * Setup test.
      */
-    @Rule
+    @ClassRule
     @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules().main().projection().preferences().https().timeout(10*60*1000);
+    public static JOSMTestRules test = new JOSMTestRules().main().projection().preferences().https()
+            .timeout(10 * 60 * 1000);
+
+    /**
+     * Setup test
+     *
+     * @throws IOException in case of I/O error
+     */
+    @BeforeClass
+    public static void beforeClass() throws IOException {
+        errorsToIgnore.addAll(TestUtils.getIgnoredErrorMessages(PluginHandlerTestIT.class));
+    }
 
     /**
@@ -75 +93 @@
         }
 
+        Map<String, Throwable> noRestartExceptions = new HashMap<>();
+        testCompletelyRestartlessPlugins(loadedPlugins, noRestartExceptions);
+
         debugPrint(invalidManifestEntries);
         debugPrint(loadingExceptions);
         debugPrint(layerExceptions);
+        debugPrint(noRestartExceptions);
+
+        invalidManifestEntries = filterKnownErrors(invalidManifestEntries);
+        loadingExceptions = filterKnownErrors(loadingExceptions);
+        layerExceptions = filterKnownErrors(layerExceptions);
+        noRestartExceptions = filterKnownErrors(noRestartExceptions);
+
         String msg = Arrays.toString(invalidManifestEntries.entrySet().toArray()) + '\n' +
                      Arrays.toString(loadingExceptions.entrySet().toArray()) + '\n' +
-                     Arrays.toString(layerExceptions.entrySet().toArray());
+                Arrays.toString(layerExceptions.entrySet().toArray()) + '\n'
+                + Arrays.toString(noRestartExceptions.entrySet().toArray());
         assertTrue(msg, invalidManifestEntries.isEmpty() && loadingExceptions.isEmpty() && layerExceptions.isEmpty());
+    }
+
+    private static void testCompletelyRestartlessPlugins(List<PluginInformation> loadedPlugins,
+            Map<String, Throwable> noRestartExceptions) {
+        try {
+            List<PluginInformation> restartable = loadedPlugins.parallelStream()
+                    .filter(info -> PluginHandler.getPlugin(info.name) instanceof Destroyable)
+                    .collect(Collectors.toList());
+            // ensure good plugin behavior with regards to Destroyable (i.e., they can be
+            // removed and readded)
+            for (int i = 0; i < 2; i++) {
+                assertFalse(PluginHandler.removePlugins(restartable));
+                assertTrue(restartable.stream().noneMatch(info -> PluginHandler.getPlugins().contains(info)));
+                loadPlugins(restartable);
+            }
+
+            assertTrue(PluginHandler.removePlugins(loadedPlugins));
+            assertTrue(restartable.parallelStream().noneMatch(info -> PluginHandler.getPlugins().contains(info)));
+        } catch (Exception | LinkageError t) {
+            Throwable root = ExceptionUtils.getRootCause(t);
+            root.printStackTrace();
+            noRestartExceptions.put(findFaultyPlugin(loadedPlugins, root), root);
+        }
+    }
+
+    private static <T> Map<String, T> filterKnownErrors(Map<String, T> errorMap) {
+        return errorMap.entrySet().parallelStream()
+                .filter(entry -> !errorsToIgnore.contains(convertEntryToString(entry)))
+                .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
     }
 
@@ -87 +145 @@
         System.out.println(invalidManifestEntries.entrySet()
                 .stream()
-                .map(e -> e.getKey() + "=\"" + e.getValue() + "\"")
+                .map(e -> convertEntryToString(e))
                 .collect(Collectors.joining(", ")));
+    }
+
+    private static String convertEntryToString(Entry<String, ?> entry) {
+        return entry.getKey() + "=\"" + entry.getValue() + "\"";
     }
 
@@ -134 +196 @@
         downloadPlugins(plugins);
 
+        loadPlugins(plugins);
+    }
+
+    static void loadPlugins(List<PluginInformation> plugins) {
         // Load early plugins
         PluginHandler.loadEarlyPlugins(null, plugins, null);