From 8771d6d4d770a8844cfdb841304a98293bfe4ff2 Mon Sep 17 00:00:00 2001 From: Giovanni Giannola Date: Wed, 29 Jul 2026 15:45:33 -0400 Subject: [PATCH 1/2] 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 --- .../DefaultExtensionController.java | 42 ++++++-- .../DefaultExtensionControllerTest.java | 98 +++++++++++++++++++ 2 files changed, 132 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java b/server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java index e3a076c91e..e212a14b47 100644 --- a/server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java +++ b/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. + *

+ * 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. + *

+ * Instances are compared by identity on purpose. Two distinct plugin instances must both be + * registered even if the plugin class considers them equal. + *

+ * 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 */ /* ********************************************************************** */ diff --git a/server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java b/server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java index 984607dcdf..f464aca265 100644 --- a/server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java +++ b/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 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(); + } + } } \ No newline at end of file From 69962aef0960c93921a558bffd6af1bfa5bc80c6 Mon Sep 17 00:00:00 2001 From: Giovanni Giannola Date: Wed, 29 Jul 2026 21:44:13 -0400 Subject: [PATCH 2/2] Use a Set for serverPlugins instead of a de-duplicating adder Addresses review feedback on #397. Rather than routing the eight registrations through a helper that scans the List, serverPlugins is now a Set so the de-duplication is the natural behaviour of the collection and the registration calls stay as they were. LinkedHashSet rather than HashSet: initPlugins loads plugins in a deliberate order (descending plugin weight), and that order is preserved when starting and stopping them. getServerPlugins still returns a List so the ExtensionController signature is unchanged for extensions. The unit tests added in the previous commit are removed. They exercised the helper directly, and with a Set the invariant is enforced by the collection type rather than by logic of our own. Signed-off-by: Giovanni Giannola --- .../DefaultExtensionController.java | 54 ++++------ .../DefaultExtensionControllerTest.java | 98 ------------------- 2 files changed, 18 insertions(+), 134 deletions(-) diff --git a/server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java b/server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java index e212a14b47..77b02c1874 100644 --- a/server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java +++ b/server/src/main/java/com/mirth/connect/server/controllers/DefaultExtensionController.java @@ -84,7 +84,14 @@ public class DefaultExtensionController extends ExtensionController { // these are plugins for specific extension points, keyed by plugin name // (not path) - private List serverPlugins = new ArrayList(); + /* + * A plugin class may implement several of the plugin type interfaces, in which case it is + * registered once for each interface it implements. A Set holds a single entry per plugin, + * so start() and stop() are invoked once per plugin rather than once per interface. + * LinkedHashSet because initPlugins loads plugins in a deliberate order (by plugin weight) + * and that order is preserved when they are started and stopped. + */ + private Set serverPlugins = new LinkedHashSet(); private Map servicePlugins = new LinkedHashMap(); private Map channelPlugins = new LinkedHashMap(); private Map codeTemplateServerPlugins = new LinkedHashMap(); @@ -240,42 +247,42 @@ public void initPlugins() { */ servicePlugin.init(currentProperties); servicePlugins.put(servicePlugin.getPluginPointName(), servicePlugin); - addServerPlugin(servicePlugin); + serverPlugins.add(servicePlugin); logger.debug("sucessfully loaded server plugin: " + serverPlugin.getPluginPointName()); } if (serverPlugin instanceof ChannelPlugin) { ChannelPlugin channelPlugin = (ChannelPlugin) serverPlugin; channelPlugins.put(channelPlugin.getPluginPointName(), channelPlugin); - addServerPlugin(channelPlugin); + serverPlugins.add(channelPlugin); logger.debug("sucessfully loaded server channel plugin: " + serverPlugin.getPluginPointName()); } if (serverPlugin instanceof CodeTemplateServerPlugin) { CodeTemplateServerPlugin codeTemplateServerPlugin = (CodeTemplateServerPlugin) serverPlugin; codeTemplateServerPlugins.put(codeTemplateServerPlugin.getPluginPointName(), codeTemplateServerPlugin); - addServerPlugin(codeTemplateServerPlugin); + serverPlugins.add(codeTemplateServerPlugin); logger.debug("sucessfully loaded server code template plugin: " + serverPlugin.getPluginPointName()); } if (serverPlugin instanceof DataTypeServerPlugin) { DataTypeServerPlugin dataTypePlugin = (DataTypeServerPlugin) serverPlugin; dataTypePlugins.put(dataTypePlugin.getPluginPointName(), dataTypePlugin); - addServerPlugin(dataTypePlugin); + serverPlugins.add(dataTypePlugin); logger.debug("sucessfully loaded server data type plugin: " + serverPlugin.getPluginPointName()); } if (serverPlugin instanceof ResourcePlugin) { ResourcePlugin resourcePlugin = (ResourcePlugin) serverPlugin; resourcePlugins.put(resourcePlugin.getPluginPointName(), resourcePlugin); - addServerPlugin(resourcePlugin); + serverPlugins.add(resourcePlugin); logger.debug("Successfully loaded resource plugin: " + resourcePlugin.getPluginPointName()); } if (serverPlugin instanceof TransmissionModeProvider) { TransmissionModeProvider transmissionModeProvider = (TransmissionModeProvider) serverPlugin; transmissionModeProviders.put(transmissionModeProvider.getPluginPointName(), transmissionModeProvider); - addServerPlugin(transmissionModeProvider); + serverPlugins.add(transmissionModeProvider); logger.debug("Successfully loaded transmission mode provider plugin: " + transmissionModeProvider.getPluginPointName()); } @@ -287,7 +294,7 @@ public void initPlugins() { } this.authorizationPlugin = authorizationPlugin; - addServerPlugin(authorizationPlugin); + serverPlugins.add(authorizationPlugin); logger.debug("sucessfully loaded server authorization plugin: " + serverPlugin.getPluginPointName()); } @@ -299,7 +306,7 @@ public void initPlugins() { } this.multiFactorAuthenticationPlugin = multiFactorAuthenticationPlugin; - addServerPlugin(multiFactorAuthenticationPlugin); + serverPlugins.add(multiFactorAuthenticationPlugin); logger.debug("sucessfully loaded server multi-factor authentication plugin: " + serverPlugin.getPluginPointName()); } } catch (Exception e) { @@ -309,32 +316,6 @@ public void initPlugins() { } } - /** - * Registers a plugin in the list used to start and stop all server plugins. - *

- * 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. - *

- * Instances are compared by identity on purpose. Two distinct plugin instances must both be - * registered even if the plugin class considers them equal. - *

- * 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 */ /* ********************************************************************** */ @@ -742,7 +723,8 @@ public List getClientLibraries() { } public List getServerPlugins() { - return serverPlugins; + // Copied into a List so the ExtensionController signature stays unchanged for extensions. + return new ArrayList(serverPlugins); } void extractZipEntry(ZipEntry entry, File installTempDir, ZipFile zipFile) throws IOException { diff --git a/server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java b/server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java index f464aca265..984607dcdf 100644 --- a/server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java +++ b/server/src/test/java/com/mirth/connect/server/controllers/DefaultExtensionControllerTest.java @@ -9,12 +9,9 @@ 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; @@ -23,7 +20,6 @@ import org.junit.Before; import org.junit.Test; -import com.mirth.connect.plugins.ServerPlugin; import com.mirth.connect.util.ZipTestUtils; public class DefaultExtensionControllerTest { @@ -76,98 +72,4 @@ 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 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(); - } - } } \ No newline at end of file