From 70f530f31c1cf2da3e611f01a0ea7c5b82b6099e Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Sat, 14 Mar 2020 01:57:20 +0100 Subject: [PATCH 1/5] Interrupt all running tasks during shutdown (and don't allow new tasks to be executed) Co-authored-by: Stefan Kolb --- CHANGELOG.md | 1 + .../org/jabref/logic/util/DelayTaskThrottler.java | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8917ce7b9f5..9d4d5256f2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve ### Fixed +- We fixed an issue where the `.sav` file was not deleted upon exiting JabRef. [#6109](https://github.com/JabRef/jabref/issues/6109) - We fixed an issue where opening a library from the recent libraries menu was not possible. [#5939](https://github.com/JabRef/jabref/issues/5939) ### Removed diff --git a/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java b/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java index 3b30027f5c3..87cdaae0c74 100644 --- a/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java +++ b/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java @@ -32,6 +32,7 @@ public DelayTaskThrottler(int delay) { this.delay = delay; this.executor = new ScheduledThreadPoolExecutor(1); this.executor.setRemoveOnCancelPolicy(true); + this.executor.setExecuteExistingDelayedTasksAfterShutdownPolicy(false); } public void schedule(Runnable command) { @@ -47,5 +48,19 @@ public void schedule(Runnable command) { public void shutdown() { executor.shutdown(); + try { + if (!executor.awaitTermination(60, TimeUnit.SECONDS)) { + LOGGER.debug("One minute passed, saving still not completed. Trying forced shutdown."); + executor.shutdownNow(); + if (executor.awaitTermination(60, TimeUnit.SECONDS)) { + LOGGER.debug("One minute passed again - forced shutdown worked."); + } else { + LOGGER.error("DelayedTaskThrottler did not terminate"); + } + } + } catch (InterruptedException ie) { + executor.shutdownNow(); + Thread.currentThread().interrupt(); + } } } From f836dbe6bf4db3cd8334fc7cdd98dcd5bd9ad403 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 1 Sep 2020 01:03:57 +0200 Subject: [PATCH 2/5] Fix position of CHANGELOG.md entry --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58bb12e0d01..334720b301f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve ### Fixed +- We fixed an issue where the `.sav` file was not deleted upon exiting JabRef. [#6109](https://github.com/JabRef/jabref/issues/6109) + ### Removed @@ -74,7 +76,6 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve ### Fixed -- We fixed an issue where the `.sav` file was not deleted upon exiting JabRef. [#6109](https://github.com/JabRef/jabref/issues/6109) - We fixed an issue where entry preview tab has no name in drop down list. [#6591](https://github.com/JabRef/jabref/issues/6591) - We fixed to only search file links in the BIB file location directory when preferences has corresponding checkbox checked. [#5891](https://github.com/JabRef/jabref/issues/5891) - We fixed wrong button order (Apply and Cancel) in ManageProtectedTermsDialog. From c3e9a2232dee0e3bfd7fc9639f646520c168e04a Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 1 Sep 2020 01:44:56 +0200 Subject: [PATCH 3/5] Extend to JabRefExecutorService --- .../org/jabref/JabRefExecutorService.java | 42 +++++++++++++++++-- .../jabref/gui/util/DefaultTaskExecutor.java | 3 ++ .../jabref/logic/util/DelayTaskThrottler.java | 4 ++ 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jabref/JabRefExecutorService.java b/src/main/java/org/jabref/JabRefExecutorService.java index a90668d9bd2..eb2e011dcd6 100644 --- a/src/main/java/org/jabref/JabRefExecutorService.java +++ b/src/main/java/org/jabref/JabRefExecutorService.java @@ -132,13 +132,47 @@ public void submit(TimerTask timerTask, long millisecondsDelay) { timer.schedule(timerTask, millisecondsDelay); } + /** + * Shuts everything down. Upon termination, this method returns. + */ public void shutdownEverything() { - // those threads will be allowed to finish - this.executorService.shutdown(); - // those threads will be interrupted in their current task - this.lowPriorityExecutorService.shutdownNow(); // kill the remote thread stopRemoteThread(); + + try { + // those threads will be allowed to finish + this.executorService.shutdown(); + if (!executorService.awaitTermination(60, TimeUnit.SECONDS)) { + LOGGER.debug("One minute passed, saving still not completed. Trying forced shutdown."); + executorService.shutdownNow(); + if (executorService.awaitTermination(60, TimeUnit.SECONDS)) { + LOGGER.debug("One minute passed again - forced shutdown worked."); + } else { + LOGGER.error("DelayedTaskThrottler did not terminate"); + } + } + } catch (InterruptedException ie) { + executorService.shutdownNow(); + Thread.currentThread().interrupt(); + } + + try { + // those threads will be interrupted in their current task + this.lowPriorityExecutorService.shutdownNow(); + if (!lowPriorityExecutorService.awaitTermination(60, TimeUnit.SECONDS)) { + LOGGER.debug("One minute passed, saving still not completed. Trying forced shutdown."); + lowPriorityExecutorService.shutdownNow(); + if (lowPriorityExecutorService.awaitTermination(60, TimeUnit.SECONDS)) { + LOGGER.debug("One minute passed again - forced shutdown worked."); + } else { + LOGGER.error("DelayedTaskThrottler did not terminate"); + } + } + } catch (InterruptedException ie) { + lowPriorityExecutorService.shutdownNow(); + Thread.currentThread().interrupt(); + } + timer.cancel(); } diff --git a/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java b/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java index 6cbb6b27515..a461bef98ca 100644 --- a/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java +++ b/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java @@ -122,6 +122,9 @@ public Future schedule(BackgroundTask task, long delay, TimeUnit unit) return scheduledExecutor.schedule(getJavaFXTask(task), delay, unit); } + /** + * Shuts everything down. Upon termination, this method returns. + */ @Override public void shutdown() { executor.shutdownNow(); diff --git a/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java b/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java index 87cdaae0c74..47e22434581 100644 --- a/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java +++ b/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java @@ -46,7 +46,11 @@ public void schedule(Runnable command) { } } + /** + * Shuts everything down. Upon termination, this method returns. + */ public void shutdown() { + // this is non-blocking. See https://stackoverflow.com/a/57383461/873282. executor.shutdown(); try { if (!executor.awaitTermination(60, TimeUnit.SECONDS)) { From 8a15394264a84cba50d6630588b7da9b7d256c51 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 1 Sep 2020 12:14:28 +0200 Subject: [PATCH 4/5] No more code clones --- .../org/jabref/JabRefExecutorService.java | 66 +++++++++---------- .../jabref/logic/util/DelayTaskThrottler.java | 19 +----- 2 files changed, 35 insertions(+), 50 deletions(-) diff --git a/src/main/java/org/jabref/JabRefExecutorService.java b/src/main/java/org/jabref/JabRefExecutorService.java index eb2e011dcd6..006ce8dd58d 100644 --- a/src/main/java/org/jabref/JabRefExecutorService.java +++ b/src/main/java/org/jabref/JabRefExecutorService.java @@ -22,24 +22,29 @@ public class JabRefExecutorService { public static final JabRefExecutorService INSTANCE = new JabRefExecutorService(); + private static final Logger LOGGER = LoggerFactory.getLogger(JabRefExecutorService.class); + private final ExecutorService executorService = Executors.newCachedThreadPool(r -> { Thread thread = new Thread(r); thread.setName("JabRef CachedThreadPool"); thread.setUncaughtExceptionHandler(new FallbackExceptionHandler()); return thread; }); + 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 Thread remoteThread; private JabRefExecutorService() { - } + } public void execute(Runnable command) { Objects.requireNonNull(command); @@ -139,39 +144,8 @@ public void shutdownEverything() { // kill the remote thread stopRemoteThread(); - try { - // those threads will be allowed to finish - this.executorService.shutdown(); - if (!executorService.awaitTermination(60, TimeUnit.SECONDS)) { - LOGGER.debug("One minute passed, saving still not completed. Trying forced shutdown."); - executorService.shutdownNow(); - if (executorService.awaitTermination(60, TimeUnit.SECONDS)) { - LOGGER.debug("One minute passed again - forced shutdown worked."); - } else { - LOGGER.error("DelayedTaskThrottler did not terminate"); - } - } - } catch (InterruptedException ie) { - executorService.shutdownNow(); - Thread.currentThread().interrupt(); - } - - try { - // those threads will be interrupted in their current task - this.lowPriorityExecutorService.shutdownNow(); - if (!lowPriorityExecutorService.awaitTermination(60, TimeUnit.SECONDS)) { - LOGGER.debug("One minute passed, saving still not completed. Trying forced shutdown."); - lowPriorityExecutorService.shutdownNow(); - if (lowPriorityExecutorService.awaitTermination(60, TimeUnit.SECONDS)) { - LOGGER.debug("One minute passed again - forced shutdown worked."); - } else { - LOGGER.error("DelayedTaskThrottler did not terminate"); - } - } - } catch (InterruptedException ie) { - lowPriorityExecutorService.shutdownNow(); - Thread.currentThread().interrupt(); - } + gracefullyShutdown(this.executorService); + gracefullyShutdown(this.lowPriorityExecutorService); timer.cancel(); } @@ -198,4 +172,28 @@ public void run() { } } } + + /** + * Shuts down the provided executor service by first trying a normal shutdown, then waiting for the shutdown and then forcibly shutting it down. + * Returns if the status of the shut down is known. + */ + public static void gracefullyShutdown(ExecutorService executorService) { + try { + // This is non-blocking. See https://stackoverflow.com/a/57383461/873282. + executorService.shutdown(); + if (!executorService.awaitTermination(60, TimeUnit.SECONDS)) { + LOGGER.debug("One minute passed, {} still not completed. Trying forced shutdown.", executorService.toString()); + // those threads will be interrupted in their current task + executorService.shutdownNow(); + if (executorService.awaitTermination(60, TimeUnit.SECONDS)) { + LOGGER.debug("One minute passed again - forced shutdown of {} worked.", executorService.toString()); + } else { + LOGGER.error("{} did not terminate", executorService.toString()); + } + } + } catch (InterruptedException ie) { + executorService.shutdownNow(); + Thread.currentThread().interrupt(); + } + } } diff --git a/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java b/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java index 47e22434581..fcb0c74551f 100644 --- a/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java +++ b/src/main/java/org/jabref/logic/util/DelayTaskThrottler.java @@ -5,6 +5,8 @@ import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; +import org.jabref.JabRefExecutorService; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -50,21 +52,6 @@ public void schedule(Runnable command) { * Shuts everything down. Upon termination, this method returns. */ public void shutdown() { - // this is non-blocking. See https://stackoverflow.com/a/57383461/873282. - executor.shutdown(); - try { - if (!executor.awaitTermination(60, TimeUnit.SECONDS)) { - LOGGER.debug("One minute passed, saving still not completed. Trying forced shutdown."); - executor.shutdownNow(); - if (executor.awaitTermination(60, TimeUnit.SECONDS)) { - LOGGER.debug("One minute passed again - forced shutdown worked."); - } else { - LOGGER.error("DelayedTaskThrottler did not terminate"); - } - } - } catch (InterruptedException ie) { - executor.shutdownNow(); - Thread.currentThread().interrupt(); - } + JabRefExecutorService.gracefullyShutdown(executor); } } From eb0b55be7a11b9cad5c6e21812e204341069cf5e Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 1 Sep 2020 12:18:29 +0200 Subject: [PATCH 5/5] Adress comments --- src/main/java/org/jabref/JabRefExecutorService.java | 2 +- src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java | 2 +- src/main/java/org/jabref/gui/util/TaskExecutor.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jabref/JabRefExecutorService.java b/src/main/java/org/jabref/JabRefExecutorService.java index 006ce8dd58d..0820bf196bc 100644 --- a/src/main/java/org/jabref/JabRefExecutorService.java +++ b/src/main/java/org/jabref/JabRefExecutorService.java @@ -138,7 +138,7 @@ public void submit(TimerTask timerTask, long millisecondsDelay) { } /** - * Shuts everything down. Upon termination, this method returns. + * Shuts everything down. After termination, this method returns. */ public void shutdownEverything() { // kill the remote thread diff --git a/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java b/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java index a461bef98ca..a6073dcad96 100644 --- a/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java +++ b/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java @@ -123,7 +123,7 @@ public Future schedule(BackgroundTask task, long delay, TimeUnit unit) } /** - * Shuts everything down. Upon termination, this method returns. + * Shuts everything down. After termination, this method returns. */ @Override public void shutdown() { diff --git a/src/main/java/org/jabref/gui/util/TaskExecutor.java b/src/main/java/org/jabref/gui/util/TaskExecutor.java index f56d516eed6..e08d858a0cc 100644 --- a/src/main/java/org/jabref/gui/util/TaskExecutor.java +++ b/src/main/java/org/jabref/gui/util/TaskExecutor.java @@ -45,7 +45,7 @@ public interface TaskExecutor { Future schedule(BackgroundTask task, long delay, TimeUnit unit); /** - * Shutdown the task executor. + * Shutdown the task executor. May happen in the background or may be finished when this method returns. */ void shutdown();