From a23245f3dd8484d5c5aaad2a3b1ff28ccfbb5377 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Lenhard?= Date: Fri, 2 Dec 2016 13:35:10 +0100 Subject: [PATCH] Replace usage of Threads and priorities with thread pool (#2304) * Replace usage of Threads and priorities with thread pool * NotStartedBefore check in RemoteListenerServerLifecycle is no longer needed, as the Runnable is used instead of the Thread * Replace RemoteListenerServerThread with Runnable * Add support for named Runnables * Remove unused import * Remove logging that is no longer needed * Throw exception to make sense of travis test results * Throw exception in case the port is already bound * Undo changes to the remote listener * Do not forget to stop remote thread * Readd newlines at file ends * And add some more newlines at the end of files * Turn members of NamedRunnable final * Minor formatting and logging improvements * Add changelog entry --- CHANGELOG.md | 2 +- src/main/java/net/sf/jabref/Globals.java | 3 +- .../net/sf/jabref/JabRefExecutorService.java | 116 ++++++++++-------- .../java/net/sf/jabref/gui/BasePanel.java | 2 +- .../net/sf/jabref/gui/DuplicateSearch.java | 2 +- .../gui/exporter/SaveDatabaseAction.java | 2 +- .../server/RemoteListenerServerLifecycle.java | 3 +- 7 files changed, 70 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d47c7c2c7b..bd8613a55fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# - The default emacs executable name on linux changed from `gnuclient` to `emacsclient`. [feature-request 433](https://sourceforge.net/p/jabref/feature-requests/433/) - Adds integrity check to detect all bibtex keys which deviate from their generation pattern [#2206](https://github.com/JabRef/jabref/issues/2206) - Adds an integrity check that detects invalid DOIs [#1445](https://github.com/JabRef/jabref/issues/1445) - +- Replaces manual thread management with cached thread pool diff --git a/src/main/java/net/sf/jabref/Globals.java b/src/main/java/net/sf/jabref/Globals.java index ef55ee21ef8..682b980e5c5 100644 --- a/src/main/java/net/sf/jabref/Globals.java +++ b/src/main/java/net/sf/jabref/Globals.java @@ -61,8 +61,7 @@ public static void startBackgroundTasks() { Globals.streamEavesdropper = StreamEavesdropper.eavesdropOnSystem(); Globals.fileUpdateMonitor = new FileUpdateMonitor(); - JabRefExecutorService.INSTANCE.executeWithLowPriorityInOwnThread(Globals.fileUpdateMonitor, - "FileUpdateMonitor"); + JabRefExecutorService.INSTANCE.executeInterruptableTask(Globals.fileUpdateMonitor, "FileUpdateMonitor"); } public static GlobalFocusListener getFocusListener() { diff --git a/src/main/java/net/sf/jabref/JabRefExecutorService.java b/src/main/java/net/sf/jabref/JabRefExecutorService.java index 6debf7875bd..017c96499c7 100644 --- a/src/main/java/net/sf/jabref/JabRefExecutorService.java +++ b/src/main/java/net/sf/jabref/JabRefExecutorService.java @@ -2,7 +2,6 @@ import java.util.Timer; import java.util.TimerTask; -import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; @@ -23,13 +22,21 @@ public class JabRefExecutorService implements Executor { public static final JabRefExecutorService INSTANCE = new JabRefExecutorService(); + private Thread remoteThread; + private final ExecutorService executorService = Executors.newCachedThreadPool(r -> { Thread thread = new Thread(r); thread.setName("JabRef CachedThreadPool"); thread.setUncaughtExceptionHandler(new FallbackExceptionHandler()); return thread; }); - private final ConcurrentLinkedQueue startedThreads = new ConcurrentLinkedQueue<>(); + + private final ExecutorService lowPriorityExecutorService = Executors.newCachedThreadPool(r -> { + Thread thread = new Thread(r); + thread.setName("JabRef LowPriorityCachedThreadPool"); + thread.setUncaughtExceptionHandler(new FallbackExceptionHandler()); + return thread; + }); private final Timer timer = new Timer("timer", true); @@ -37,8 +44,8 @@ private JabRefExecutorService() {} @Override public void execute(Runnable command) { - if(command == null) { - //TODO logger + if (command == null) { + LOGGER.debug("Received null as command for execution"); return; } @@ -46,13 +53,13 @@ public void execute(Runnable command) { } public void executeAndWait(Runnable command) { - if(command == null) { - //TODO logger + if (command == null) { + LOGGER.debug("Received null as command for execution"); return; } Future future = executorService.submit(command); - while(true) { + while (true) { try { future.get(); return; @@ -64,80 +71,83 @@ public void executeAndWait(Runnable command) { } } - private static class AutoCleanupRunnable implements Runnable { + public void executeInterruptableTask(final Runnable runnable) { + this.lowPriorityExecutorService.execute(runnable); + } - private final Runnable runnable; - private final ConcurrentLinkedQueue startedThreads; + public void executeInterruptableTask(final Runnable runnable, String taskName) { + this.lowPriorityExecutorService.execute(new NamedRunnable(taskName, runnable)); + } - public Thread thread; + class NamedRunnable implements Runnable { - private AutoCleanupRunnable(Runnable runnable, ConcurrentLinkedQueue startedThreads) { - this.runnable = runnable; - this.startedThreads = startedThreads; + private final String name; + + private final Runnable task; + + public NamedRunnable(String name, Runnable runnable){ + this.name = name; + this.task = runnable; } @Override public void run() { + final String orgName = Thread.currentThread().getName(); + Thread.currentThread().setName(name); try { - runnable.run(); + task.run(); } finally { - startedThreads.remove(thread); + Thread.currentThread().setName(orgName); } } } - public void executeWithLowPriorityInOwnThread(final Runnable runnable, String name) { - AutoCleanupRunnable target = new AutoCleanupRunnable(runnable, startedThreads); - final Thread thread = new Thread(target); - target.thread = thread; - thread.setName("JabRef - " + name + " - low prio"); - thread.setUncaughtExceptionHandler(new FallbackExceptionHandler()); - startedThreads.add(thread); - thread.setPriority(Thread.MIN_PRIORITY); - thread.start(); - } - - public void executeInOwnThread(Thread thread) { - // this is a special case method for Threads that cannot be interrupted so easily - // this method should normally not be used - startedThreads.add(thread); - // TODO memory leak when thread is finished - thread.start(); - } - - public void executeWithLowPriorityInOwnThreadAndWait(Runnable runnable) { - Thread thread = new Thread(runnable); - thread.setName("JabRef low prio"); - thread.setUncaughtExceptionHandler(new FallbackExceptionHandler()); - startedThreads.add(thread); - thread.setPriority(Thread.MIN_PRIORITY); - thread.start(); - - waitForThreadToFinish(thread); - } + public void executeInterruptableTaskAndWait(Runnable runnable) { + if(runnable == null) { + LOGGER.debug("Received null as command for execution"); + return; + } - private void waitForThreadToFinish(Thread thread) { - while(true) { + Future future = lowPriorityExecutorService.submit(runnable); + while (true) { try { - thread.join(); - startedThreads.remove(thread); + future.get(); return; } catch (InterruptedException ignored) { // Ignored + } catch (ExecutionException e) { + LOGGER.error("Problem executing command", e); } } } + public void manageRemoteThread(Thread thread) { + if (this.remoteThread != null){ + throw new IllegalStateException("Remote thread is already attached"); + } else { + this.remoteThread = thread; + remoteThread.start(); + } + } + + public void stopRemoteThread() { + if (remoteThread != null) { + remoteThread.interrupt(); + remoteThread = null; + } + } + public void submit(TimerTask timerTask, long millisecondsDelay) { timer.schedule(timerTask, millisecondsDelay); } public void shutdownEverything() { + // those threads will be allowed to finish this.executorService.shutdown(); - for(Thread thread : startedThreads) { - thread.interrupt(); - } - startedThreads.clear(); + //those threads will be interrupted in their current task + this.lowPriorityExecutorService.shutdownNow(); + // kill the remote thread + stopRemoteThread(); // timer doesn't need to be canceled as it is run in daemon mode, which ensures that it is stopped if the application is shut down } diff --git a/src/main/java/net/sf/jabref/gui/BasePanel.java b/src/main/java/net/sf/jabref/gui/BasePanel.java index 0523c1939c6..ad26c5b21e0 100644 --- a/src/main/java/net/sf/jabref/gui/BasePanel.java +++ b/src/main/java/net/sf/jabref/gui/BasePanel.java @@ -2094,7 +2094,7 @@ public void fileUpdated() { return; } - JabRefExecutorService.INSTANCE.executeWithLowPriorityInOwnThreadAndWait(scanner); + JabRefExecutorService.INSTANCE.executeInterruptableTaskAndWait(scanner); // Adding the sidepane component is Swing work, so we must do this in the Swing // thread: diff --git a/src/main/java/net/sf/jabref/gui/DuplicateSearch.java b/src/main/java/net/sf/jabref/gui/DuplicateSearch.java index dfef3e59a74..fbfc1cba274 100644 --- a/src/main/java/net/sf/jabref/gui/DuplicateSearch.java +++ b/src/main/java/net/sf/jabref/gui/DuplicateSearch.java @@ -42,7 +42,7 @@ public void run() { } SearcherRunnable st = new SearcherRunnable(); - JabRefExecutorService.INSTANCE.executeWithLowPriorityInOwnThread(st, "Searcher"); + JabRefExecutorService.INSTANCE.executeInterruptableTask(st, "DuplicateSearcher"); int current = 0; final List toRemove = new ArrayList<>(); diff --git a/src/main/java/net/sf/jabref/gui/exporter/SaveDatabaseAction.java b/src/main/java/net/sf/jabref/gui/exporter/SaveDatabaseAction.java index 3bb7e6821b3..68376cdb0ec 100644 --- a/src/main/java/net/sf/jabref/gui/exporter/SaveDatabaseAction.java +++ b/src/main/java/net/sf/jabref/gui/exporter/SaveDatabaseAction.java @@ -427,7 +427,7 @@ private boolean checkExternalModification() { ChangeScanner scanner = new ChangeScanner(panel.frame(), panel, panel.getBibDatabaseContext().getDatabaseFile().get()); - JabRefExecutorService.INSTANCE.executeWithLowPriorityInOwnThreadAndWait(scanner); + JabRefExecutorService.INSTANCE.executeInterruptableTaskAndWait(scanner); if (scanner.changesFound()) { scanner.displayResult(resolved -> { if (resolved) { diff --git a/src/main/java/net/sf/jabref/logic/remote/server/RemoteListenerServerLifecycle.java b/src/main/java/net/sf/jabref/logic/remote/server/RemoteListenerServerLifecycle.java index dac8f8437e5..630d8e5cb37 100644 --- a/src/main/java/net/sf/jabref/logic/remote/server/RemoteListenerServerLifecycle.java +++ b/src/main/java/net/sf/jabref/logic/remote/server/RemoteListenerServerLifecycle.java @@ -26,6 +26,7 @@ public void stop() { if (isOpen()) { remoteListenerServerThread.interrupt(); remoteListenerServerThread = null; + JabRefExecutorService.INSTANCE.stopRemoteThread(); } } @@ -56,7 +57,7 @@ public boolean isOpen() { public void start() { if (isOpen() && isNotStartedBefore()) { // threads can only be started when in state NEW - JabRefExecutorService.INSTANCE.executeInOwnThread(remoteListenerServerThread); + JabRefExecutorService.INSTANCE.manageRemoteThread(remoteListenerServerThread); } }