PluginManager: require start/stop to be invoked on the EDT

The current way of invoking it on the executor would let the client have
a start and a stop for a plugin sitting in the executor's queue, making
it toggle endlessly. This only was because we were using invokeAndWait
in startPlugin, which can't be invoked on the EDT because it would
deadlock with itself.

runelite-client/src/main/java/net/runelite/client/externalplugins/ExternalPluginManager.java

@@ -33,6 +33,7 @@ import com.google.common.hash.HashingInputStream;
 import com.google.common.io.Files;
 import java.io.File;
 import java.io.IOException;
+import java.lang.reflect.InvocationTargetException;
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -44,6 +45,7 @@ import java.util.concurrent.ScheduledExecutorService;
 import java.util.function.Function;
 import javax.inject.Inject;
 import javax.inject.Singleton;
+import javax.swing.SwingUtilities;
 import lombok.extern.slf4j.Slf4j;
 import net.runelite.client.RuneLite;
 import net.runelite.client.RuneLiteProperties;
@@ -242,9 +244,19 @@ public class ExternalPluginManager
 				log.info("Stopping external plugin \"{}\"", p.getClass());
 				try
 				{
-					pluginManager.stopPlugin(p);
+					SwingUtilities.invokeAndWait(() ->
+					{
+						try
+						{
+							pluginManager.stopPlugin(p);
+						}
+						catch (Exception e)
+						{
+							throw new RuntimeException(e);
+						}
+					});
 				}
-				catch (PluginInstantiationException e)
+				catch (InterruptedException | InvocationTargetException e)
 				{
 					log.warn("Unable to stop external plugin \"{}\"", p.getClass().getName(), e);
 				}
@@ -272,15 +284,25 @@ public class ExternalPluginManager
 						clazzes.add(cl.loadClass(className));
 					}
 
-					newPlugins = pluginManager.loadPlugins(clazzes, null);
+					List<Plugin> newPlugins2 = newPlugins = pluginManager.loadPlugins(clazzes, null);
 					if (!startup)
 					{
 						pluginManager.loadDefaultPluginConfiguration(newPlugins);
 
-						for (Plugin p : newPlugins)
+						SwingUtilities.invokeAndWait(() ->
 						{
-							pluginManager.startPlugin(p);
+							try
-						}
+							{
+								for (Plugin p : newPlugins2)
+								{
+									pluginManager.startPlugin(p);
+								}
+							}
+							catch (PluginInstantiationException e)
+							{
+								throw new RuntimeException(e);
+							}
+						});
 					}
 				}
 				catch (Exception e)
@@ -292,10 +314,21 @@ public class ExternalPluginManager
 						{
 							try
 							{
-								pluginManager.stopPlugin(p);
+								SwingUtilities.invokeAndWait(() ->
+								{
+									try
+									{
+										pluginManager.stopPlugin(p);
+									}
+									catch (Exception e2)
+									{
+										throw new RuntimeException(e2);
+									}
+								});
 							}
-							catch (Exception inner)
+							catch (InterruptedException | InvocationTargetException e2)
 							{
+								log.info("Unable to fully stop plugin \"{}\"", manifest.getInternalName(), e2);
 							}
 							pluginManager.remove(p);
 						}

runelite-client/src/main/java/net/runelite/client/plugins/PluginManager.java

@@ -132,8 +132,9 @@ public class PluginManager
 	private void refreshPlugins()
 	{
 		loadDefaultPluginConfiguration(null);
-		getPlugins()
+		SwingUtilities.invokeLater(() ->
-			.forEach(plugin -> executor.submit(() ->
+		{
+			for (Plugin plugin : getPlugins())
 			{
 				try
 				{
@@ -153,7 +154,8 @@ public class PluginManager
 				{
 					log.warn("Error during starting/stopping plugin {}", plugin.getClass().getSimpleName(), e);
 				}
-			}));
+			}
+		});
 	}
 
 	public Config getPluginConfigProxy(Plugin plugin)
@@ -215,12 +217,22 @@ public class PluginManager
 		{
 			try
 			{
-				startPlugin(plugin);
+				SwingUtilities.invokeAndWait(() ->
+				{
+					try
+					{
+						startPlugin(plugin);
+					}
+					catch (PluginInstantiationException ex)
+					{
+						log.warn("Unable to start plugin {}", plugin.getClass().getSimpleName(), ex);
+						plugins.remove(plugin);
+					}
+				});
 			}
-			catch (PluginInstantiationException ex)
+			catch (InterruptedException | InvocationTargetException e)
 			{
-				log.warn("Unable to start plugin {}", plugin.getClass().getSimpleName(), ex);
+				throw new RuntimeException(e);
-				plugins.remove(plugin);
 			}
 
 			loaded++;
@@ -325,8 +337,11 @@ public class PluginManager
 		return newPlugins;
 	}
 
-	public synchronized boolean startPlugin(Plugin plugin) throws PluginInstantiationException
+	public boolean startPlugin(Plugin plugin) throws PluginInstantiationException
 	{
+		// plugins always start in the EDT
+		assert SwingUtilities.isEventDispatchThread();
+
 		if (activePlugins.contains(plugin) || !isPluginEnabled(plugin))
 		{
 			return false;
@@ -336,18 +351,7 @@ public class PluginManager
 
 		try
 		{
-			// plugins always start in the event thread
+			plugin.startUp();
-			SwingUtilities.invokeAndWait(() ->
-			{
-				try
-				{
-					plugin.startUp();
-				}
-				catch (Exception ex)
-				{
-					throw new RuntimeException(ex);
-				}
-			});
 
 			log.debug("Plugin {} is now running", plugin.getClass().getSimpleName());
 			if (!isOutdated && sceneTileManager != null)
@@ -363,7 +367,7 @@ public class PluginManager
 			schedule(plugin);
 			eventBus.post(new PluginChanged(plugin, true));
 		}
-		catch (InterruptedException | InvocationTargetException | IllegalArgumentException ex)
+		catch (Exception ex)
 		{
 			throw new PluginInstantiationException(ex);
 		}
@@ -371,36 +375,27 @@ public class PluginManager
 		return true;
 	}
 
-	public synchronized boolean stopPlugin(Plugin plugin) throws PluginInstantiationException
+	public boolean stopPlugin(Plugin plugin) throws PluginInstantiationException
 	{
+		// plugins always stop in the EDT
+		assert SwingUtilities.isEventDispatchThread();
+
 		if (!activePlugins.remove(plugin))
 		{
 			return false;
 		}
 
+		unschedule(plugin);
+		eventBus.unregister(plugin);
+
 		try
 		{
-			unschedule(plugin);
+			plugin.shutDown();
-			eventBus.unregister(plugin);
-
-			// plugins always stop in the event thread
-			SwingUtilities.invokeAndWait(() ->
-			{
-				try
-				{
-					plugin.shutDown();
-				}
-				catch (Exception ex)
-				{
-					throw new RuntimeException(ex);
-				}
-			});
 
 			log.debug("Plugin {} is now stopped", plugin.getClass().getSimpleName());
 			eventBus.post(new PluginChanged(plugin, false));
-
 		}
-		catch (InterruptedException | InvocationTargetException ex)
+		catch (Exception ex)
 		{
 			throw new PluginInstantiationException(ex);
 		}
@@ -555,6 +550,7 @@ public class PluginManager
 
 	/**
 	 * Topologically sort a graph. Uses Kahn's algorithm.
+	 *
 	 * @param graph
 	 * @param <T>
 	 * @return

runelite-client/src/main/java/net/runelite/client/plugins/config/PluginListPanel.java

@@ -311,36 +311,30 @@ class PluginListPanel extends PluginPanel
 
 	void startPlugin(Plugin plugin)
 	{
-		executorService.submit(() ->
+		pluginManager.setPluginEnabled(plugin, true);
-		{
-			pluginManager.setPluginEnabled(plugin, true);
 
-			try
+		try
-			{
+		{
-				pluginManager.startPlugin(plugin);
+			pluginManager.startPlugin(plugin);
-			}
+		}
-			catch (PluginInstantiationException ex)
+		catch (PluginInstantiationException ex)
-			{
+		{
-				log.warn("Error when starting plugin {}", plugin.getClass().getSimpleName(), ex);
+			log.warn("Error when starting plugin {}", plugin.getClass().getSimpleName(), ex);
-			}
+		}
-		});
 	}
 
 	void stopPlugin(Plugin plugin)
 	{
-		executorService.submit(() ->
+		pluginManager.setPluginEnabled(plugin, false);
-		{
-			pluginManager.setPluginEnabled(plugin, false);
 
-			try
+		try
-			{
+		{
-				pluginManager.stopPlugin(plugin);
+			pluginManager.stopPlugin(plugin);
-			}
+		}
-			catch (PluginInstantiationException ex)
+		catch (PluginInstantiationException ex)
-			{
+		{
-				log.warn("Error when stopping plugin {}", plugin.getClass().getSimpleName(), ex);
+			log.warn("Error when stopping plugin {}", plugin.getClass().getSimpleName(), ex);
-			}
+		}
-		});
 	}
 
 	private List<String> getPinnedPluginNames()

runelite-client/src/main/java/net/runelite/client/plugins/gpu/GpuPlugin.java

@@ -48,6 +48,7 @@ import java.nio.FloatBuffer;
 import java.nio.IntBuffer;
 import java.util.function.Function;
 import javax.inject.Inject;
+import javax.swing.SwingUtilities;
 import jogamp.nativewindow.SurfaceScaleUtils;
 import jogamp.nativewindow.jawt.x11.X11JAWTWindow;
 import jogamp.newt.awt.NewtFactoryAWT;
@@ -334,15 +335,18 @@ public class GpuPlugin extends Plugin implements DrawCallbacks
 			{
 				log.error("Error starting GPU plugin", e);
 
-				try
+				SwingUtilities.invokeLater(() ->
 				{
-					pluginManager.setPluginEnabled(this, false);
+					try
-					pluginManager.stopPlugin(this);
+					{
-				}
+						pluginManager.setPluginEnabled(this, false);
-				catch (PluginInstantiationException ex)
+						pluginManager.stopPlugin(this);
-				{
+					}
-					log.error("error stopping plugin", ex);
+					catch (PluginInstantiationException ex)
-				}
+					{
+						log.error("error stopping plugin", ex);
+					}
+				});
 
 				shutDown();
 			}