diff --git a/CHANGELOG.md b/CHANGELOG.md index 560c53118de..a869d7a09f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve - The [HtmlToLaTeXFormatter](https://docs.jabref.org/finding-sorting-and-cleaning-entries/saveactions#html-to-latex) keeps single `<` characters. - We fixed a performance regression when opening large libraries [#9041](https://github.com/JabRef/jabref/issues/9041) - We fixed a bug where spaces are trimmed when highlighting differences in the Entries merge dialog. [koppor#371](https://github.com/koppor/jabref/issues/371) +- We fixed several bugs regarding the manual and the autosave of library files that sometimes lead to exceptions or data loss. [#8448](https://github.com/JabRef/jabref/issues/8484), [#8746](https://github.com/JabRef/jabref/issues/8746), [#6684](https://github.com/JabRef/jabref/issues/6684), [#6644](https://github.com/JabRef/jabref/issues/6644), [#6102](https://github.com/JabRef/jabref/issues/6102), [#6002](https://github.com/JabRef/jabref/issues/6000) +- We fixed an issue where applied save actions on saving the library file would lead to the dialog "The libary has been modified by another program" popping up [#4877](https://github.com/JabRef/jabref/issues/4877) ### Removed diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index 575307bcda8..5cff210b5d0 100644 --- a/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/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) { diff --git a/src/main/java/org/jabref/gui/dialogs/AutosaveUiManager.java b/src/main/java/org/jabref/gui/dialogs/AutosaveUiManager.java index 7aee99be695..a7bc85890ee 100644 --- a/src/main/java/org/jabref/gui/dialogs/AutosaveUiManager.java +++ b/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); } diff --git a/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java b/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java index e22f5790ceb..84bf9346e23 100644 --- a/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java +++ b/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 encodingProblems, SavePreferences.DatabaseSaveType saveType) throws SaveException { diff --git a/src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java b/src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java index 3e53d41357d..990da5c2db4 100644 --- a/src/main/java/org/jabref/logic/autosaveandbackup/AutosaveManager.java +++ b/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 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); } } } diff --git a/src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java b/src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java index 750f201ee83..5b3f0cd8863 100644 --- a/src/main/java/org/jabref/logic/autosaveandbackup/BackupManager.java +++ b/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 runningInstances = new HashSet<>(); diff --git a/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java b/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java index 09b0e25f460..5694d79b803 100644 --- a/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java +++ b/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,13 +60,17 @@ 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. * @@ -73,10 +78,27 @@ public class AtomicFileOutputStream extends FilterOutputStream { * @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,7 +130,7 @@ 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 { @@ -125,6 +138,7 @@ public void write(byte b[], int off, int len) throws IOException { 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 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) { diff --git a/src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java b/src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java new file mode 100644 index 00000000000..80086d77db9 --- /dev/null +++ b/src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java @@ -0,0 +1,68 @@ +package org.jabref.logic.exporter; + +import java.io.ByteArrayInputStream; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; + +import com.google.common.base.Strings; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.Mockito; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.spy; + +class AtomicFileOutputStreamTest { + + private static final String FIFTY_CHARS = Strings.repeat("1234567890", 5); + private static final String FIVE_THOUSAND_CHARS = Strings.repeat("A", 5_000); + + @Test + public void normalSaveWorks(@TempDir Path tempDir) throws Exception { + Path out = tempDir.resolve("normal-save.txt"); + Files.writeString(out, FIFTY_CHARS); + + try (AtomicFileOutputStream atomicFileOutputStream = new AtomicFileOutputStream(out)) { + InputStream inputStream = new ByteArrayInputStream(FIVE_THOUSAND_CHARS.getBytes()); + inputStream.transferTo(atomicFileOutputStream); + } + + // Written file still has the contents as before the error + assertEquals(FIVE_THOUSAND_CHARS, Files.readString(out)); + } + + @Test + public void originalContentExistsAtWriteError(@TempDir Path tempDir) throws Exception { + Path pathToTestFile = tempDir.resolve("error-during-save.txt"); + Files.writeString(pathToTestFile, FIFTY_CHARS); + + Path pathToTmpFile = tempDir.resolve("error-during-save.txt.tmp"); + + try (FileOutputStream outputStream = new FileOutputStream(pathToTmpFile.toFile())) { + FileOutputStream spiedOutputStream = spy(outputStream); + doAnswer(invocation -> { + // by writing one byte, we ensure that the `.tmp` file is created + outputStream.write(((byte[]) invocation.getRawArguments()[0])[0]); + outputStream.flush(); + throw new IOException(); + }).when(spiedOutputStream) + .write(Mockito.any(byte[].class), anyInt(), anyInt()); + + assertThrows(IOException.class, () -> { + try (AtomicFileOutputStream atomicFileOutputStream = new AtomicFileOutputStream(pathToTestFile, pathToTmpFile, spiedOutputStream, false); + InputStream inputStream = new ByteArrayInputStream(FIVE_THOUSAND_CHARS.getBytes())) { + inputStream.transferTo(atomicFileOutputStream); + } + }); + } + + // Written file still has the contents as before the error + assertEquals(FIFTY_CHARS, Files.readString(pathToTestFile)); + } +} diff --git a/src/test/java/org/jabref/model/database/event/AutosaveEventTest.java b/src/test/java/org/jabref/model/database/event/AutosaveEventTest.java deleted file mode 100644 index 10ed6f709de..00000000000 --- a/src/test/java/org/jabref/model/database/event/AutosaveEventTest.java +++ /dev/null @@ -1,14 +0,0 @@ -package org.jabref.model.database.event; - -import org.junit.jupiter.api.Test; - -import static org.junit.jupiter.api.Assertions.assertNotNull; - -public class AutosaveEventTest { - - @Test - public void givenNothingWhenCreatingThenNotNull() { - AutosaveEvent e = new AutosaveEvent(); - assertNotNull(e); - } -}