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.

Sign up for GitHub

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

Jump to bottom

Completely rework save operation #4309

Merged
merged 11 commits into from
Sep 13, 2018
Merged

Completely rework save operation #4309

merged 11 commits into from
Sep 13, 2018

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Sep 1, 2018 •
edited
Loading

This is a complete rework of the file writing logic. Since this is the most essential part of JabRef, I would kindly ask to every @JabRef/developers to have a look at the code (at least on the new AtomicOutputStream which does the hard work).

The new strategy for saving the bib file is as follows:

  • Create a backup (with .bak suffix) of the original file in the same directory as the destination file (if set in preferences).
  • Write to a temporary file (with .tmp suffix) in the same directory.
  • Create a save-backup (with .sav suffix) of the original file (if it exists) in the same directory.
  • Move the temporary file to the correct place, overwriting any file that already exists at that location.
  • Delete the save-backup file.

What is not clear to me is how one should handle (almost) concurrent saving processes of different users to the same file. Is it enough to lock a file (nio.FileLock) and if yes, which (the tmp?)?

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr
Siedlerchr reviewed Sep 1, 2018
View reviewed changes
src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java
try {
Files.deleteIfExists(temporaryFile);
} catch (IOException exception) {
LOGGER.debug("Unable to delete file " + temporaryFile, exception);
Copy link
Member

Choose a reason for hiding this comment

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

For arguments in the logger you can simply use a pair of curly braces {} instead of concat with plus.
https://www.slf4j.org/faq.html#logging_performance
Here and in above

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 is not possible here since the there is no appropriate debug method that also accepts an exception.

Siedlerchr
Siedlerchr reviewed Sep 1, 2018
View reviewed changes
src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java
}

// Move temporary file (replace original if it exists)
Files.move(temporaryFile, targetFile, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
Copy link
Member

Choose a reason for hiding this comment

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

We have a move in FileUtil. And you better check if the move was successufl

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good point - I don't understand why we have a own move method that actually looks more like a "read and rewrite content"-method. Moreover, the move is successful if there is no thrown exception (I assume).

@Siedlerchr
Copy link
Member

Yeah, I generally like that you tackle this complex stuff! 👍 We should really test this carefully and also check how it interacts with the FileUpdateMonitor.
Some TemplateExporter tests fail, seems to be related to the resource closing

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 2, 2018
@tobiasdiez tobiasdiez changed the title [WIP] Completely rework save operation Completely rework save operation Sep 2, 2018
@koppor
Copy link
Member

koppor commented Sep 3, 2018 via email

@tobiasdiez
Copy link
Member Author

Thanks @koppor for the valuable input (especially for pointing out that the bak and sav file are the same). The idea of writing to a temporary file is to make sure that any point in time the bib file is valid and un-damaged, even in cases of powerloss, drive disconnect etc.

@Siedlerchr
Copy link
Member

As I experienced this first hand in another project, a locking of the file is practically useless on Unix systems. Even if you grab a write lock, it doesn't stop other processes from accessing the file or writing to it. That's because file locks on Unix are only advisory, e.g.just a signal to other process.
https://docs.oracle.com/javase/7/docs/api/java/nio/channels/FileLock.html

@tobiasdiez
Copy link
Member Author

I included all the feedback from @koppor and @Siedlerchr. Thanks for the good suggestions!
Since this PR has been in "ready-for-review" for two weeks now, I'll merge now.

@tobiasdiez tobiasdiez merged commit e83680f into master Sep 13, 2018
@tobiasdiez tobiasdiez deleted the reworkSave branch September 13, 2018 19:36
@tobiasdiez tobiasdiez mentioned this pull request Sep 13, 2018
Closed
Siedlerchr added a commit that referenced this pull request Sep 14, 2018
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into ooPanel
42b178f 
* upstream/master:
  Completely rework save operation (#4309)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr left review comments

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tobiasdiez @Siedlerchr @koppor