Merge pull request #163 from apple/make-inmemoryfilesystem-threadsafe

Makes InMemoryFileSystem threadsafe to address flaky unit tests

Author: abertelrud

Sources/TSCBasic/FileSystem.swift

@@ -488,6 +488,9 @@ private class LocalFileSystem: FileSystem {
 //
 /// Concrete FileSystem implementation which simulates an empty disk.
 public class InMemoryFileSystem: FileSystem {
+    
+    /// Private internal representation of a file system node.
+    /// Not threadsafe.
     private class Node {
         /// The actual node data.
         let contents: NodeContents
@@ -501,6 +504,9 @@ public class InMemoryFileSystem: FileSystem {
            return Node(contents.copy())
         }
     }
+
+    /// Private internal representation the contents of a file system node.
+    /// Not threadsafe.
     private enum NodeContents {
         case file(ByteString)
         case directory(DirectoryContents)
@@ -518,6 +524,9 @@ public class InMemoryFileSystem: FileSystem {
             }
         }
     }
+    
+    /// Private internal representation the contents of a directory.
+    /// Not threadsafe.
     private class DirectoryContents {
         var entries: [String: Node]
 
@@ -535,9 +544,16 @@ public class InMemoryFileSystem: FileSystem {
         }
     }
 
-    /// The root filesytem.
+    /// The root node of the filesytem.
     private var root: Node
-    /// Used to access lockFiles in a thread safe manner.
+    
+    /// Protects `root` and everything underneath it.
+    /// FIXME: Using a single lock for this is a performance problem, but in
+    /// reality, the only practical use for InMemoryFileSystem is for unit
+    /// tests.
+    private let lock = Lock()
+    
+    /// Exclusive file system lock vended to clients through `withLock()`.
     private let lockFilesLock = Lock()
 
     public init() {
@@ -546,12 +562,15 @@ public class InMemoryFileSystem: FileSystem {
 
     /// Creates deep copy of the object.
     public func copy() -> InMemoryFileSystem {
-        let fs = InMemoryFileSystem()
-        fs.root = root.copy()
-        return fs
+        return lock.withLock {
+            let fs = InMemoryFileSystem()
+            fs.root = root.copy()
+            return fs
+        }
     }
 
-    /// Get the node corresponding to the given path.
+    /// Private function to look up the node corresponding to a path.
+    /// Not threadsafe.
     private func getNode(_ path: AbsolutePath, followSymlink: Bool = true) throws -> Node? {
         func getNodeInternal(_ path: AbsolutePath) throws -> Node? {
             // If this is the root node, return it.
@@ -590,46 +609,54 @@ public class InMemoryFileSystem: FileSystem {
     // MARK: FileSystem Implementation
 
     public func exists(_ path: AbsolutePath, followSymlink: Bool) -> Bool {
-        do {
-            switch try getNode(path, followSymlink: followSymlink)?.contents {
-            case .file, .directory, .symlink: return true
-            case .none: return false
+        return lock.withLock {
+            do {
+                switch try getNode(path, followSymlink: followSymlink)?.contents {
+                case .file, .directory, .symlink: return true
+                case .none: return false
+                }
+            } catch {
+                return false
             }
-        } catch {
-            return false
         }
     }
 
     public func isDirectory(_ path: AbsolutePath) -> Bool {
-        do {
-            if case .directory? = try getNode(path)?.contents {
-                return true
+        return lock.withLock {
+            do {
+                if case .directory? = try getNode(path)?.contents {
+                    return true
+                }
+                return false
+            } catch {
+                return false
             }
-            return false
-        } catch {
-            return false
         }
     }
 
     public func isFile(_ path: AbsolutePath) -> Bool {
-        do {
-            if case .file? = try getNode(path)?.contents {
-                return true
+        return lock.withLock {
+            do {
+                if case .file? = try getNode(path)?.contents {
+                    return true
+                }
+                return false
+            } catch {
+                return false
             }
-            return false
-        } catch {
-            return false
         }
     }
 
     public func isSymlink(_ path: AbsolutePath) -> Bool {
-        do {
-            if case .symlink? = try getNode(path, followSymlink: false)?.contents {
-                return true
+        return lock.withLock {
+            do {
+                if case .symlink? = try getNode(path, followSymlink: false)?.contents {
+                    return true
+                }
+                return false
+            } catch {
+                return false
             }
-            return false
-        } catch {
-            return false
         }
     }
 
@@ -658,18 +685,21 @@ public class InMemoryFileSystem: FileSystem {
     }
 
     public func getDirectoryContents(_ path: AbsolutePath) throws -> [String] {
-        guard let node = try getNode(path) else {
-            throw FileSystemError.noEntry
-        }
-        guard case .directory(let contents) = node.contents else {
-            throw FileSystemError.notDirectory
-        }
+        return try lock.withLock {
+            guard let node = try getNode(path) else {
+                throw FileSystemError.noEntry
+            }
+            guard case .directory(let contents) = node.contents else {
+                throw FileSystemError.notDirectory
+            }
 
-        // FIXME: Perhaps we should change the protocol to allow lazy behavior.
-        return [String](contents.entries.keys)
+            // FIXME: Perhaps we should change the protocol to allow lazy behavior.
+            return [String](contents.entries.keys)
+        }
     }
-
-    public func createDirectory(_ path: AbsolutePath, recursive: Bool) throws {
+    
+    /// Not threadsafe.
+    private func _createDirectory(_ path: AbsolutePath, recursive: Bool) throws {
         // Ignore if client passes root.
         guard !path.isRoot else {
             return
@@ -681,10 +711,10 @@ public class InMemoryFileSystem: FileSystem {
             // to create the parent and retry.
             if recursive && path != parentPath {
                 // Attempt to create the parent.
-                try createDirectory(parentPath, recursive: true)
+                try _createDirectory(parentPath, recursive: true)
 
                 // Re-attempt creation, non-recursively.
-                return try createDirectory(path, recursive: false)
+                return try _createDirectory(path, recursive: false)
             } else {
                 // Otherwise, we failed.
                 throw FileSystemError.noEntry
@@ -713,71 +743,83 @@ public class InMemoryFileSystem: FileSystem {
         contents.entries[path.basename] = Node(.directory(DirectoryContents()))
     }
 
-    public func createSymbolicLink(_ path: AbsolutePath, pointingAt destination: AbsolutePath, relative: Bool) throws {
-        // Create directory to destination parent.
-        guard let destinationParent = try getNode(path.parentDirectory) else {
-            throw FileSystemError.noEntry
+    public func createDirectory(_ path: AbsolutePath, recursive: Bool) throws {
+        return try lock.withLock {
+            try _createDirectory(path, recursive: recursive)
         }
+    }
 
-        // Check that the parent is a directory.
-        guard case .directory(let contents) = destinationParent.contents else {
-            throw FileSystemError.notDirectory
-        }
+    public func createSymbolicLink(_ path: AbsolutePath, pointingAt destination: AbsolutePath, relative: Bool) throws {
+        return try lock.withLock {
+            // Create directory to destination parent.
+            guard let destinationParent = try getNode(path.parentDirectory) else {
+                throw FileSystemError.noEntry
+            }
 
-        guard contents.entries[path.basename] == nil else {
-            throw FileSystemError.alreadyExistsAtDestination
-        }
+            // Check that the parent is a directory.
+            guard case .directory(let contents) = destinationParent.contents else {
+                throw FileSystemError.notDirectory
+            }
+
+            guard contents.entries[path.basename] == nil else {
+                throw FileSystemError.alreadyExistsAtDestination
+            }
 
-        let destination = relative ? destination.relative(to: path.parentDirectory).pathString : destination.pathString
+            let destination = relative ? destination.relative(to: path.parentDirectory).pathString : destination.pathString
 
-        contents.entries[path.basename] = Node(.symlink(destination))
+            contents.entries[path.basename] = Node(.symlink(destination))
+        }
     }
 
     public func readFileContents(_ path: AbsolutePath) throws -> ByteString {
-        // Get the node.
-        guard let node = try getNode(path) else {
-            throw FileSystemError.noEntry
-        }
+        return try lock.withLock {
+            // Get the node.
+            guard let node = try getNode(path) else {
+                throw FileSystemError.noEntry
+            }
 
-        // Check that the node is a file.
-        guard case .file(let contents) = node.contents else {
-            // The path is a directory, this is an error.
-            throw FileSystemError.isDirectory
-        }
+            // Check that the node is a file.
+            guard case .file(let contents) = node.contents else {
+                // The path is a directory, this is an error.
+                throw FileSystemError.isDirectory
+            }
 
-        // Return the file contents.
-        return contents
+            // Return the file contents.
+            return contents
+        }
     }
 
     public func writeFileContents(_ path: AbsolutePath, bytes: ByteString) throws {
-        // It is an error if this is the root node.
-        let parentPath = path.parentDirectory
-        guard path != parentPath else {
-            throw FileSystemError.isDirectory
-        }
+        return try lock.withLock {
+            // It is an error if this is the root node.
+            let parentPath = path.parentDirectory
+            guard path != parentPath else {
+                throw FileSystemError.isDirectory
+            }
 
-        // Get the parent node.
-        guard let parent = try getNode(parentPath) else {
-            throw FileSystemError.noEntry
-        }
+            // Get the parent node.
+            guard let parent = try getNode(parentPath) else {
+                throw FileSystemError.noEntry
+            }
 
-        // Check that the parent is a directory.
-        guard case .directory(let contents) = parent.contents else {
-            // The parent isn't a directory, this is an error.
-            throw FileSystemError.notDirectory
-        }
+            // Check that the parent is a directory.
+            guard case .directory(let contents) = parent.contents else {
+                // The parent isn't a directory, this is an error.
+                throw FileSystemError.notDirectory
+            }
 
-        // Check if the node exists.
-        if let node = contents.entries[path.basename] {
-            // Verify it is a file.
-            guard case .file = node.contents else {
-                // The path is a directory, this is an error.
-                throw FileSystemError.isDirectory
+            // Check if the node exists.
+            if let node = contents.entries[path.basename] {
+                // Verify it is a file.
+                guard case .file = node.contents else {
+                    // The path is a directory, this is an error.
+                    throw FileSystemError.isDirectory
+                }
             }
-        }
 
-        // Write the file.
-        contents.entries[path.basename] = Node(.file(bytes))
+            // Write the file.
+            contents.entries[path.basename] = Node(.file(bytes))
+        }
     }
 
     public func writeFileContents(_ path: AbsolutePath, bytes: ByteString, atomically: Bool) throws {
@@ -787,21 +829,25 @@ public class InMemoryFileSystem: FileSystem {
     }
 
     public func removeFileTree(_ path: AbsolutePath) throws {
-        // Ignore root and get the parent node's content if its a directory.
-        guard !path.isRoot,
-              let parent = try? getNode(path.parentDirectory),
-            case .directory(let contents) = parent.contents else {
-            return
+        return lock.withLock {
+            // Ignore root and get the parent node's content if its a directory.
+            guard !path.isRoot,
+                  let parent = try? getNode(path.parentDirectory),
+                case .directory(let contents) = parent.contents else {
+                return
+            }
+            // Set it to nil to release the contents.
+            contents.entries[path.basename] = nil
         }
-        // Set it to nil to release the contents.
-        contents.entries[path.basename] = nil
     }
 
     public func chmod(_ mode: FileMode, path: AbsolutePath, options: Set<FileMode.Option>) throws {
         // FIXME: We don't have these semantics in InMemoryFileSystem.
     }
-
-    public func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws {
+    
+    /// Private implementation of core copying function.
+    /// Not threadsafe.
+    private func _copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws {
         // Get the source node.
         guard let source = try getNode(sourcePath) else {
             throw FileSystemError.noEntry
@@ -824,20 +870,28 @@ public class InMemoryFileSystem: FileSystem {
         contents.entries[destinationPath.basename] = source
     }
 
-    public func move(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws {
-        // Get the source parent node.
-        guard let sourceParent = try getNode(sourcePath.parentDirectory) else {
-            throw FileSystemError.noEntry
+    public func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws {
+        return try lock.withLock {
+            try _copy(from: sourcePath, to: destinationPath)
         }
+    }
 
-        // Check that the parent is a directory.
-        guard case .directory(let contents) = sourceParent.contents else {
-            throw FileSystemError.notDirectory
-        }
+    public func move(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws {
+        return try lock.withLock {
+            // Get the source parent node.
+            guard let sourceParent = try getNode(sourcePath.parentDirectory) else {
+                throw FileSystemError.noEntry
+            }
+
+            // Check that the parent is a directory.
+            guard case .directory(let contents) = sourceParent.contents else {
+                throw FileSystemError.notDirectory
+            }
 
-        try copy(from: sourcePath, to: destinationPath)
+            try _copy(from: sourcePath, to: destinationPath)
 
-        contents.entries[sourcePath.basename] = nil
+            contents.entries[sourcePath.basename] = nil
+        }
     }
 
     public func withLock<T>(on path: AbsolutePath, type: FileLock.LockType = .exclusive, _ body: () throws -> T) throws -> T {