Merged in feature/copy_on_write (pull request #369)

Copy on write

scm-dao-xml/src/main/java/sonia/scm/repository/xml/MetadataStore.java

@@ -5,6 +5,7 @@ import org.slf4j.LoggerFactory;
 import sonia.scm.ContextEntry;
 import sonia.scm.repository.InternalRepositoryException;
 import sonia.scm.repository.Repository;
+import sonia.scm.store.CopyOnWrite;
 import sonia.scm.store.StoreConstants;
 import sonia.scm.update.UpdateStepRepositoryMetadataAccess;
 
@@ -43,7 +44,10 @@ public class MetadataStore implements UpdateStepRepositoryMetadataAccess<Path> {
     try {
       Marshaller marshaller = jaxbContext.createMarshaller();
       marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, Boolean.TRUE);
-      marshaller.marshal(repository, resolveDataPath(path).toFile());
+      CopyOnWrite.withTemporaryFile(
+        temp -> marshaller.marshal(repository, temp.toFile()),
+        resolveDataPath(path)
+      );
     } catch (JAXBException ex) {
       throw new InternalRepositoryException(repository, "failed write repository metadata", ex);
     }

scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java

@@ -4,6 +4,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import sonia.scm.ContextEntry;
 import sonia.scm.repository.InternalRepositoryException;
+import sonia.scm.store.CopyOnWrite;
 import sonia.scm.xml.IndentXMLStreamWriter;
 import sonia.scm.xml.XmlStreams;
 
@@ -40,23 +41,28 @@ class PathDatabase {
     ensureParentDirectoryExists();
     LOG.trace("write repository path database to {}", storePath);
 
-    try (IndentXMLStreamWriter writer = XmlStreams.createWriter(storePath)) {
-      writer.writeStartDocument(ENCODING, VERSION);
+    CopyOnWrite.withTemporaryFile(
+      temp -> {
+        try (IndentXMLStreamWriter writer = XmlStreams.createWriter(temp)) {
+          writer.writeStartDocument(ENCODING, VERSION);
 
-      writeRepositoriesStart(writer, creationTime, lastModified);
-      for (Map.Entry<String, Path> e : pathDatabase.entrySet()) {
-        writeRepository(writer, e.getKey(), e.getValue());
-      }
-      writer.writeEndElement();
+          writeRepositoriesStart(writer, creationTime, lastModified);
+          for (Map.Entry<String, Path> e : pathDatabase.entrySet()) {
+            writeRepository(writer, e.getKey(), e.getValue());
+          }
+          writer.writeEndElement();
 
-      writer.writeEndDocument();
-    } catch (XMLStreamException | IOException ex) {
-      throw new InternalRepositoryException(
-        ContextEntry.ContextBuilder.entity(Path.class, storePath.toString()).build(),
-        "failed to write repository path database",
-        ex
-      );
-    }
+          writer.writeEndDocument();
+        } catch (XMLStreamException | IOException ex) {
+          throw new InternalRepositoryException(
+            ContextEntry.ContextBuilder.entity(Path.class, storePath.toString()).build(),
+            "failed to write repository path database",
+            ex
+          );
+        }
+      },
+      storePath
+    );
   }
 
   private void ensureParentDirectoryExists() {

scm-dao-xml/src/main/java/sonia/scm/store/CopyOnWrite.java

@@ -0,0 +1,112 @@
+package sonia.scm.store;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.UUID;
+
+public final class CopyOnWrite {
+
+  private static final Logger LOG = LoggerFactory.getLogger(CopyOnWrite.class);
+
+  private CopyOnWrite() {}
+
+  public static void withTemporaryFile(FileWriter writer, Path targetFile) {
+    validateInput(targetFile);
+    Path temporaryFile = createTemporaryFile(targetFile);
+    executeCallback(writer, targetFile, temporaryFile);
+    replaceOriginalFile(targetFile, temporaryFile);
+  }
+
+  @SuppressWarnings("squid:S3725") // performance of Files#isDirectory
+  private static void validateInput(Path targetFile) {
+    if (Files.isDirectory(targetFile)) {
+      throw new IllegalArgumentException("target file has to be a regular file, not a directory");
+    }
+    if (targetFile.getParent() == null) {
+      throw new IllegalArgumentException("target file has to be specified with a parent directory");
+    }
+  }
+
+  private static Path createTemporaryFile(Path targetFile) {
+    Path temporaryFile = targetFile.getParent().resolve(UUID.randomUUID().toString());
+    try {
+      Files.createFile(temporaryFile);
+    } catch (IOException ex) {
+      LOG.error("Error creating temporary file {} to replace file {}", temporaryFile, targetFile);
+      throw new StoreException("could not create temporary file", ex);
+    }
+    return temporaryFile;
+  }
+
+  private static void executeCallback(FileWriter writer, Path targetFile, Path temporaryFile) {
+    try {
+      writer.write(temporaryFile);
+    } catch (RuntimeException e) {
+      throw e;
+    } catch (Exception ex) {
+      LOG.error("Error writing to temporary file {}. Target file {} has not been modified", temporaryFile, targetFile);
+      throw new StoreException("could not write temporary file", ex);
+    }
+  }
+
+  private static void replaceOriginalFile(Path targetFile, Path temporaryFile) {
+    Path backupFile = backupOriginalFile(targetFile);
+    try {
+      Files.move(temporaryFile, targetFile);
+    } catch (IOException e) {
+      LOG.error("Error renaming temporary file {} to target file {}", temporaryFile, targetFile);
+      restoreBackup(targetFile, backupFile);
+      throw new StoreException("could rename temporary file to target file", e);
+    }
+    deleteBackupFile(backupFile);
+  }
+
+  @SuppressWarnings("squid:S3725") // performance of Files#exists
+  private static Path backupOriginalFile(Path targetFile) {
+    Path directory = targetFile.getParent();
+    if (Files.exists(targetFile)) {
+      Path backupFile = directory.resolve(UUID.randomUUID().toString());
+      try {
+        Files.move(targetFile, backupFile);
+      } catch (IOException e) {
+        LOG.error("Could not backup original file {}. Aborting here so that original file will not be overwritten.", targetFile);
+        throw new StoreException("could not create backup of file", e);
+      }
+      return backupFile;
+    } else {
+      return null;
+    }
+  }
+
+  private static void deleteBackupFile(Path backupFile) {
+    if (backupFile != null) {
+      try {
+        Files.delete(backupFile);
+      } catch (IOException e) {
+        LOG.warn("Could not delete backup file {}", backupFile);
+        throw new StoreException("could not delete backup file", e);
+      }
+    }
+  }
+
+  private static void restoreBackup(Path targetFile, Path backupFile) {
+    if (backupFile != null) {
+      try {
+        Files.move(backupFile, targetFile);
+        LOG.info("Recovered original file {} from backup", targetFile);
+      } catch (IOException e) {
+        LOG.error("Could not replace original file {} with backup file {} after failure", targetFile, backupFile);
+      }
+    }
+  }
+
+  @FunctionalInterface
+  public interface FileWriter {
+    @SuppressWarnings("squid:S00112") // We do not want to limit exceptions here
+    void write(Path t) throws Exception;
+  }
+}

scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java

@@ -317,48 +317,47 @@ public class JAXBConfigurationEntryStore<V> implements ConfigurationEntryStore<V
   {
     logger.debug("store configuration to {}", file);
 
-    try (IndentXMLStreamWriter writer = XmlStreams.createWriter(file))
-    {
-      writer.writeStartDocument();
+    CopyOnWrite.withTemporaryFile(
+      temp -> {
+        try (IndentXMLStreamWriter writer = XmlStreams.createWriter(temp)) {
+          writer.writeStartDocument();
 
-      // configuration start
-      writer.writeStartElement(TAG_CONFIGURATION);
+          // configuration start
+          writer.writeStartElement(TAG_CONFIGURATION);
 
-      Marshaller m = context.createMarshaller();
+          Marshaller m = context.createMarshaller();
 
-      m.setProperty(Marshaller.JAXB_FRAGMENT, Boolean.TRUE);
+          m.setProperty(Marshaller.JAXB_FRAGMENT, Boolean.TRUE);
 
-      for (Entry<String, V> e : entries.entrySet())
-      {
+          for (Entry<String, V> e : entries.entrySet()) {
 
-        // entry start
-        writer.writeStartElement(TAG_ENTRY);
+            // entry start
+            writer.writeStartElement(TAG_ENTRY);
 
-        // key start
-        writer.writeStartElement(TAG_KEY);
-        writer.writeCharacters(e.getKey());
+            // key start
+            writer.writeStartElement(TAG_KEY);
+            writer.writeCharacters(e.getKey());
 
-        // key end
-        writer.writeEndElement();
+            // key end
+            writer.writeEndElement();
 
-        // value
-        JAXBElement<V> je = new JAXBElement<V>(QName.valueOf(TAG_VALUE), type,
-                              e.getValue());
+            // value
+            JAXBElement<V> je = new JAXBElement<>(QName.valueOf(TAG_VALUE), type,
+              e.getValue());
 
-        m.marshal(je, writer);
+            m.marshal(je, writer);
 
-        // entry end
-        writer.writeEndElement();
-      }
+            // entry end
+            writer.writeEndElement();
+          }
 
-      // configuration end
-      writer.writeEndElement();
-      writer.writeEndDocument();
-    }
-    catch (Exception ex)
-    {
-      throw new StoreException("could not store configuration", ex);
-    }
+          // configuration end
+          writer.writeEndElement();
+          writer.writeEndDocument();
+        }
+      },
+      file.toPath()
+    );
   }
 
   //~--- fields ---------------------------------------------------------------

scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationStore.java

@@ -113,7 +113,10 @@ public class JAXBConfigurationStore<T> extends AbstractStore<T> {
       Marshaller marshaller = context.createMarshaller();
 
       marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, Boolean.TRUE);
-      marshaller.marshal(object, configFile);
+      CopyOnWrite.withTemporaryFile(
+        temp -> marshaller.marshal(object, temp.toFile()),
+        configFile.toPath()
+      );
     }
     catch (JAXBException ex) {
       throw new StoreException("failed to marshall object", ex);

scm-dao-xml/src/test/java/sonia/scm/store/CopyOnWriteTest.java

@@ -0,0 +1,114 @@
+package sonia.scm.store;
+
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junitpioneer.jupiter.TempDirectory;
+
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static sonia.scm.store.CopyOnWrite.withTemporaryFile;
+
+@ExtendWith(TempDirectory.class)
+class CopyOnWriteTest {
+
+  @Test
+  void shouldCreateNewFile(@TempDirectory.TempDir Path tempDir) {
+    Path expectedFile = tempDir.resolve("toBeCreated.txt");
+
+    withTemporaryFile(
+      file -> new FileOutputStream(file.toFile()).write("great success".getBytes()),
+      expectedFile);
+
+    Assertions.assertThat(expectedFile).hasContent("great success");
+  }
+
+  @Test
+  void shouldOverwriteExistingFile(@TempDirectory.TempDir Path tempDir) throws IOException {
+    Path expectedFile = tempDir.resolve("toBeOverwritten.txt");
+    Files.createFile(expectedFile);
+
+    withTemporaryFile(
+      file -> new FileOutputStream(file.toFile()).write("great success".getBytes()),
+      expectedFile);
+
+    Assertions.assertThat(expectedFile).hasContent("great success");
+  }
+
+  @Test
+  void shouldFailForDirectory(@TempDirectory.TempDir Path tempDir) {
+    assertThrows(IllegalArgumentException.class,
+      () -> withTemporaryFile(
+        file -> new FileOutputStream(file.toFile()).write("should not be written".getBytes()),
+        tempDir));
+  }
+
+  @Test
+  void shouldFailForMissingDirectory() {
+    assertThrows(
+      IllegalArgumentException.class,
+      () -> withTemporaryFile(
+        file -> new FileOutputStream(file.toFile()).write("should not be written".getBytes()),
+        Paths.get("someFile")));
+  }
+
+  @Test
+  void shouldKeepBackupIfTemporaryFileCouldNotBeWritten(@TempDirectory.TempDir Path tempDir) throws IOException {
+    Path unchangedOriginalFile = tempDir.resolve("notToBeDeleted.txt");
+    new FileOutputStream(unchangedOriginalFile.toFile()).write("this should be kept".getBytes());
+
+    assertThrows(
+      StoreException.class,
+      () -> withTemporaryFile(
+        file -> {
+          throw new IOException("test");
+        },
+        unchangedOriginalFile));
+
+    Assertions.assertThat(unchangedOriginalFile).hasContent("this should be kept");
+  }
+
+  @Test
+  void shouldNotWrapRuntimeExceptions(@TempDirectory.TempDir Path tempDir) throws IOException {
+    Path someFile = tempDir.resolve("something.txt");
+
+    assertThrows(
+      NullPointerException.class,
+      () -> withTemporaryFile(
+        file -> {
+          throw new NullPointerException("test");
+        },
+        someFile));
+  }
+
+  @Test
+  void shouldKeepBackupIfTemporaryFileIsMissing(@TempDirectory.TempDir Path tempDir) throws IOException {
+    Path backedUpFile = tempDir.resolve("notToBeDeleted.txt");
+    new FileOutputStream(backedUpFile.toFile()).write("this should be kept".getBytes());
+
+    assertThrows(
+      StoreException.class,
+      () -> withTemporaryFile(
+        Files::delete,
+        backedUpFile));
+
+    Assertions.assertThat(backedUpFile).hasContent("this should be kept");
+  }
+
+  @Test
+  void shouldDeleteExistingFile(@TempDirectory.TempDir Path tempDir) throws IOException {
+    Path expectedFile = tempDir.resolve("toBeReplaced.txt");
+    new FileOutputStream(expectedFile.toFile()).write("this should be removed".getBytes());
+
+    withTemporaryFile(
+      file -> new FileOutputStream(file.toFile()).write("overwritten".getBytes()),
+      expectedFile);
+
+    Assertions.assertThat(Files.list(tempDir)).hasSize(1);
+  }
+}