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

@@ -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
 
 
 

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() {

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,36 +22,44 @@ 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<Thread> 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);
 
     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;
         }
 
         executorService.execute(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<Thread> 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<Thread> 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
     }
 

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:

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<BibEntry> toRemove = new ArrayList<>();

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) {

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);
         }
     }