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

AtomicFileOutputStream does not overwrite file if exception occurred during write #9067

Merged
merged 8 commits into from
Aug 16, 2022

Conversation

koppor
Copy link
Member

@koppor koppor commented Aug 16, 2022

This refines the AtomicFileOutputStream to not overwrite a file if an exception occurred during write. This is done by introducing a flag errorDuringWrite and handling that in the close() method.


  • 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
Copy link
Member

If you have now autosave and normal saving at the same time you will get an exception due to the File lock because of this condition:

   try {
            // Lock files (so that at least not another JabRef instance writes at the same time to the same tmp file)
            if (out instanceof FileOutputStream) {
                temporaryFileLock = ((FileOutputStream) out).getChannel().lock();
            } else {
                temporaryFileLock = null;
            }
        } catch (OverlappingFileLockException exception) {
            throw new IOException("Could not obtain write access to " + temporaryFile + ". Maybe another instance of JabRef is currently writing to the same file?", exception);
        }

@Siedlerchr
Copy link
Member

Siedlerchr commented Aug 16, 2022

Please also note:

Platform dependencies

This file-locking API is intended to map directly to the native locking facility of the underlying operating system. Thus the locks held on a file should be visible to all programs that have access to the file, regardless of the language in which those programs are written.

Whether or not a lock actually prevents another program from accessing the content of the locked region is system-dependent and therefore unspecified. The native file-locking facilities of some systems are merely advisory, meaning that programs must cooperatively observe a known locking protocol in order to guarantee data integrity. On other systems native file locks are mandatory, meaning that if one program locks a region of a file then other programs are actually prevented from accessing that region in a way that would violate the lock. On yet other systems, whether native file locks are advisory or mandatory is configurable on a per-file basis. To ensure consistent and correct behavior across platforms, it is strongly recommended that the locks provided by this API be used as if they were advisory locks.

On some systems, acquiring a mandatory lock on a region of a file prevents that region from being mapped into memory, and vice versa. Programs that combine locking and mapping should be prepared for this combination to fail.

On some systems, closing a channel releases all locks held by the Java virtual machine on the underlying file regardless of whether the locks were acquired via that channel or via another channel open on the same file. It is strongly recommended that, within a program, a unique channel be used to acquire all locks on any given file.

Some network filesystems permit file locking to be used with memory-mapped files only when the locked regions are page-aligned and a whole multiple of the underlying hardware's page size. Some network filesystems do not implement file locks on regions that extend past a certain position, often 230 or 231. In general, great care should be taken when locking files that reside on network filesystems.

DEBUG: Unable to release lock on file /Users/christophs/Library/Application Support/JabRef/100.0.0/backups/60e5716b--Endnote.bib--2022-08-16--21.44.48.bak.tmp: java.nio.channels.ClosedChannelException
	at java.base/sun.nio.ch.FileLockImpl.release(FileLockImpl.java:58)
	at org.jabref/org.jabref.logic.exporter.AtomicFileOutputStream.cleanup(AtomicFileOutputStream.java:163)
	at org.jabref/org.jabref.logic.exporter.AtomicFileOutputStream.close(AtomicFileOutputStream.java:246)
	at java.base/sun.nio.cs.StreamEncoder.implClose(StreamEncoder.java:347)
	at java.base/sun.nio.cs.StreamEncoder.close(StreamEncoder.java:169)
	at java.base/java.io.OutputStreamWriter.close(OutputStreamWriter.java:254)
	at org.jabref/org.jabref.logic.autosaveandbackup.BackupManager.performBackup(BackupManager.java:204)
	at java.base/java.util.Optional.ifPresent(Optional.java:178)
	at org.jabref/org.jabref.logic.autosaveandbackup.BackupManager.lambda$4(BackupManager.java:234)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)

koppor and others added 4 commits August 16, 2022 22:12
Co-authored-by: Christoph <siedlerkiller@gmail.com>
Co-authored-by: ThiloteE <73715071+ThiloteE@users.noreply.github.com>
…cation of save actions

Co-authored-by: Christoph <siedlerkiller@gmail.com>
Co-authored-by: ThiloteE <73715071+ThiloteE@users.noreply.github.com>
/**
* Creates a new output stream to write to or replace the file at the specified path.
*
* @param path the path of the file to write to or replace
* @param keepBackup whether to keep the backup file after a successful write process
*/
public AtomicFileOutputStream(Path path, boolean keepBackup) throws IOException {
super(Files.newOutputStream(getPathOfTemporaryFile(path)));
// Files.newOutputStream(getPathOfTemporaryFile(path)) leads to a "sun.nio.ch.ChannelOutputStream", which does not offer "lock"
Copy link
Member

Choose a reason for hiding this comment

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

Useless comment

Copy link
Member

Choose a reason for hiding this comment

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

No, the comment is indeed useful because it makes clear why we chose the old io and not the nio one

@calixtus calixtus merged commit 6a71395 into main Aug 16, 2022
@calixtus calixtus deleted the refine-aotmic-output branch August 16, 2022 21:36
Siedlerchr added a commit that referenced this pull request Aug 21, 2022
* upstream/main:
  AtomicFileOutputStream does not overwrite file if exception occurred during write (#9067)
  Observable Preferences L (GrobidPreferences) (#9065)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment