Merge pull request #2678 from swazrgb/single-executor

runelite-client/src/main/java/net/runelite/client/RuneLiteModule.java

@@ -24,13 +24,18 @@
  */
 package net.runelite.client;
 
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import com.google.inject.AbstractModule;
 import com.google.inject.Provides;
 import com.google.inject.name.Names;
 import java.applet.Applet;
 import java.io.File;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 import java.util.function.Supplier;
 import javax.annotation.Nullable;
 import javax.inject.Singleton;
@@ -51,6 +56,7 @@ import net.runelite.client.plugins.PluginManager;
 import net.runelite.client.task.Scheduler;
 import net.runelite.client.util.DeferredEventBus;
 import net.runelite.client.util.ExecutorServiceExceptionLogger;
+import net.runelite.client.util.NonScheduledExecutorServiceExceptionLogger;
 import net.runelite.http.api.RuneLiteAPI;
 import okhttp3.Cache;
 import okhttp3.OkHttpClient;
@@ -74,7 +80,6 @@ public class RuneLiteModule extends AbstractModule
 	protected void configure()
 	{
 		bind(File.class).annotatedWith(Names.named("config")).toInstance(config);
-		bind(ScheduledExecutorService.class).toInstance(new ExecutorServiceExceptionLogger(Executors.newSingleThreadScheduledExecutor()));
 		bind(OkHttpClient.class).toInstance(RuneLiteAPI.CLIENT.newBuilder()
 			.cache(new Cache(new File(RuneLite.CACHE_DIR, "okhttp"), MAX_OKHTTP_CACHE_SIZE))
 			.build());
@@ -140,4 +145,30 @@ public class RuneLiteModule extends AbstractModule
 	{
 		return configManager.getConfig(LauncherConfig.class);
 	}
+
+	@Provides
+	@Singleton
+	ScheduledExecutorService provideScheduledExecutorService()
+	{
+		return new ExecutorServiceExceptionLogger(Executors.newSingleThreadScheduledExecutor(new ThreadFactoryBuilder()
+			.setNameFormat("scheduled-%d")
+			.build()));
+	}
+
+	@Provides
+	@Singleton
+	ExecutorService provideExecutorService()
+	{
+		int poolSize = 2 * Runtime.getRuntime().availableProcessors();
+
+		// Will start up to poolSize threads (because of allowCoreThreadTimeOut) as necessary, and times out
+		// unused threads after 1 minute
+		ThreadPoolExecutor executor = new ThreadPoolExecutor(poolSize, poolSize,
+			60L, TimeUnit.SECONDS,
+			new LinkedBlockingQueue<>(),
+			new ThreadFactoryBuilder().setNameFormat("worker-%d").build());
+		executor.allowCoreThreadTimeOut(true);
+
+		return new NonScheduledExecutorServiceExceptionLogger(executor);
+	}
 }

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

@@ -28,7 +28,6 @@ import com.google.common.collect.Lists;
 import com.google.common.graph.GraphBuilder;
 import com.google.common.graph.Graphs;
 import com.google.common.graph.MutableGraph;
-import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import com.google.inject.Binder;
 import com.google.inject.CreationException;
 import com.google.inject.Injector;
@@ -48,7 +47,6 @@ import java.util.Set;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
@@ -103,6 +101,7 @@ public class ExternalPluginManager
 	private final List<UpdateRepository> repositories = new ArrayList<>();
 	private final OpenOSRSConfig openOSRSConfig;
 	private final EventBus eventBus;
+	private final ExecutorService executorService;
 	private final ConfigManager configManager;
 	private final Map<String, String> pluginsMap = new HashMap<>();
 	@Getter(AccessLevel.PUBLIC)
@@ -119,12 +118,14 @@ public class ExternalPluginManager
 		PluginManager pluginManager,
 		OpenOSRSConfig openOSRSConfig,
 		EventBus eventBus,
+		ExecutorService executorService,
 		ConfigManager configManager,
 		Groups groups)
 	{
 		this.runelitePluginManager = pluginManager;
 		this.openOSRSConfig = openOSRSConfig;
 		this.eventBus = eventBus;
+		this.executorService = executorService;
 		this.configManager = configManager;
 		this.groups = groups;
 
@@ -477,62 +478,44 @@ public class ExternalPluginManager
 
 		final long start = System.currentTimeMillis();
 
-		// some plugins get stuck on IO, so add some extra threads
+		List<Plugin> scannedPlugins = new CopyOnWriteArrayList<>();
-		ExecutorService exec = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() * 2,
+		sortedPlugins.forEach(group ->
-			new ThreadFactoryBuilder()
-				.setNameFormat("external-plugin-manager-%d")
-				.build());
-
-		try
 		{
-			List<Plugin> scannedPlugins = new CopyOnWriteArrayList<>();
+			List<Future<?>> curGroup = new ArrayList<>();
-			sortedPlugins.forEach(group ->
+			group.forEach(pluginClazz ->
-			{
+				curGroup.add(executorService.submit(() ->
-				List<Future<?>> curGroup = new ArrayList<>();
-				group.forEach(pluginClazz ->
-					curGroup.add(exec.submit(() ->
-					{
-						Plugin plugininst;
-						try
-						{
-							//noinspection unchecked
-							plugininst = instantiate(scannedPlugins, (Class<Plugin>) pluginClazz, init, initConfig);
-							scannedPlugins.add(plugininst);
-						}
-						catch (PluginInstantiationException e)
-						{
-							log.warn("Error instantiating plugin!", e);
-							return;
-						}
-
-						loaded.getAndIncrement();
-
-						RuneLiteSplashScreen.stage(.67, .75, "Loading external plugins", loaded.get(), scannedPlugins.size());
-					})));
-				curGroup.forEach(future ->
 				{
+					Plugin plugininst;
 					try
 					{
-						future.get();
+						//noinspection unchecked
+						plugininst = instantiate(scannedPlugins, (Class<Plugin>) pluginClazz, init, initConfig);
+						scannedPlugins.add(plugininst);
 					}
-					catch (InterruptedException | ExecutionException e)
+					catch (PluginInstantiationException e)
 					{
-						log.warn("Could not instantiate external plugin", e);
+						log.warn("Error instantiating plugin!", e);
+						return;
 					}
-				});
-			});
 
-			log.info("External plugin instantiation took {}ms", System.currentTimeMillis() - start);
+					loaded.getAndIncrement();
-		}
+
-		finally
+					RuneLiteSplashScreen.stage(.67, .75, "Loading external plugins", loaded.get(), scannedPlugins.size());
-		{
+				})));
-			List<Runnable> unfinishedTasks = exec.shutdownNow();
+			curGroup.forEach(future ->
-			if (!unfinishedTasks.isEmpty())
 			{
-				// This shouldn't happen since we Future#get all tasks submitted to the executor
+				try
-				log.warn("Did not complete all update tasks: {}", unfinishedTasks);
+				{
-			}
+					future.get();
-		}
+				}
+				catch (InterruptedException | ExecutionException e)
+				{
+					log.warn("Could not instantiate external plugin", e);
+				}
+			});
+		});
+
+		log.info("External plugin instantiation took {}ms", System.currentTimeMillis() - start);
 	}
 
 	@SuppressWarnings("unchecked")

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

@@ -34,7 +34,6 @@ import com.google.common.graph.Graphs;
 import com.google.common.graph.MutableGraph;
 import com.google.common.reflect.ClassPath;
 import com.google.common.reflect.ClassPath.ClassInfo;
-import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import com.google.inject.Binder;
 import com.google.inject.CreationException;
 import com.google.inject.Injector;
@@ -54,7 +53,6 @@ import java.util.Set;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Collectors;
@@ -96,6 +94,7 @@ public class PluginManager
 
 	private final EventBus eventBus;
 	private final Scheduler scheduler;
+	private final ExecutorService executorService;
 	private final ConfigManager configManager;
 	private final Provider<GameEventManager> sceneTileManager;
 	private final List<Plugin> plugins = new CopyOnWriteArrayList<>();
@@ -115,6 +114,7 @@ public class PluginManager
 	PluginManager(
 		final EventBus eventBus,
 		final Scheduler scheduler,
+		final ExecutorService executorService,
 		final ConfigManager configManager,
 		final Provider<GameEventManager> sceneTileManager,
 		final Groups groups,
@@ -122,6 +122,7 @@ public class PluginManager
 	{
 		this.eventBus = eventBus;
 		this.scheduler = scheduler;
+		this.executorService = executorService;
 		this.configManager = configManager;
 		this.sceneTileManager = sceneTileManager;
 		this.groups = groups;
@@ -404,20 +405,12 @@ public class PluginManager
 
 		final long start = System.currentTimeMillis();
 
-		// some plugins get stuck on IO, so add some extra threads
-		ExecutorService exec = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() * 2,
-			new ThreadFactoryBuilder()
-				.setNameFormat("plugin-manager-%d")
-				.build());
-
-		try
-		{
 			List<Plugin> scannedPlugins = new CopyOnWriteArrayList<>();
 			sortedPlugins.forEach(group ->
 			{
 				List<Future<?>> curGroup = new ArrayList<>();
 				group.forEach(pluginClazz ->
-					curGroup.add(exec.submit(() ->
+					curGroup.add(executorService.submit(() ->
 					{
 						Plugin plugin;
 						try
@@ -450,16 +443,6 @@ public class PluginManager
 
 			log.info("Plugin instantiation took {}ms", System.currentTimeMillis() - start);
 			return scannedPlugins;
-		}
-		finally
-		{
-			List<Runnable> unfinishedTasks = exec.shutdownNow();
-			if (!unfinishedTasks.isEmpty())
-			{
-				// This shouldn't happen since we Future#get all tasks submitted to the executor
-				log.warn("Did not complete all update tasks: {}", unfinishedTasks);
-			}
-		}
 	}
 
 	public boolean startPlugin(Plugin plugin) throws PluginInstantiationException

runelite-client/src/main/java/net/runelite/client/ui/ClientUI.java

@@ -50,6 +50,8 @@ import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.time.Duration;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
 import javax.inject.Inject;
 import javax.inject.Provider;
@@ -134,6 +136,7 @@ public class ClientUI
 	private final MouseManager mouseManager;
 	private final Applet client;
 	private final ConfigManager configManager;
+	private final ExecutorService executorService;
 	private final Provider<ClientThread> clientThreadProvider;
 	private final EventBus eventBus;
 	private final CardLayout cardLayout = new CardLayout();
@@ -161,6 +164,7 @@ public class ClientUI
 		MouseManager mouseManager,
 		@Nullable Applet client,
 		ConfigManager configManager,
+		ExecutorService executorService,
 		Provider<ClientThread> clientThreadProvider,
 		EventBus eventbus)
 	{
@@ -169,6 +173,7 @@ public class ClientUI
 		this.mouseManager = mouseManager;
 		this.client = client;
 		this.configManager = configManager;
+		this.executorService = executorService;
 		this.clientThreadProvider = clientThreadProvider;
 		this.eventBus = eventbus;
 
@@ -606,9 +611,21 @@ public class ClientUI
 		saveClientBoundsConfig();
 		ClientShutdown csev = new ClientShutdown();
 		eventBus.post(ClientShutdown.class, csev);
+		executorService.shutdown();
+
 		new Thread(() ->
 		{
 			csev.waitForAllConsumers(Duration.ofSeconds(10));
+			try
+			{
+				if (!executorService.awaitTermination(5, TimeUnit.SECONDS))
+				{
+					executorService.shutdownNow();
+				}
+			}
+			catch (InterruptedException ignored)
+			{
+			}
 
 			if (client != null)
 			{

runelite-client/src/main/java/net/runelite/client/util/NonScheduledExecutorServiceExceptionLogger.java

@@ -0,0 +1,109 @@
+package net.runelite.client.util;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import static net.runelite.client.util.RunnableExceptionLogger.wrap;
+import org.jetbrains.annotations.NotNull;
+
+// Awkward name because plugins already referenced the ExecutorServiceExceptionLogger
+// (which only handles ScheduledExecutorServices) before this class was introduced
+public class NonScheduledExecutorServiceExceptionLogger implements ExecutorService
+{
+	private final ExecutorService service;
+
+	public NonScheduledExecutorServiceExceptionLogger(ExecutorService service)
+	{
+		this.service = service;
+	}
+
+	@Override
+	public void shutdown()
+	{
+		service.shutdown();
+	}
+
+	@NotNull
+	@Override
+	public List<Runnable> shutdownNow()
+	{
+		return service.shutdownNow();
+	}
+
+	@Override
+	public boolean isShutdown()
+	{
+		return service.isShutdown();
+	}
+
+	@Override
+	public boolean isTerminated()
+	{
+		return service.isTerminated();
+	}
+
+	@Override
+	public boolean awaitTermination(long timeout, @NotNull TimeUnit unit) throws InterruptedException
+	{
+		return service.awaitTermination(timeout, unit);
+	}
+
+	@Override
+	public void execute(@NotNull Runnable command)
+	{
+		service.execute(wrap(command));
+	}
+
+	@NotNull
+	@Override
+	public <T> Future<T> submit(@NotNull Callable<T> task)
+	{
+		return service.submit(CallableExceptionLogger.wrap(task));
+	}
+
+	@NotNull
+	@Override
+	public <T> Future<T> submit(@NotNull Runnable task, T result)
+	{
+		return service.submit(wrap(task), result);
+	}
+
+	@NotNull
+	@Override
+	public Future<?> submit(@NotNull Runnable task)
+	{
+		return service.submit(wrap(task));
+	}
+
+	@NotNull
+	@Override
+	public <T> List<Future<T>> invokeAll(@NotNull Collection<? extends Callable<T>> tasks) throws InterruptedException
+	{
+		return service.invokeAll(tasks);
+	}
+
+	@NotNull
+	@Override
+	public <T> List<Future<T>> invokeAll(@NotNull Collection<? extends Callable<T>> tasks, long timeout, @NotNull TimeUnit unit) throws InterruptedException
+	{
+		return service.invokeAll(tasks, timeout, unit);
+	}
+
+	@NotNull
+	@Override
+	public <T> T invokeAny(@NotNull Collection<? extends Callable<T>> tasks) throws InterruptedException, ExecutionException
+	{
+		return service.invokeAny(tasks);
+	}
+
+	@Override
+	public <T> T invokeAny(@NotNull Collection<? extends Callable<T>> tasks, long timeout, @NotNull TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException
+	{
+		return service.invokeAny(tasks, timeout, unit);
+	}
+}

runelite-client/src/test/java/net/runelite/client/plugins/PluginManagerTest.java

@@ -47,6 +47,8 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import net.runelite.api.Client;
 import net.runelite.api.events.Event;
 import net.runelite.client.RuneLite;
@@ -56,6 +58,7 @@ import net.runelite.client.config.ConfigItem;
 import net.runelite.client.eventbus.AccessorGenerator;
 import net.runelite.client.eventbus.EventBus;
 import net.runelite.client.eventbus.Subscribe;
+import org.junit.After;
 import static org.junit.Assert.assertEquals;
 import org.junit.Before;
 import org.junit.Rule;
@@ -81,12 +84,16 @@ public class PluginManagerTest
 	@Bind
 	public Client client;
 
+	private ExecutorService executorService;
+
 	private Set<Class> pluginClasses;
 	private Set<Class> configClasses;
 
 	@Before
 	public void before() throws IOException
 	{
+		executorService = Executors.newSingleThreadScheduledExecutor();
+
 		Injector injector = Guice.createInjector(Modules
 			.override(new RuneLiteModule(() -> null, RuneLite.DEFAULT_CONFIG_FILE))
 			.with(BoundFieldModule.of(this)));
@@ -114,10 +121,16 @@ public class PluginManagerTest
 		}
 	}
 
+	@After
+	public void after()
+	{
+		executorService.shutdownNow();
+	}
+
 	@Test
 	public void testLoadPlugins() throws Exception
 	{
-		PluginManager pluginManager = new PluginManager(null, null, null, null, null, null);
+		PluginManager pluginManager = new PluginManager(null, null, executorService, null, null, null, null);
 		pluginManager.setOutdated(true);
 		pluginManager.loadCorePlugins();
 		Collection<Plugin> plugins = pluginManager.getPlugins();
@@ -128,7 +141,7 @@ public class PluginManagerTest
 			.count();
 		assertEquals(expected, plugins.size());
 
-		pluginManager = new PluginManager(null, null, null, null, null, null);
+		pluginManager = new PluginManager(null, null, executorService, null, null, null, null);
 		pluginManager.loadCorePlugins();
 		plugins = pluginManager.getPlugins();
 
@@ -146,7 +159,7 @@ public class PluginManagerTest
 		modules.add(new GraphvizModule());
 		modules.add(new RuneLiteModule(() -> null, RuneLite.DEFAULT_CONFIG_FILE));
 
-		PluginManager pluginManager = new PluginManager(null, null, null, null, null, null);
+		PluginManager pluginManager = new PluginManager(null, null, executorService, null, null, null, null);
 		pluginManager.loadCorePlugins();
 		modules.addAll(pluginManager.getPlugins());
 
@@ -198,7 +211,7 @@ public class PluginManagerTest
 	public void testEventbusAnnotations() throws Exception
 	{
 		EventBus eventbus = new EventBus();
-		PluginManager pluginManager = new PluginManager(eventbus, null, null, null, null, null)
+		PluginManager pluginManager = new PluginManager(eventbus, null, executorService, null, null, null, null)
 		{
 			@Override
 			public boolean isPluginEnabled(Plugin plugin)
@@ -207,7 +220,9 @@ public class PluginManagerTest
 			}
 		};
 
-		class TestEvent implements Event {}
+		class TestEvent implements Event
+		{
+		}
 		class TestPlugin extends Plugin
 		{
 			private boolean thisShouldBeTrue = false;