Replace usage of Threads and priorities with thread pool#2304
Conversation
Siedlerchr
left a comment
There was a problem hiding this comment.
It looks good to me, however I have no practical experience with that ExecutorService. So I can't say anyhting about that
| Globals.fileUpdateMonitor = new FileUpdateMonitor(); | ||
| JabRefExecutorService.INSTANCE.executeWithLowPriorityInOwnThread(Globals.fileUpdateMonitor, | ||
| "FileUpdateMonitor"); | ||
| JabRefExecutorService.INSTANCE.executeInterruptableTask(Globals.fileUpdateMonitor); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
# Conflicts: # src/main/java/net/sf/jabref/JabRefExecutorService.java
…needed, as the Runnable is used instead of the Thread
|
So, what started out as a simple improvement ran into some issues. This is all due to the, in my point of view, quite unfortunate implementation of the remote functionality. This is heavily tied to being managed in a thread specifically and the lifecycle heavily depends on that. I was not successful in refactoring it towards a runnable within a reasonable amount of time and reverted that part of the PR. The rest is no problem: Also the former low-priority threads are now managed in a thread pool as opposed to a custom thread list and in case someone ever wants to do a manual prioritization it would be a one-liner to add it back. Moreover, threads can be named and are named as before. The remote thread is kept as is and also managed by I have installed the compiled version from builds.jabref, played around a little, and everything seems fine. I would appreciate it if someone else would also do some testing (no visible error should happen and JabRef should terminate in an orderly fashion) and then this is good to go from my side. |
| private String name; | ||
|
|
||
| public Thread thread; | ||
| private Runnable task; |
There was a problem hiding this comment.
final for both fields
|
I do remember the pain with RemoteThread. Hm, would something like this work? https://techblog.bozho.net/interrupting-executor-tasks/ |
|
I would just leave the RemoteThread as it is in this PR now. The problem is that the whole remote functionality is dependent on this being a RemoteThread and it also depends on various lifecycle methods of the Thread class itself (calls for Thread.State, etc.). This just won't work as soon as you use an ExecutorService and changing it would mean rewriting the remote functionality. Purely for a refactoring, this is not worth it imho. |
|
We checked this in the devcall and I will merge it now. The build breaks at the moment because of the BibLatexEntryTypes, but that is also the case for master, so it does not prevent merging. |
* upstream/master: Ignore failing test Replace usage of Threads and priorities with thread pool (#2304) Class variable declarations and method declarations are now separated by one line Disable joining of wrapped lines Installer Code Signing #1879 (#2320) Add bibtex key deviation check (#2328) Update mockito-core (2.2.21 -> 2.2.26) and wiremock (2.3.1 -> 2.4.1) Fix opening of preference dialog with Java 9 (#2329) Add longer explanation for ID-based entry generation. (#2330) Add DOI integrity check (#2327) New strings translated (#2325) Fix exporting via commandline in no gui mode (#2316) Cleanup EMACS code (#2317) Update mockito-core from 2.2.15 to 2.2.21 Fix typo in comment Updated JabRef_tr.properties (#2315) # Conflicts: # CHANGELOG.md
The current priority-based thread handling in JabRef is bad for two reasons:
ExecutorServicesfor this.This PR replaces the manual thread-handling with a second thread pool for all threads that were low-priority before. The main (only) difference to other JabRef threads is that low-priority threads sometimes run forever and need to be interrupted when JabRef should shut down. The new structure keeps this.