diff --git a/gradle/changelog/temporary-files.yaml b/gradle/changelog/temporary-files.yaml new file mode 100644 index 0000000000..eca428419b --- /dev/null +++ b/gradle/changelog/temporary-files.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Deletion of temporary files on exceptions diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java b/scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java index 12ef73e667..46fcba4420 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java +++ b/scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java @@ -62,7 +62,13 @@ public final class CopyOnWrite { validateInput(targetFile); execute(() -> { Path temporaryFile = createTemporaryFile(targetFile); - executeCallback(writer, targetFile, temporaryFile); + try { + executeCallback(writer, targetFile, temporaryFile); + } catch (Exception e) { + LOG.warn("error writing temporary file"); + deleteTemporaryFile(temporaryFile); + throw e; + } replaceOriginalFile(targetFile, temporaryFile); onSuccess.run(); }).withLockedFileForWrite(targetFile); @@ -170,7 +176,7 @@ public final class CopyOnWrite { restoreBackup(targetFile, backupFile); throw new StoreException("could rename temporary file to target file", e); } - deleteBackupFile(backupFile); + deleteTemporaryFile(backupFile); } @SuppressWarnings("squid:S3725") // performance of Files#exists @@ -190,7 +196,7 @@ public final class CopyOnWrite { } } - private static void deleteBackupFile(Path backupFile) { + private static void deleteTemporaryFile(Path backupFile) { if (backupFile != null) { try { Files.delete(backupFile); diff --git a/scm-dao-xml/src/test/java/sonia/scm/store/CopyOnWriteTest.java b/scm-dao-xml/src/test/java/sonia/scm/store/CopyOnWriteTest.java index 3abecdcf73..9025f993e0 100644 --- a/scm-dao-xml/src/test/java/sonia/scm/store/CopyOnWriteTest.java +++ b/scm-dao-xml/src/test/java/sonia/scm/store/CopyOnWriteTest.java @@ -24,7 +24,6 @@ package sonia.scm.store; -import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -35,6 +34,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; import static sonia.scm.store.CopyOnWrite.withTemporaryFile; @@ -50,7 +50,7 @@ class CopyOnWriteTest { } }, expectedFile); - Assertions.assertThat(expectedFile).hasContent("great success"); + assertThat(expectedFile).hasContent("great success"); } @Test @@ -64,7 +64,7 @@ class CopyOnWriteTest { } }, expectedFile); - Assertions.assertThat(expectedFile).hasContent("great success"); + assertThat(expectedFile).hasContent("great success"); } @Test @@ -101,7 +101,22 @@ class CopyOnWriteTest { }, unchangedOriginalFile)); - Assertions.assertThat(unchangedOriginalFile).hasContent("this should be kept"); + assertThat(unchangedOriginalFile).hasContent("this should be kept"); + } + + @Test + void shouldDeleteTemporaryFileIfFileCouldNotBeWritten(@TempDir Path tempDir) throws IOException { + Path unchangedOriginalFile = tempDir.resolve("target.txt"); + + assertThrows( + StoreException.class, + () -> withTemporaryFile( + file -> { + throw new IOException("test"); + }, + unchangedOriginalFile)); + + assertThat(tempDir).isEmptyDirectory(); } @Test @@ -131,7 +146,7 @@ class CopyOnWriteTest { Files::delete, backedUpFile)); - Assertions.assertThat(backedUpFile).hasContent("this should be kept"); + assertThat(backedUpFile).hasContent("this should be kept"); } @Test @@ -148,6 +163,6 @@ class CopyOnWriteTest { } }, expectedFile); - Assertions.assertThat(Files.list(tempDir)).hasSize(1); + assertThat(Files.list(tempDir)).hasSize(1); } }