More conservative write #8750

koppor · 2022-05-02T17:47:56Z

Refs #8746

This introduces a feature of our AtomicFileWriter to not "commit" changes if an error happened during write.

I currently don't know how to test. Maybe simulating a small virtual hard disk not having enough space for writing?

should fix:

open things:

When opening a library and a .bak file is found -> show the diff jabref-koppor#391

does not fix:

When saving: The libary has been modified by another program #4877
When saving: The library has been modified by another program - triggered by adding and removing new group #9064
Fix exception when saving and autosave trigger at the same time #6694

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Siedlerchr · 2022-05-02T18:12:01Z

Still does not fix one of the root issues that there is no Filelock and that this method can be executed in parallel

koppor · 2022-05-02T18:17:47Z

(outdated)

Sure, this DOES NOT fix following issues: #6694, #8484, #6102, #4877, #8746. All of them are (currently) part of our next milestone v5.7: https://github.com/JabRef/jabref/milestone/26

This thing was one quick thought to implement following contract:

jabref/src/main/java/org/jabref/logic/exporter/AtomicFileWriter.java

Line 14 in dd173e0

    
            * underlying output stream. In this way, we make sure that the errors during the write process do not destroy the

…ive or dropbox)

This reverts commit 048729b.

tobiasdiez

I would suggest to take some inspiration by established libraries like https://github.com/npm/write-file-atomic or https://github.com/typicode/steno that implement similar secure writes, if you haven't done so. In particular, adding the pid and threadid, and invocation counter to the temporary file name looks like a good idea. Also having a thread lock-by-file is a good design choice in my opinion.

tobiasdiez · 2022-05-14T22:27:14Z

src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java

            cleanup();
        }
    }

+    private void replaceOriginalFileByWrittenFile() throws IOException {
+        // Move temporary file (replace original if it exists)
+        // We implement the move as write into the original and delete the temporary one to keep file permissions etc.


Then hard-exiting JabRef during a write operation may leave the file corrupt and kind of defeats the purpose of the atomic writer.

Note to self: I'll create an ADR. - The intended implementation fixes #8887.

koppor · 2022-05-16T20:54:45Z

Refs #6688, because users don't want that directory to be cluttered.

Siedlerchr · 2022-06-21T20:56:03Z

Another more general thing I just came across as I looked into this saving issues again:

We call the AtomicOutputStream with a NIO-File-Channel based Stream (ChannelOutputstream) which wil fail the instanceof check and never create a File Lock => We would need to call the constructor with
super(new FileOutputStream(getPathOfTemporaryFile(path).toAbsolutePath().toString()));
Second problem: Each saving creates a new SaveDatabaseAction object which itself creates a new Writer and Outputstream.
The fileLock variable is an object var. To have any effect it would ne need to be static
Saving and writing happens on the javafx thread...

# Conflicts: # build.gradle # src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java

src/test/java/org/jabref/logic/exporter/AtomicFileWriterTest.java

src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java

# Conflicts: # CHANGELOG.md

src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java

… use line ending of library

…anager, too)

src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java

# Conflicts: # src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

Additionally - FileUtil vs. FileHelper comments Co-authored-by: Christoph <siedlerkiller@gmail.com> Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

Co-authored-by: Christoph <siedlerkiller@gmail.com> Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

src/main/java/org/jabref/logic/util/io/FileUtil.java

src/test/java/org/jabref/logic/util/io/FileUtilTest.java

src/test/java/org/jabref/logic/autosaveandbackup/BackupManagerTest.java

tobiasdiez · 2022-08-14T21:51:40Z

I did a bit of research how other (code) editors are handling this and was a bit surprised that vscode for example uses a simple write (not an atomic write). However, there are quite a few issues in the vscode repo about lost or corrupt files, see microsoft/vscode#98063 and linked issues. Other data points: visual studio uses atomic writer, sublime text has an option to toggle between them.

While going through vscode's code, I found a few really nice implementations of things that JabRef is struggling with. They might be handy to be used as reference. In general, the vscode is very readable and well-structured; so worth looking at.

https://github.com/microsoft/vscode/blob/78397428676e15782e253261358b0398c2a1149e/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopy.ts#L812 - the actual safe operation of a file, and how to navigate all the different user scenarios that might lead to issues (auto save just triggered, multiple save operations triggered by user, save operation triggered by user although there are no changes etc). One important point is that the have a "version" attached to each state of the document, and use this to decide the correct order of save operations (or which to cancel). If I'm not mistaken, it is even written to the file as a metadata, and then used to compare the in memory representation to possible changes in the file.
https://github.com/microsoft/vscode/blob/78397428676e15782e253261358b0398c2a1149e/src/vs/workbench/services/workingCopy/common/storedFileWorkingCopySaveParticipant.ts and https://github.com/microsoft/vscode/blob/4a130c40ed876644ed8af2943809d08221375408/src/vs/workbench/api/browser/mainThreadSaveParticipant.ts - handling of operations that run on save (e.g. our cleanup ops)
https://github.com/microsoft/vscode/blob/main/src/vs/workbench/browser/parts/editor/editorAutoSave.ts - auto save (nicely separated from everything else, using listeners to be notified about relevant changes in focus etc)
https://hackage.haskell.org/package/atomic-write - applies the permission update to the temporary file (that is then moved into place), if I remember correctly, we are updating the file after the move of the temporary file, which might lead to issues with file watchers like dropbox

# Conflicts: # src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java # src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java # src/main/java/org/jabref/logic/util/BackupFileType.java # src/main/java/org/jabref/logic/util/io/FileUtil.java # src/test/java/org/jabref/logic/autosaveandbackup/BackupManagerTest.java

github-actions · 2022-08-16T11:03:49Z

src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java

+
+import org.jabref.logic.util.BackupFileType;
+import org.jabref.logic.util.io.BackupFileUtil;
+import org.jabref.logic.util.io.FileUtil;


🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.imports.UnusedImportsCheck> _{reported by reviewdog 🐶}
Unused import - org.jabref.logic.util.io.FileUtil.

koppor · 2022-12-16T00:48:55Z

We fixed the AtomicFileOutputStream in summer. The discussions at #8750 (comment) show that we should re-iterate on the idea again. - Especially #7718 should be regarded.

We close this PR and need to go back. The ADR https://github.com/JabRef/jabref/pull/8750/files#diff-123ad108d1738a32004ec352c9156e0088de245e0cb840687b658725c40bbf96 can still from a good basis for capturing the implementation decisions.

Write to final file only of no exception during write happened

242e799

ThiloteE added the export / save label May 6, 2022

koppor added 9 commits May 12, 2022 21:57

Merge remote-tracking branch 'origin/main' into more-conservative-save

125e09e

Merge remote-tracking branch 'origin/main' into more-conservative-save

69ba879

Add log on disk

048729b

Add writing to a local tmp file (instead of somewhere on a network dr…

2fb0657

…ive or dropbox)

Fix logger

1671496

Change file handling during writing

977e2e2

Initial AtomicFileWriterTest

292de7a

Add test case

3e5f117

Revert "Add log on disk"

902b52e

This reverts commit 048729b.

tobiasdiez requested changes May 14, 2022

View reviewed changes

koppor mentioned this pull request Jun 6, 2022

I suggest adding the option to select the auto save path #8887

Open

Merge branch 'main' into more-conservative-save

a9172e6

# Conflicts: # build.gradle # src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java

github-actions bot reviewed Aug 8, 2022

View reviewed changes

koppor added 10 commits August 11, 2022 00:05

Merge remote-tracking branch 'origin/main' into more-conservative-save

e948789

Merge remote-tracking branch 'origin/main' into more-conservative-save

67e181c

Fix checkstyle

84d09d2

Merge remote-tracking branch 'origin/main' into more-conservative-save

1c76e6d

# Conflicts: # CHANGELOG.md

Remove "keepBackup" from AtomicFileOutputStream

36f4cf1

Add ADR-0026

2d21fe5

Add links to ADR

be4d336

Use more clear method

75e0aed

Fix typos

40157de

Add getUniqueFilePrefix(Path)

9054614

github-actions bot reviewed Aug 11, 2022

View reviewed changes

src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java Outdated Show resolved Hide resolved

koppor added 5 commits August 11, 2022 23:20

Merge remote-tracking branch 'origin/main' into more-conservative-save

7bbf120

Fix checkstyle

45d01ac

Fix org.jabref.logic.autosaveandbackup.BackupManager#performBackup to…

b8fc4f6

… use line ending of library

Fix typos

ba392b3

Move path determination method to FileUtil (will be reused at BackupM…

4becafb

…anager, too)

github-actions bot reviewed Aug 11, 2022

View reviewed changes

src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java Outdated Show resolved Hide resolved

koppor and others added 5 commits August 13, 2022 21:04

Merge remote-tracking branch 'origin/main' into more-conservative-save

a35a6de

# Conflicts: # src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

Introduce BackupFileType

ee078fd

Additionally - FileUtil vs. FileHelper comments Co-authored-by: Christoph <siedlerkiller@gmail.com> Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

Rename method

bc7853c

Co-authored-by: Christoph <siedlerkiller@gmail.com> Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

Add test for getPathOfBackupFileAndCreateDirectory()

d1dc0f4

Co-authored-by: Christoph <siedlerkiller@gmail.com> Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

Start to implement multiple backups

8a08df7

Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

github-actions bot reviewed Aug 13, 2022

View reviewed changes

ThiloteE added this to the v5.8 milestone Aug 13, 2022

calixtus mentioned this pull request Aug 14, 2022

Refine BackupManager #9054

Merged

6 tasks

koppor added 2 commits August 16, 2022 12:29

Fix AtomicFileOutputStreamTest

60ad4d1

github-actions bot reviewed Aug 16, 2022

View reviewed changes

koppor modified the milestones: v5.8, v6.0 Aug 16, 2022

koppor closed this Dec 16, 2022

koppor mentioned this pull request Dec 16, 2022

JabRef breaks hard links to .bib files #7718

Open

koppor deleted the more-conservative-save branch December 18, 2022 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More conservative write #8750

More conservative write #8750

koppor commented May 2, 2022 •

edited by ThiloteE

Loading

Siedlerchr commented May 2, 2022

koppor commented May 2, 2022 •

edited

Loading

tobiasdiez left a comment •

edited

Loading

tobiasdiez May 14, 2022

koppor Jun 6, 2022

koppor commented May 16, 2022

Siedlerchr commented Jun 21, 2022

tobiasdiez commented Aug 14, 2022

github-actions bot Aug 16, 2022

koppor commented Dec 16, 2022

More conservative write #8750

More conservative write #8750

Conversation

koppor commented May 2, 2022 • edited by ThiloteE Loading

Siedlerchr commented May 2, 2022

koppor commented May 2, 2022 • edited Loading

tobiasdiez left a comment • edited Loading

Choose a reason for hiding this comment

tobiasdiez May 14, 2022

Choose a reason for hiding this comment

koppor Jun 6, 2022

Choose a reason for hiding this comment

koppor commented May 16, 2022

Siedlerchr commented Jun 21, 2022

tobiasdiez commented Aug 14, 2022

github-actions bot Aug 16, 2022

Choose a reason for hiding this comment

koppor commented Dec 16, 2022

koppor commented May 2, 2022 •

edited by ThiloteE

Loading

koppor commented May 2, 2022 •

edited

Loading

tobiasdiez left a comment •

edited

Loading