store lock files in temporary directory (swiftlang#273)

motivation: local file system store lock files next to the file being locked and never cleans them up

changes:
* add a tempDirectory to FileSystem protocol
* update determineTempDirectory to use tempDirectory from localFileSystem
* move logic to handle local file system locks to FileLock and call it from localFileSystem
* expose an API on FileLock to create local file system locks, which by default uses the temp directory to store lock files, using the file-to-lock path and name to construct the lock file name
* move local file system lock logic to use the new FileLock API
* add tests

rdar://86644116

Author: tomerd

Sources/TSCBasic/FileSystem.swift

@@ -6,7 +6,7 @@
 
  See http://swift.org/LICENSE.txt for license information
  See http://swift.org/CONTRIBUTORS.txt for Swift project authors
-*/
+ */
 
 import TSCLibc
 import Foundation
@@ -179,6 +179,9 @@ public protocol FileSystem: AnyObject {
     /// Get the caches directory of current user
     var cachesDirectory: AbsolutePath? { get }
 
+    /// Get the temp directory
+    var tempDirectory: AbsolutePath { get }
+
     /// Create the given directory.
     func createDirectory(_ path: AbsolutePath) throws
 
@@ -351,10 +354,18 @@ private class LocalFileSystem: FileSystem {
         return FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first.flatMap { AbsolutePath($0.path) }
     }
 
+    var tempDirectory: AbsolutePath {
+        let override = ProcessEnv.vars["TMPDIR"] ?? ProcessEnv.vars["TEMP"] ?? ProcessEnv.vars["TMP"]
+        if let path = override.flatMap({ try? AbsolutePath(validating: $0) }) {
+            return path
+        }
+        return AbsolutePath(NSTemporaryDirectory())
+    }
+
     func getDirectoryContents(_ path: AbsolutePath) throws -> [String] {
-      #if canImport(Darwin)
+#if canImport(Darwin)
         return try FileManager.default.contentsOfDirectory(atPath: path.pathString)
-      #else
+#else
         do {
             return try FileManager.default.contentsOfDirectory(atPath: path.pathString)
         } catch let error as NSError {
@@ -366,7 +377,7 @@ private class LocalFileSystem: FileSystem {
             }
             throw error
         }
-      #endif
+#endif
     }
 
     func createDirectory(_ path: AbsolutePath, recursive: Bool) throws {
@@ -477,10 +488,10 @@ private class LocalFileSystem: FileSystem {
         guard isDirectory(path) else { return }
 
         guard let traverse = FileManager.default.enumerator(
-                at: URL(fileURLWithPath: path.pathString),
-                includingPropertiesForKeys: nil) else {
-            throw FileSystemError(.noEntry, path)
-        }
+            at: URL(fileURLWithPath: path.pathString),
+            includingPropertiesForKeys: nil) else {
+                throw FileSystemError(.noEntry, path)
+            }
 
         if !options.contains(.recursive) {
             traverse.skipDescendants()
@@ -506,8 +517,7 @@ private class LocalFileSystem: FileSystem {
     }
 
     func withLock<T>(on path: AbsolutePath, type: FileLock.LockType = .exclusive, _ body: () throws -> T) throws -> T {
-        let lock = FileLock(name: path.basename, cachePath: path.parentDirectory)
-        return try lock.withLock(type: type, body)
+        try FileLock.withLock(fileToLock: path, type: type, body: body)
     }
 }
 
@@ -528,7 +538,7 @@ public class InMemoryFileSystem: FileSystem {
 
         /// Creates deep copy of the object.
         func copy() -> Node {
-           return Node(contents.copy())
+            return Node(contents.copy())
         }
     }
 
@@ -722,6 +732,10 @@ public class InMemoryFileSystem: FileSystem {
         return self.homeDirectory.appending(component: "caches")
     }
 
+    public var tempDirectory: AbsolutePath {
+        return AbsolutePath("/tmp")
+    }
+
     public func getDirectoryContents(_ path: AbsolutePath) throws -> [String] {
         return try lock.withLock {
             guard let node = try getNode(path) else {
@@ -871,9 +885,9 @@ public class InMemoryFileSystem: FileSystem {
             // 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
-            }
+                  case .directory(let contents) = parent.contents else {
+                      return
+                  }
             // Set it to nil to release the contents.
             contents.entries[path.basename] = nil
         }
@@ -935,7 +949,7 @@ public class InMemoryFileSystem: FileSystem {
     public func withLock<T>(on path: AbsolutePath, type: FileLock.LockType = .exclusive, _ body: () throws -> T) throws -> T {
         let resolvedPath: AbsolutePath = try lock.withLock {
             if case let .symlink(destination) = try getNode(path)?.contents {
-                return  AbsolutePath(destination, relativeTo: path.parentDirectory)
+                return AbsolutePath(destination, relativeTo: path.parentDirectory)
             } else {
                 return path
             }
@@ -1028,6 +1042,10 @@ public class RerootedFileSystemView: FileSystem {
         fatalError("cachesDirectory on RerootedFileSystemView is not supported.")
     }
 
+    public var tempDirectory: AbsolutePath {
+        fatalError("tempDirectory on RerootedFileSystemView is not supported.")
+    }
+
     public func getDirectoryContents(_ path: AbsolutePath) throws -> [String] {
         return try underlyingFileSystem.getDirectoryContents(formUnderlyingPath(path))
     }

Sources/TSCBasic/Lock.swift

@@ -44,9 +44,9 @@ extension ProcessLockError: CustomNSError {
         return [NSLocalizedDescriptionKey: "\(self)"]
     }
 }
-/// Provides functionality to aquire a lock on a file via POSIX's flock() method.
+/// Provides functionality to acquire a lock on a file via POSIX's flock() method.
 /// It can be used for things like serializing concurrent mutations on a shared resource
-/// by mutiple instances of a process. The `FileLock` is not thread-safe.
+/// by multiple instances of a process. The `FileLock` is not thread-safe.
 public final class FileLock {
 
     public enum LockType {
@@ -64,15 +64,19 @@ public final class FileLock {
     /// Path to the lock file.
     private let lockFile: AbsolutePath
 
-    /// Create an instance of process lock with a name and the path where
-    /// the lock file can be created.
+    /// Create an instance of FileLock at the path specified
     ///
-    /// Note: The cache path should be a valid directory.
-    public init(name: String, cachePath: AbsolutePath) {
-        self.lockFile = cachePath.appending(component: name + ".lock")
+    /// Note: The parent directory path should be a valid directory.
+    public init(at lockFile: AbsolutePath) {
+        self.lockFile = lockFile
     }
 
-    /// Try to aquire a lock. This method will block until lock the already aquired by other process.
+    @available(*, deprecated, message: "use init(at:) instead")
+    public convenience init(name: String, cachePath: AbsolutePath) {
+        self.init(at: cachePath.appending(component: name + ".lock"))
+    }
+
+    /// Try to acquire a lock. This method will block until lock the already aquired by other process.
     ///
     /// Note: This method can throw if underlying POSIX methods fail.
     public func lock(type: LockType = .exclusive) throws {
@@ -163,4 +167,24 @@ public final class FileLock {
         defer { unlock() }
         return try body()
     }
+    
+    public static func withLock<T>(fileToLock: AbsolutePath, lockFilesDirectory: AbsolutePath? = nil, type: LockType = .exclusive, body: () throws -> T) throws -> T {
+        // unless specified, we use the tempDirectory to store lock files
+        let lockFilesDirectory = lockFilesDirectory ?? localFileSystem.tempDirectory
+        if !localFileSystem.exists(lockFilesDirectory) {
+            throw FileSystemError(.noEntry, lockFilesDirectory)
+        }
+        if !localFileSystem.isDirectory(lockFilesDirectory) {
+            throw FileSystemError(.notDirectory, lockFilesDirectory)
+        }
+        // use the parent path to generate unique filename in temp
+        var lockFileName = (resolveSymlinks(fileToLock.parentDirectory).appending(component: fileToLock.basename)).components.joined(separator: "_")
+        if lockFileName.hasPrefix(AbsolutePath.root.pathString) {
+            lockFileName = String(lockFileName.dropFirst(AbsolutePath.root.pathString.count))
+        }
+        let lockFilePath = lockFilesDirectory.appending(component: lockFileName + ".lock")
+        
+        let lock = FileLock(at: lockFilePath)
+        return try lock.withLock(type: type, body)
+    }
 }

Sources/TSCBasic/TemporaryFile.swift

@@ -53,24 +53,13 @@ private extension TempFileError {
 ///
 /// - Returns: Path to directory in which temporary file should be created.
 public func determineTempDirectory(_ dir: AbsolutePath? = nil) throws -> AbsolutePath {
-    let tmpDir = dir ?? cachedTempDirectory
+    let tmpDir = dir ?? localFileSystem.tempDirectory
     guard localFileSystem.isDirectory(tmpDir) else {
         throw TempFileError.couldNotFindTmpDir(tmpDir.pathString)
     }
     return tmpDir
 }
 
-/// Returns temporary directory location by searching relevant env variables.
-///
-/// Evaluates once per execution.
-private var cachedTempDirectory: AbsolutePath = {
-    let override = ProcessEnv.vars["TMPDIR"] ?? ProcessEnv.vars["TEMP"] ?? ProcessEnv.vars["TMP"]
-    if let path = override.flatMap({ try? AbsolutePath(validating: $0) }) {
-        return path
-    }
-    return AbsolutePath(NSTemporaryDirectory())
-}()
-
 /// The closure argument of the `body` closue of `withTemporaryFile`.
 public struct TemporaryFile {
     /// If specified during init, the temporary file name begins with this prefix.

Tests/TSCBasicTests/LockTests.swift

@@ -38,7 +38,7 @@ class LockTests: XCTestCase {
                 let N = 10
                 let threads = (1...N).map { idx in
                     return Thread {
-                        let lock = FileLock(name: "TestLock", cachePath: tempDirPath)
+                        let lock = FileLock(at: tempDirPath.appending(component: "TestLock"))
                         try! lock.withLock {
                             // Get thr current contents of the file if any.
                             let currentData: Int
@@ -71,7 +71,7 @@ class LockTests: XCTestCase {
 
             let writerThreads = (0..<100).map { _ in
                 return Thread {
-                    let lock = FileLock(name: "foo", cachePath: tempDir)
+                    let lock = FileLock(at: tempDir.appending(component: "foo"))
                     try! lock.withLock(type: .exclusive) {
                         // Get the current contents of the file if any.
                         let valueA = Int(try localFileSystem.readFileContents(fileA).description)!
@@ -90,7 +90,7 @@ class LockTests: XCTestCase {
 
             let readerThreads = (0..<20).map { _ in
                 return Thread {
-                    let lock = FileLock(name: "foo", cachePath: tempDir)
+                    let lock = FileLock(at: tempDir.appending(component: "foo"))
                     try! lock.withLock(type: .shared) {
                         try XCTAssertEqual(localFileSystem.readFileContents(fileA), localFileSystem.readFileContents(fileB))
 
@@ -110,5 +110,61 @@ class LockTests: XCTestCase {
             try XCTAssertEqual(localFileSystem.readFileContents(fileB), "100")
         }
     }
-
+    
+    func testFileLockLocation() throws {
+        do {
+            let fileName = UUID().uuidString
+            let fileToLock = localFileSystem.homeDirectory.appending(component: fileName)
+            try localFileSystem.withLock(on: fileToLock, type: .exclusive) {}
+            
+            // lock file expected at temp when lockFilesDirectory set to nil
+            // which is the case when going through localFileSystem
+            let lockFile = try localFileSystem.getDirectoryContents(localFileSystem.tempDirectory)
+                .first(where: { $0.contains(fileName) })
+            XCTAssertNotNil(lockFile, "expected lock file at \(localFileSystem.tempDirectory)")
+        }
+        
+        do {
+            let fileName = UUID().uuidString
+            let fileToLock = localFileSystem.homeDirectory.appending(component: fileName)
+            try FileLock.withLock(fileToLock: fileToLock, lockFilesDirectory: nil, body: {})
+            
+            // lock file expected at temp when lockFilesDirectory set to nil
+            let lockFile = try localFileSystem.getDirectoryContents(localFileSystem.tempDirectory)
+                .first(where: { $0.contains(fileName) })
+            XCTAssertNotNil(lockFile, "expected lock file at \(localFileSystem.tempDirectory)")
+        }
+        
+        do {
+            try withTemporaryDirectory { tempDir in
+                let fileName = UUID().uuidString
+                let fileToLock = localFileSystem.homeDirectory.appending(component: fileName)
+                try FileLock.withLock(fileToLock: fileToLock, lockFilesDirectory: tempDir, body: {})
+                
+                // lock file expected at specified directory when lockFilesDirectory is set to a valid directory
+                let lockFile = try localFileSystem.getDirectoryContents(tempDir)
+                    .first(where: { $0.contains(fileName) })
+                XCTAssertNotNil(lockFile, "expected lock file at \(tempDir)")
+            }
+        }
+        
+        do {
+            let fileName = UUID().uuidString
+            let fileToLock = localFileSystem.homeDirectory.appending(component: fileName)
+            let lockFilesDirectory = localFileSystem.homeDirectory.appending(component: UUID().uuidString)
+            XCTAssertThrows(FileSystemError(.noEntry, lockFilesDirectory)) {
+                try FileLock.withLock(fileToLock: fileToLock, lockFilesDirectory: lockFilesDirectory, body: {})
+            }
+        }
+        
+        do {
+            let fileName = UUID().uuidString
+            let fileToLock = localFileSystem.homeDirectory.appending(component: fileName)
+            let lockFilesDirectory = localFileSystem.homeDirectory.appending(component: UUID().uuidString)
+            try localFileSystem.writeFileContents(lockFilesDirectory, bytes: [])
+            XCTAssertThrows(FileSystemError(.notDirectory, lockFilesDirectory)) {
+                try FileLock.withLock(fileToLock: fileToLock, lockFilesDirectory: lockFilesDirectory, body: {})
+            }
+        }
+    }
 }