Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interrupt all running tasks during shutdown #6118

Merged
merged 10 commits into from
Sep 1, 2020
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down
42 changes: 38 additions & 4 deletions src/main/java/org/jabref/JabRefExecutorService.java
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,47 @@ public void submit(TimerTask timerTask, long millisecondsDelay) {
timer.schedule(timerTask, millisecondsDelay);
}

/**
* Shuts everything down. Upon termination, this method returns.
koppor marked this conversation as resolved.
Show resolved Hide resolved
*/
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract try block to method and call it for executorService and lowPriorityExecutorService

// 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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats not the delayed task throttler

}
}
} catch (InterruptedException ie) {
executorService.shutdownNow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

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

Expand Down
3 changes: 3 additions & 0 deletions src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ public <V> Future<?> schedule(BackgroundTask<V> task, long delay, TimeUnit unit)
return scheduledExecutor.schedule(getJavaFXTask(task), delay, unit);
}

/**
* Shuts everything down. Upon termination, this method returns.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add to interface

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't do - Added a comment to TaskExecutor

*/
@Override
public void shutdown() {
executor.shutdownNow();
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/org/jabref/logic/util/DelayTaskThrottler.java
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -45,7 +46,25 @@ 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call above helper method

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