Add atomic writes to FileSystem and implement it for LocalFileSystem (#1617)

* Add atomic writes to FileSystem and implement it for LocalFileSystem

* Add header doc

* Cleanup temp file if write/rename fails, verify with test

* "Make" in-memory file system atomic

* Add an option to not delete TemporaryFile on dealloc

* Adapted to Ankit's feedback

* Cleanup previous commit

Author: Honza Dvorsky

Sources/Basic/FileSystem.swift

@@ -166,6 +166,11 @@ public protocol FileSystem: class {
     // FIXME: This is obviously not a very efficient or flexible API.
     func writeFileContents(_ path: AbsolutePath, bytes: ByteString) throws
 
+    /// Write the contents of a file.
+    //
+    // FIXME: This is obviously not a very efficient or flexible API.
+    func writeFileContents(_ path: AbsolutePath, bytes: ByteString, atomically: Bool) throws
+
     /// Recursively deletes the file system entity at `path`.
     ///
     /// If there is no file system entity at `path`, this function does nothing (in particular, this is not considered
@@ -193,6 +198,15 @@ public extension FileSystem {
     func chmod(_ mode: FileMode, path: AbsolutePath) throws {
         try chmod(mode, path: path, options: [])
     }
+    
+    // Unless the file system type provides an override for this method, throw
+    // if `atomically` is `true`, otherwise fall back to whatever implementation already exists.
+    func writeFileContents(_ path: AbsolutePath, bytes: ByteString, atomically: Bool) throws {
+        guard !atomically else {
+            throw FileSystemError.unsupported
+        }
+        try writeFileContents(path, bytes: bytes)
+    }
 
     /// Write to a file from a stream producer.
     func writeFileContents(_ path: AbsolutePath, body: (OutputByteStream) -> Void) throws {
@@ -351,6 +365,24 @@ private class LocalFileSystem: FileSystem {
             break
         }
     }
+    
+    func writeFileContents(_ path: AbsolutePath, bytes: ByteString, atomically: Bool) throws {
+        // Perform non-atomic writes using the fast path.
+        if !atomically {
+            return try writeFileContents(path, bytes: bytes)
+        }
+        let temp = try TemporaryFile(dir: path.parentDirectory, deleteOnClose: false)
+        do {
+            try writeFileContents(temp.path, bytes: bytes)
+            try rename(temp.path, to: path)
+        } catch {
+            // Write or rename failed, delete the temporary file.
+            // Rethrow the original error, however, as that's the
+            // root cause of the failure.
+            _ = try? self.removeFileTree(temp.path)
+            throw error
+        }
+    }
 
     func removeFileTree(_ path: AbsolutePath) throws {
         if self.exists(path, followSymlink: false) {
@@ -681,6 +713,12 @@ public class InMemoryFileSystem: FileSystem {
         // Write the file.
         contents.entries[path.basename] = Node(.file(bytes))
     }
+    
+    public func writeFileContents(_ path: AbsolutePath, bytes: ByteString, atomically: Bool) throws {
+        // In memory file system's writeFileContents is already atomic, so ignore the parameter here
+        // and just call the base implementation.
+        try writeFileContents(path, bytes: bytes)
+    }
 
     public func removeFileTree(_ path: AbsolutePath) throws {
         // Ignore root and get the parent node's content if its a directory.

Tests/BasicTests/FileSystemTests.swift

@@ -13,6 +13,7 @@ import XCTest
 import Basic
 import TestSupport
 import SPMLibc
+import POSIX
 
 class FileSystemTests: XCTestCase {
 
@@ -134,7 +135,23 @@ class FileSystemTests: XCTestCase {
         XCTAssertTrue(fs.isFile(filePath))
         let data = try! fs.readFileContents(filePath)
         XCTAssertEqual(data, ByteString(testData))
-
+        
+        // Atomic writes
+        let inMemoryFilePath = AbsolutePath("/file.text")
+        XCTAssertNoThrow(try Basic.InMemoryFileSystem(files: [:]).writeFileContents(inMemoryFilePath, bytes: ByteString(testData), atomically: true))
+        XCTAssertNoThrow(try Basic.InMemoryFileSystem(files: [:]).writeFileContents(inMemoryFilePath, bytes: ByteString(testData), atomically: false))
+        // Local file system does support atomic writes, so it doesn't throw.
+        let byteString = ByteString(testData)
+        let filePath1 = tmpDir.path.appending(components: "test-data-1.txt")
+        XCTAssertNoThrow(try fs.writeFileContents(filePath1, bytes: byteString, atomically: false))
+        let read1 = try fs.readFileContents(filePath1)
+        XCTAssertEqual(read1, byteString)
+
+        let filePath2 = tmpDir.path.appending(components: "test-data-2.txt")
+        XCTAssertNoThrow(try fs.writeFileContents(filePath2, bytes: byteString, atomically: true))
+        let read2 = try fs.readFileContents(filePath2)
+        XCTAssertEqual(read2, byteString)
+                
         // Check overwrite of a file.
         try! fs.writeFileContents(filePath, bytes: "Hello, new world!")
         XCTAssertEqual(try! fs.readFileContents(filePath), "Hello, new world!")