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

Replace usage of Threads and priorities with thread pool #2304

Merged
merged 17 commits into from
Dec 2, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/main/java/net/sf/jabref/Globals.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

I really liked the fact that a single thread was running in the background and I could find this thread when looking at the thread dump during a debug session. Maybe use a single threaded executor service just for this file update monitor?

Copy link
Member Author

Choose a reason for hiding this comment

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

This shouldn't be a problem. Maybe it makes sense for the specific threads, which are very prominent. Using an executor makes lifecycle management a little nicer at least.

I have to look into the remote thread as well, I guess it makes sense for that one, too.

}

public static GlobalFocusListener getFocusListener() {
Expand Down
54 changes: 21 additions & 33 deletions src/main/java/net/sf/jabref/JabRefExecutorService.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ public class JabRefExecutorService implements Executor {
thread.setName("JabRef CachedThreadPool");
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");
return thread;
});

private final Timer timer = new Timer("timer", true);

Expand Down Expand Up @@ -85,56 +90,39 @@ public void run() {
}
}

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");
startedThreads.add(thread);
thread.setPriority(Thread.MIN_PRIORITY);
thread.start();
public void executeInterruptableTask(final Runnable runnable) {
this.lowPriorityExecutorService.execute(runnable);
}

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");
startedThreads.add(thread);
thread.setPriority(Thread.MIN_PRIORITY);
thread.start();

waitForThreadToFinish(thread);
}
public void executeInterruptableTaskAndWait(Runnable runnable) {
if(runnable == null) {
//TODO logger
return;
}

private void waitForThreadToFinish(Thread thread) {
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 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();
// 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
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/sf/jabref/gui/BasePanel.java
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/net/sf/jabref/gui/DuplicateSearch.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public void run() {
}

SearcherRunnable st = new SearcherRunnable();
JabRefExecutorService.INSTANCE.executeWithLowPriorityInOwnThread(st, "Searcher");
JabRefExecutorService.INSTANCE.executeInterruptableTask(st);
int current = 0;

final List<BibEntry> toRemove = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,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.executeInterruptableTask(remoteListenerServerThread);
}
}

Expand Down