Fix #120: start and stop each server plugin only once

initPlugins registers a plugin instance once for every plugin type
interface it implements, adding it to the serverPlugins list up to eight
times. startPlugins and stopPlugins iterate that list, so a plugin
implementing more than one interface (for example both ServicePlugin and
ChannelPlugin) had start() and stop() invoked once per interface instead
of once per plugin.

Route the registrations through a helper that ignores an instance which
is already registered. The comparison is by identity so that two
distinct instances are still both registered even if the plugin class
reports them as equal.

Adds tests covering duplicate registration, stop() being called once for
a plugin registered for multiple types, and distinct-but-equal instances
still being registered separately.

Signed-off-by: Giovanni Giannola <giogiannola@globalesm.com>

Author: ggiannola

server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java

@@ -240,42 +240,42 @@ public void initPlugins() {
                          */
                         servicePlugin.init(currentProperties);
                         servicePlugins.put(servicePlugin.getPluginPointName(), servicePlugin);
-                        serverPlugins.add(servicePlugin);
+                        addServerPlugin(servicePlugin);
                         logger.debug("sucessfully loaded server plugin: " + serverPlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof ChannelPlugin) {
                         ChannelPlugin channelPlugin = (ChannelPlugin) serverPlugin;
                         channelPlugins.put(channelPlugin.getPluginPointName(), channelPlugin);
-                        serverPlugins.add(channelPlugin);
+                        addServerPlugin(channelPlugin);
                         logger.debug("sucessfully loaded server channel plugin: " + serverPlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof CodeTemplateServerPlugin) {
                         CodeTemplateServerPlugin codeTemplateServerPlugin = (CodeTemplateServerPlugin) serverPlugin;
                         codeTemplateServerPlugins.put(codeTemplateServerPlugin.getPluginPointName(), codeTemplateServerPlugin);
-                        serverPlugins.add(codeTemplateServerPlugin);
+                        addServerPlugin(codeTemplateServerPlugin);
                         logger.debug("sucessfully loaded server code template plugin: " + serverPlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof DataTypeServerPlugin) {
                         DataTypeServerPlugin dataTypePlugin = (DataTypeServerPlugin) serverPlugin;
                         dataTypePlugins.put(dataTypePlugin.getPluginPointName(), dataTypePlugin);
-                        serverPlugins.add(dataTypePlugin);
+                        addServerPlugin(dataTypePlugin);
                         logger.debug("sucessfully loaded server data type plugin: " + serverPlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof ResourcePlugin) {
                         ResourcePlugin resourcePlugin = (ResourcePlugin) serverPlugin;
                         resourcePlugins.put(resourcePlugin.getPluginPointName(), resourcePlugin);
-                        serverPlugins.add(resourcePlugin);
+                        addServerPlugin(resourcePlugin);
                         logger.debug("Successfully loaded resource plugin: " + resourcePlugin.getPluginPointName());
                     }
 
                     if (serverPlugin instanceof TransmissionModeProvider) {
                         TransmissionModeProvider transmissionModeProvider = (TransmissionModeProvider) serverPlugin;
                         transmissionModeProviders.put(transmissionModeProvider.getPluginPointName(), transmissionModeProvider);
-                        serverPlugins.add(transmissionModeProvider);
+                        addServerPlugin(transmissionModeProvider);
                         logger.debug("Successfully loaded transmission mode provider plugin: " + transmissionModeProvider.getPluginPointName());
                     }
 
@@ -287,7 +287,7 @@ public void initPlugins() {
                         }
 
                         this.authorizationPlugin = authorizationPlugin;
-                        serverPlugins.add(authorizationPlugin);
+                        addServerPlugin(authorizationPlugin);
                         logger.debug("sucessfully loaded server authorization plugin: " + serverPlugin.getPluginPointName());
                     }
 
@@ -299,7 +299,7 @@ public void initPlugins() {
                         }
 
                         this.multiFactorAuthenticationPlugin = multiFactorAuthenticationPlugin;
-                        serverPlugins.add(multiFactorAuthenticationPlugin);
+                        addServerPlugin(multiFactorAuthenticationPlugin);
                         logger.debug("sucessfully loaded server multi-factor authentication plugin: " + serverPlugin.getPluginPointName());
                     }
                 } catch (Exception e) {
@@ -309,6 +309,32 @@ public void initPlugins() {
         }
     }
 
+    /**
+     * Registers a plugin in the list used to start and stop all server plugins.
+     * <p>
+     * A single plugin class may implement more than one of the plugin type interfaces (for example
+     * both {@link ServicePlugin} and {@link ChannelPlugin}). Such a plugin is registered against
+     * each type it implements, so this method guards against adding the same instance more than
+     * once. Without the guard, {@link #startPlugins()} and {@link #stopPlugins()} would invoke
+     * {@link ServerPlugin#start()} and {@link ServerPlugin#stop()} once per implemented interface
+     * rather than once per plugin.
+     * <p>
+     * Instances are compared by identity on purpose. Two distinct plugin instances must both be
+     * registered even if the plugin class considers them equal.
+     * <p>
+     * Package private so it can be exercised directly by unit tests without going through the full
+     * extension loading process.
+     */
+    void addServerPlugin(ServerPlugin serverPlugin) {
+        for (ServerPlugin registeredPlugin : serverPlugins) {
+            if (registeredPlugin == serverPlugin) {
+                return;
+            }
+        }
+
+        serverPlugins.add(serverPlugin);
+    }
+
     /* These are the maps for the different types of plugins */
     /* ********************************************************************** */
 

server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java

@@ -9,9 +9,12 @@
 
 package com.mirth.connect.server.controllers;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
 import java.io.File;
+import java.util.List;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipException;
 import java.util.zip.ZipFile;
@@ -20,6 +23,7 @@
 import org.junit.Before;
 import org.junit.Test;
 
+import com.mirth.connect.plugins.ServerPlugin;
 import com.mirth.connect.util.ZipTestUtils;
 
 public class DefaultExtensionControllerTest {
@@ -72,4 +76,98 @@ public void cleanupTestFolder() {
     private ZipFile createTempZipFile(String fileName) throws Exception {
         return new ZipFile(ZipTestUtils.createTempZipFile(fileName));
     }
+
+    /*
+     * A plugin class may implement several of the plugin type interfaces (ServicePlugin,
+     * ChannelPlugin, and so on). initPlugins registers the instance once per interface it
+     * implements, so registration must ignore an instance that is already registered. Otherwise
+     * start() and stop() get invoked once per implemented interface instead of once per plugin.
+     */
+    @Test
+    public void testAddServerPluginIgnoresDuplicateRegistrationOfSameInstance() {
+        DefaultExtensionController extensionController = new DefaultExtensionController();
+        CountingServerPlugin plugin = new CountingServerPlugin("multi-type plugin");
+
+        extensionController.addServerPlugin(plugin);
+        extensionController.addServerPlugin(plugin);
+        extensionController.addServerPlugin(plugin);
+
+        List<ServerPlugin> registeredPlugins = extensionController.getServerPlugins();
+        assertEquals(1, registeredPlugins.size());
+        assertSame(plugin, registeredPlugins.get(0));
+    }
+
+    @Test
+    public void testStopPluginsStopsPluginRegisteredForMultipleTypesOnce() {
+        DefaultExtensionController extensionController = new DefaultExtensionController();
+        CountingServerPlugin plugin = new CountingServerPlugin("multi-type plugin");
+
+        extensionController.addServerPlugin(plugin);
+        extensionController.addServerPlugin(plugin);
+
+        extensionController.stopPlugins();
+
+        assertEquals(1, plugin.stopCount);
+    }
+
+    /*
+     * Registration deliberately compares instances by identity, so two separate plugin instances
+     * are both registered and both stopped even when the plugin class reports them as equal.
+     */
+    @Test
+    public void testAddServerPluginRegistersDistinctButEqualInstancesSeparately() {
+        DefaultExtensionController extensionController = new DefaultExtensionController();
+        AlwaysEqualServerPlugin firstPlugin = new AlwaysEqualServerPlugin();
+        AlwaysEqualServerPlugin secondPlugin = new AlwaysEqualServerPlugin();
+
+        extensionController.addServerPlugin(firstPlugin);
+        extensionController.addServerPlugin(secondPlugin);
+
+        assertEquals(2, extensionController.getServerPlugins().size());
+
+        extensionController.stopPlugins();
+
+        assertEquals(1, firstPlugin.stopCount);
+        assertEquals(1, secondPlugin.stopCount);
+    }
+
+    private static class CountingServerPlugin implements ServerPlugin {
+        private final String pluginPointName;
+        int stopCount;
+
+        private CountingServerPlugin(String pluginPointName) {
+            this.pluginPointName = pluginPointName;
+        }
+
+        @Override
+        public String getPluginPointName() {
+            return pluginPointName;
+        }
+
+        @Override
+        public void start() {
+            // Not exercised here: startPlugins() also reaches into ControllerFactory.
+        }
+
+        @Override
+        public void stop() {
+            stopCount++;
+        }
+    }
+
+    private static class AlwaysEqualServerPlugin extends CountingServerPlugin {
+        private AlwaysEqualServerPlugin() {
+            super("always equal plugin");
+        }
+
+        @Override
+        public boolean equals(Object other) {
+            return other instanceof AlwaysEqualServerPlugin;
+        }
+
+        @Override
+        public int hashCode() {
+            return AlwaysEqualServerPlugin.class.hashCode();
+        }
+    }
 }