AtomicFileOutputStream does not overwrite file if exception occurred during write

src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java

@@ -58,7 +58,7 @@ public DatabaseChangeMonitor(BibDatabaseContext database,
             try {
                 fileMonitor.addListenerForFile(path, this);
             } catch (IOException e) {
-                LOGGER.error("Error while trying to monitor " + path, e);
+                LOGGER.error("Error while trying to monitor {}", path, e);
             }
         });
 
@@ -75,16 +75,18 @@ public DatabaseChangeMonitor(BibDatabaseContext database,
 
     @Override
     public void fileUpdated() {
-        // File on disk has changed, thus look for notable changes and notify listeners in case there are such changes
-        ChangeScanner scanner = new ChangeScanner(database, dialogService, preferencesService, stateManager, themeManager);
-        BackgroundTask.wrap(scanner::scanForChanges)
-                      .onSuccess(changes -> {
-                          if (!changes.isEmpty()) {
-                              listeners.forEach(listener -> listener.databaseChanged(changes));
-                          }
-                      })
-                      .onFailure(e -> LOGGER.error("Error while watching for changes", e))
-                      .executeWith(taskExecutor);
+        synchronized (database) {
+            // File on disk has changed, thus look for notable changes and notify listeners in case there are such changes
+            ChangeScanner scanner = new ChangeScanner(database, dialogService, preferencesService, stateManager, themeManager);
+            BackgroundTask.wrap(scanner::scanForChanges)
+                          .onSuccess(changes -> {
+                              if (!changes.isEmpty()) {
+                                  listeners.forEach(listener -> listener.databaseChanged(changes));
+                              }
+                          })
+                          .onFailure(e -> LOGGER.error("Error while watching for changes", e))
+                          .executeWith(taskExecutor);
+        }
     }
 
     public void addListener(DatabaseChangeListener listener) {

src/main/java/org/jabref/gui/dialogs/AutosaveUiManager.java

@@ -16,16 +16,16 @@
 public class AutosaveUiManager {
     private static final Logger LOGGER = LoggerFactory.getLogger(AutosaveUiManager.class);
 
-    private final LibraryTab libraryTab;
+    private SaveDatabaseAction saveDatabaseAction;
 
     public AutosaveUiManager(LibraryTab libraryTab) {
-        this.libraryTab = libraryTab;
+        this.saveDatabaseAction = new SaveDatabaseAction(libraryTab, Globals.prefs, Globals.entryTypesManager);
     }
 
     @Subscribe
     public void listen(AutosaveEvent event) {
         try {
-            new SaveDatabaseAction(libraryTab, Globals.prefs, Globals.entryTypesManager).save(SaveDatabaseAction.SaveDatabaseMode.SILENT);
+            this.saveDatabaseAction.save(SaveDatabaseAction.SaveDatabaseMode.SILENT);
         } catch (Throwable e) {
             LOGGER.error("Problem occurred while saving.", e);
         }

src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

@@ -192,13 +192,21 @@ private boolean save(Path targetPath, SaveDatabaseMode mode) {
             dialogService.notify(String.format("%s...", Localization.lang("Saving library")));
         }
 
-        libraryTab.setSaving(true);
+        synchronized (libraryTab) {
+            if (libraryTab.isSaving()) {
+                // if another thread is saving, we do not need to save
+                return true;
+            }
+            libraryTab.setSaving(true);
+        }
+
         try {
             Charset encoding = libraryTab.getBibDatabaseContext()
                                          .getMetaData()
                                          .getEncoding()
                                          .orElse(StandardCharsets.UTF_8);
-            // Make sure to remember which encoding we used.
+
+            // Make sure to remember which encoding we used
             libraryTab.getBibDatabaseContext().getMetaData().setEncoding(encoding, ChangePropagation.DO_NOT_POST_EVENT);
 
             // Save the database
@@ -227,28 +235,29 @@ private boolean saveDatabase(Path file, boolean selectedOnly, Charset encoding,
         SavePreferences savePreferences = this.preferences.getSavePreferences()
                                                       .withSaveType(saveType);
         BibDatabaseContext bibDatabaseContext = libraryTab.getBibDatabaseContext();
-        try (AtomicFileWriter fileWriter = new AtomicFileWriter(file, encoding, savePreferences.shouldMakeBackup())) {
-            BibWriter bibWriter = new BibWriter(fileWriter, bibDatabaseContext.getDatabase().getNewLineSeparator());
-            BibtexDatabaseWriter databaseWriter = new BibtexDatabaseWriter(bibWriter, generalPreferences, savePreferences, entryTypesManager);
-
-            if (selectedOnly) {
-                databaseWriter.savePartOfDatabase(bibDatabaseContext, libraryTab.getSelectedEntries());
-            } else {
-                databaseWriter.saveDatabase(bibDatabaseContext);
-            }
+        synchronized (bibDatabaseContext) {
+            try (AtomicFileWriter fileWriter = new AtomicFileWriter(file, encoding, savePreferences.shouldMakeBackup())) {
+                BibWriter bibWriter = new BibWriter(fileWriter, bibDatabaseContext.getDatabase().getNewLineSeparator());
+                BibtexDatabaseWriter databaseWriter = new BibtexDatabaseWriter(bibWriter, generalPreferences, savePreferences, entryTypesManager);
+
+                if (selectedOnly) {
+                    databaseWriter.savePartOfDatabase(bibDatabaseContext, libraryTab.getSelectedEntries());
+                } else {
+                    databaseWriter.saveDatabase(bibDatabaseContext);
+                }
 
-            libraryTab.registerUndoableChanges(databaseWriter.getSaveActionsFieldChanges());
+                libraryTab.registerUndoableChanges(databaseWriter.getSaveActionsFieldChanges());
 
-            if (fileWriter.hasEncodingProblems()) {
-                saveWithDifferentEncoding(file, selectedOnly, encoding, fileWriter.getEncodingProblems(), saveType);
+                if (fileWriter.hasEncodingProblems()) {
+                    saveWithDifferentEncoding(file, selectedOnly, encoding, fileWriter.getEncodingProblems(), saveType);
+                }
+            } catch (UnsupportedCharsetException ex) {
+                throw new SaveException(Localization.lang("Character encoding '%0' is not supported.", encoding.displayName()), ex);
+            } catch (IOException ex) {
+                throw new SaveException("Problems saving: " + ex, ex);
             }
-        } catch (UnsupportedCharsetException ex) {
-            throw new SaveException(Localization.lang("Character encoding '%0' is not supported.", encoding.displayName()), ex);
-        } catch (IOException ex) {
-            throw new SaveException("Problems saving: " + ex, ex);
+            return true;
         }
-
-        return true;
     }
 
     private void saveWithDifferentEncoding(Path file, boolean selectedOnly, Charset encoding, Set<Character> encodingProblems, SavePreferences.DatabaseSaveType saveType) throws SaveException {

src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java

@@ -3,9 +3,9 @@
 import java.util.HashSet;
 import java.util.Set;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 
 import org.jabref.logic.util.CoarseChangeFilter;
-import org.jabref.logic.util.DelayTaskThrottler;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.database.event.AutosaveEvent;
 import org.jabref.model.database.event.BibDatabaseContextChangedEvent;
@@ -24,35 +24,47 @@ public class AutosaveManager {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(AutosaveManager.class);
 
+    private static final int DELAY_BETWEEN_AUTOSAVE_ATTEMPTS_IN_SECONDS = 31;
+
     private static Set<AutosaveManager> runningInstances = new HashSet<>();
 
     private final BibDatabaseContext bibDatabaseContext;
 
     private final EventBus eventBus;
     private final CoarseChangeFilter changeFilter;
-    private final DelayTaskThrottler throttler;
+    private final ScheduledThreadPoolExecutor executor;
+    private boolean needsSave = false;
 
     private AutosaveManager(BibDatabaseContext bibDatabaseContext) {
         this.bibDatabaseContext = bibDatabaseContext;
-        this.throttler = new DelayTaskThrottler(2000);
         this.eventBus = new EventBus();
         this.changeFilter = new CoarseChangeFilter(bibDatabaseContext);
         changeFilter.registerListener(this);
+
+        this.executor = new ScheduledThreadPoolExecutor(2);
+        this.executor.scheduleAtFixedRate(
+                () -> {
+                    if (needsSave) {
+                       eventBus.post(new AutosaveEvent());
+                       needsSave = false;
+                    }
+                },
+                DELAY_BETWEEN_AUTOSAVE_ATTEMPTS_IN_SECONDS,
+                DELAY_BETWEEN_AUTOSAVE_ATTEMPTS_IN_SECONDS,
+                TimeUnit.SECONDS);
     }
 
     @Subscribe
     public void listen(@SuppressWarnings("unused") BibDatabaseContextChangedEvent event) {
         if (!event.isFilteredOut()) {
-            throttler.schedule(() -> {
-                eventBus.post(new AutosaveEvent());
-            });
+            this.needsSave = true;
         }
     }
 
     private void shutdown() {
         changeFilter.unregisterListener(this);
         changeFilter.shutdown();
-        throttler.shutdown();
+        executor.shutdown();
     }
 
     /**
@@ -88,7 +100,7 @@ public void unregisterListener(Object listener) {
             eventBus.unregister(listener);
         } catch (IllegalArgumentException e) {
             // occurs if the event source has not been registered, should not prevent shutdown
-            LOGGER.debug("Proble, unregistering", e);
+            LOGGER.debug("Problem unregistering", e);
         }
     }
 }

src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java

@@ -48,7 +48,7 @@ public class BackupManager {
 
     private static final int MAXIMUM_BACKUP_FILE_COUNT = 10;
 
-    private static final int DELAY_BETWEEN_BACKUP_ATTEMPTS_IN_SECONDS = 20;
+    private static final int DELAY_BETWEEN_BACKUP_ATTEMPTS_IN_SECONDS = 19;
 
     private static Set<BackupManager> runningInstances = new HashSet<>();
 

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

@@ -3,6 +3,7 @@
 import java.io.FileOutputStream;
 import java.io.FilterOutputStream;
 import java.io.IOException;
+import java.io.OutputStream;
 import java.nio.channels.FileLock;
 import java.nio.channels.OverlappingFileLockException;
 import java.nio.file.Files;
@@ -48,7 +49,7 @@ public class AtomicFileOutputStream extends FilterOutputStream {
     private static final Logger LOGGER = LoggerFactory.getLogger(AtomicFileOutputStream.class);
 
     private static final String TEMPORARY_EXTENSION = ".tmp";
-    private static final String SAVE_EXTENSION = BackupFileType.SAVE.getExtensions().get(0);
+    private static final String SAVE_EXTENSION = "." + BackupFileType.SAVE.getExtensions().get(0);
 
     /**
      * The file we want to create/replace.
@@ -59,24 +60,45 @@ public class AtomicFileOutputStream extends FilterOutputStream {
      * The file to which writes are redirected to.
      */
     private final Path temporaryFile;
+
     private final FileLock temporaryFileLock;
     /**
      * A backup of the target file (if it exists), created when the stream is closed
      */
     private final Path backupFile;
+
     private final boolean keepBackup;
 
+    private boolean errorDuringWrite = false;
+
     /**
      * 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"
+        this(path, getPathOfTemporaryFile(path), new FileOutputStream(getPathOfTemporaryFile(path).toFile()), keepBackup);
+    }
+
+    /**
+     * Creates a new output stream to write to or replace the file at the specified path.
+     * The backup file is deleted when write was successful.
+     *
+     * @param path the path of the file to write to or replace
+     */
+    public AtomicFileOutputStream(Path path) throws IOException {
+        this(path, false);
+    }
 
+    /**
+     * Required for proper testing
+     */
+    AtomicFileOutputStream(Path path, Path pathOfTemporaryFile, OutputStream temporaryFileOutputStream, boolean keepBackup) throws IOException {
+        super(temporaryFileOutputStream);
         this.targetFile = path;
-        this.temporaryFile = getPathOfTemporaryFile(path);
+        this.temporaryFile = pathOfTemporaryFile;
         this.backupFile = getPathOfSaveBackupFile(path);
         this.keepBackup = keepBackup;
 
@@ -92,15 +114,6 @@ public AtomicFileOutputStream(Path path, boolean keepBackup) throws IOException
         }
     }
 
-    /**
-     * Creates a new output stream to write to or replace the file at the specified path. The backup file is deleted when the write was successful.
-     *
-     * @param path the path of the file to write to or replace
-     */
-    public AtomicFileOutputStream(Path path) throws IOException {
-        this(path, false);
-    }
-
     private static Path getPathOfTemporaryFile(Path targetFile) {
         return FileUtil.addExtension(targetFile, TEMPORARY_EXTENSION);
     }
@@ -117,14 +130,15 @@ public Path getBackup() {
     }
 
     /**
-     * Override for performance reasons.
+     * Overridden because of cleanup actions in case of an error
      */
     @Override
     public void write(byte b[], int off, int len) throws IOException {
         try {
             out.write(b, off, len);
         } catch (IOException exception) {
             cleanup();
+            errorDuringWrite = true;
             throw exception;
         }
     }
@@ -133,32 +147,36 @@ public void write(byte b[], int off, int len) throws IOException {
      * Closes the write process to the temporary file but does not commit to the target file.
      */
     public void abort() {
+        errorDuringWrite = true;
         try {
             super.close();
             Files.deleteIfExists(temporaryFile);
             Files.deleteIfExists(backupFile);
         } catch (IOException exception) {
-            LOGGER.debug("Unable to abort writing to file " + temporaryFile, exception);
+            LOGGER.debug("Unable to abort writing to file {}", temporaryFile, exception);
         }
     }
 
     private void cleanup() {
-        try {
-            Files.deleteIfExists(temporaryFile);
-        } catch (IOException exception) {
-            LOGGER.debug("Unable to delete file " + temporaryFile, exception);
-        }
-
         try {
             if (temporaryFileLock != null) {
                 temporaryFileLock.release();
             }
         } catch (IOException exception) {
-            LOGGER.warn("Unable to release lock on file " + temporaryFile, exception);
+            // Currently, we always get the exception:
+            // Unable to release lock on file C:\Users\koppor\AppData\Local\Temp\junit11976839611279549873\error-during-save.txt.tmp: java.nio.channels.ClosedChannelException
+            LOGGER.debug("Unable to release lock on file {}", temporaryFile, exception);
+        }
+        try {
+            Files.deleteIfExists(temporaryFile);
+        } catch (IOException exception) {
+            LOGGER.debug("Unable to delete file {}", temporaryFile, exception);
         }
     }
 
-    // perform the final operations to move the temporary file to its final destination
+    /**
+     * perform the final operations to move the temporary file to its final destination
+     */
     @Override
     public void close() throws IOException {
         try {
@@ -175,6 +193,11 @@ public void close() throws IOException {
             }
             super.close();
 
+            if (errorDuringWrite) {
+                // in case there was an error during write, we do not replace the original file
+                return;
+            }
+
             // We successfully wrote everything to the temporary file, lets copy it to the correct place
             // First, make backup of original file and try to save file permissions to restore them later (by default: 664)
             Set<PosixFilePermission> oldFilePermissions = EnumSet.of(PosixFilePermission.OWNER_READ,
@@ -183,7 +206,11 @@ public void close() throws IOException {
                     PosixFilePermission.GROUP_WRITE,
                     PosixFilePermission.OTHERS_READ);
             if (Files.exists(targetFile)) {
-                Files.copy(targetFile, backupFile, StandardCopyOption.REPLACE_EXISTING);
+                try {
+                    Files.copy(targetFile, backupFile, StandardCopyOption.REPLACE_EXISTING);
+                } catch (Exception e) {
+                    LOGGER.warn("Could not create backup file {}", backupFile);
+                }
                 if (FileUtil.IS_POSIX_COMPLIANT) {
                     try {
                         oldFilePermissions = Files.getPosixFilePermissions(targetFile);
@@ -193,8 +220,13 @@ public void close() throws IOException {
                 }
             }
 
-            // Move temporary file (replace original if it exists)
-            Files.move(temporaryFile, targetFile, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+            try {
+                // Move temporary file (replace original if it exists)
+                Files.move(temporaryFile, targetFile, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+            } catch (Exception e) {
+                LOGGER.warn("Could not move temporary file", e);
+                throw e;
+            }
 
             // Restore file permissions
             if (FileUtil.IS_POSIX_COMPLIANT) {