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

Add throttle to AutosaveUIManager #5680

Merged
merged 2 commits into from
Dec 7, 2019

Conversation

Ka0o0
Copy link
Contributor

@Ka0o0 Ka0o0 commented Nov 27, 2019

As discussed in #5679 there are a lot of save actions performed when autosave is turned on. This PR adds a timer which limits the save actions to one in 200ms which is especially handy when automated tasks like find and replace are performed.

  • Change in CHANGELOG.md described (if applicable)
  • Manually tested changed features in running JabRef (always required)

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

I checked the JavaDoc for Timer:

Terminates this timer, discarding any currently scheduled tasks. Does not interfere with a currently executing task (if it exists). Once a timer has been terminated, its execution thread terminates gracefully, and no more tasks may be scheduled on it.

I think, this is not the thing one wants to achieve here: The current save should be canceled a new one should be triggered.

I remember a discussion we had with BackgroundWorker, but I currently don't find it.

Refs #2838 (comment)

We wanted to implement a real tasking framework in JabRef. Maybe, JavaFX offers such a thing... This would really solve issues with the PDF indexer and also with our tex feature (http://blog.jabref.org/2019/08/06/GSoC-LatexCitationsTab/)

@koppor
Copy link
Member

koppor commented Nov 29, 2019

When working on this, the comments on #5071 (comment) should be checked 😇

@koppor koppor mentioned this pull request Dec 1, 2019
5 tasks
@Ka0o0 Ka0o0 changed the title Add throttle to AutosaveUIManager [WIP] Add throttle to AutosaveUIManager Dec 4, 2019
@Siedlerchr
Copy link
Member

I found the ScheduledService from javafx, mabye this is an option as well?
https://openjfx.io/javadoc/12/javafx.graphics/javafx/concurrent/ScheduledService.html

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Dec 4, 2019

@koppor by introducing a deferred execution of the actual save operation, a race condition can happen when the old save operation is still in progress.
This leads me to another question: let's say an auto-save action is happening and during the save process the user presses ctrl-s what happens?

As I can see there are two SaveDatabaseAction created: One in the BasePanel and one in the AutosaveManager. No locking is happening. So basically the later manual saved version could be overwritten by the older autosave one?

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Dec 4, 2019

@Siedlerchr I think this is more for recurring tasks rather to simply defer a task, as it says that it will go from scheduled -> running -> finished -> scheduled automatically.

@Ka0o0 Ka0o0 force-pushed the improve-5679-throttel-for-autosave branch from a950aee to 6442ac9 Compare December 4, 2019 18:05
@Ka0o0 Ka0o0 force-pushed the improve-5679-throttel-for-autosave branch from 6442ac9 to 5013f8b Compare December 4, 2019 18:07
@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Dec 4, 2019

Actually, @Siedlerchr you were right! There is a ScheduledThreadPoolExecutor.

I now completely changed the way to tackle the problem. AutosaveManager had a normal ExecutorService. There was actually a bug. When saving took longer, ExecutorService would through an exception as the pool size was only of the size one. Under certain circumstances newer information might not get auto-saved therefore.

The current implementation replaces the ExecutorService by an ScheduledExecutorService.

Additionally, the bug of #4877 can now be reproduced. When autosave is enabled, change something and quickly press ctrl-s -> you will see the error. So we should make the SaveDatabaseAction thread safe to prevent the error.

@koppor what do you think?

@Ka0o0 Ka0o0 changed the title [WIP] Add throttle to AutosaveUIManager Add throttle to AutosaveUIManager Dec 4, 2019
@Ka0o0 Ka0o0 requested a review from koppor December 4, 2019 18:15
@Siedlerchr
Copy link
Member

Ah cool, that sounds good. Thread safe sounds fitting.
There are different ways to achieve this and maybe one of those newer locking things is helping
https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/util/concurrent/package-summary.html

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I like this approach! Good job. I've only a few minor nitpicking comments.
In general, I'm not a big fan that the AutoSaveManager controls their own executor services and would prefer if this could be moved to https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/JabRefExecutorService.java. However, I couldn't come up with a good reusable solution (as you want to have exactly one service per database...)

@koppor
Copy link
Member

koppor commented Dec 4, 2019

@Ka0o0 Do you use Eclipse or IntelliJ? With IntelliJ (and the complete setup from https://github.com/JabRef/jabref/wiki/Guidelines-for-setting-up-a-local-workspace), the autoformat takes care.

by introducing a deferred execution of the actual save operation, a race condition can happen when the old save operation is still in progress. This leads me to another question: let's say an auto-save action is happening and during the save process the user presses ctrl-s what happens?

(Unsure whether this is still the question...) - Either cancel the current autosave or let it go and then trigger it again. I would prefer the former (as long as we write to .sav and then move to .bib - thus we do not destroy the .bib file of the user)

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Dec 5, 2019

@koppor actually I have important that already but I forgot to do auto formatting before committing. I've now created a pre-commit hook which runs checkstyle so that this doesn't happens again.

@Ka0o0 Ka0o0 requested a review from tobiasdiez December 5, 2019 17:31
@tobiasdiez
Copy link
Member

Thanks again!

@tobiasdiez tobiasdiez merged commit d0f6c2b into JabRef:master Dec 7, 2019
Siedlerchr added a commit that referenced this pull request Dec 8, 2019
* upstream/master:
  Add throttle to AutosaveUIManager (#5680)
  Do not couple search position to sidebar width (#5716)
  fix springer fetcher tests
  Bump controlsfx from 11.0.0 to 11.0.1 (#5714)
  Add CHANGELOG.md entry for Oracle
  Enable oracle tests (#5683)
Siedlerchr added a commit that referenced this pull request Dec 8, 2019
* upstream/master:
  Fix upload to GitHub artifacts (#5712)
  Try to fix csl update (#5718)
  Fix import into currently open library (#5717)
  Add throttle to AutosaveUIManager (#5680)
  Do not couple search position to sidebar width (#5716)
  fix springer fetcher tests
  Bump controlsfx from 11.0.0 to 11.0.1 (#5714)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants