From a1cbeeb076dbd61076b5e85a96b2160aadc74f92 Mon Sep 17 00:00:00 2001 From: Michael Ilseman Date: Tue, 7 Mar 2023 17:04:04 -0700 Subject: [PATCH] lock and lock(wait:) --- Sources/System/FileControl.swift | 2 +- Sources/System/FileControlRaw.swift | 41 +------ Sources/System/FileLock.swift | 126 +++++++++++---------- Sources/System/Internals/CInterop.swift | 2 +- Sources/System/Util.swift | 35 +----- Tests/SystemTests/FileLockTest.swift | 50 ++++---- Tests/SystemTests/FileOperationsTest.swift | 21 ---- 7 files changed, 105 insertions(+), 172 deletions(-) diff --git a/Sources/System/FileControl.swift b/Sources/System/FileControl.swift index 6b5e050c..eefb5f4b 100644 --- a/Sources/System/FileControl.swift +++ b/Sources/System/FileControl.swift @@ -12,7 +12,7 @@ extension FileDescriptor { internal func _fcntl( - _ cmd: Control.Command, _ lock: inout FileDescriptor.FileLock, + _ cmd: Command, _ lock: inout FileDescriptor.FileLock, retryOnInterrupt: Bool ) -> Result<(), Errno> { nothingOrErrno(retryOnInterrupt: retryOnInterrupt) { diff --git a/Sources/System/FileControlRaw.swift b/Sources/System/FileControlRaw.swift index 61f134ba..79eedf02 100644 --- a/Sources/System/FileControlRaw.swift +++ b/Sources/System/FileControlRaw.swift @@ -20,75 +20,40 @@ import Glibc #if !os(Windows) -extension FileDescriptor { - /// A namespace for types and values for `FileDescriptor.control()`, aka `fcntl`. - /// - /// TODO: a better name? "Internals", "Raw", "FCNTL"? I feel like a - /// precedent would be useful for sysctl, ioctl, and other grab-bag - /// things. "junk drawer" can be an anti-pattern, but is better than - /// trashing the higher namespace. - // public - internal enum Control {} - -} // - MARK: Commands -extension FileDescriptor.Control { +// TODO: Make below API as part of broader `fcntl` support. +extension FileDescriptor { /// Commands (and various constants) to pass to `fcntl`. - // @frozen - // public internal struct Command: RawRepresentable, Hashable { -// @_alwaysEmitIntoClient -// public internal let rawValue: CInt -// @_alwaysEmitIntoClient -// public internal init(rawValue: CInt) { self.rawValue = rawValue } @_alwaysEmitIntoClient private init(_ raw: CInt) { self.init(rawValue: raw) } + /// Get open file description record locking information. /// - /// TODO: link to https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html - /// TODO: reference FileDesciptor.isLocked() or something like that - /// /// The corresponding C constant is `F_GETLK`. -// @_alwaysEmitIntoClient -// public internal static var getOFDLock: Command { Command(_F_OFD_GETLK) } /// Set open file description record locking information. /// - /// TODO: link to https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html - /// TODO: reference FileDesciptor.lock() - /// /// The corresponding C constant is `F_SETLK`. -// @_alwaysEmitIntoClient -// public internal static var setOFDLock: Command { Command(_F_OFD_SETLK) } /// Set open file description record locking information and wait until /// the request can be completed. /// - /// TODO: link to https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html - /// TODO: reference FileDesciptor.lock() - /// /// The corresponding C constant is `F_SETLKW`. -// @_alwaysEmitIntoClient -// public internal static var setOFDLockWait: Command { Command(_F_OFD_SETLKW) } #if !os(Linux) /// Set open file description record locking information and wait until /// the request can be completed, returning on timeout. /// - /// TODO: link to https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html - /// TODO: reference FileDesciptor.lock() - /// /// The corresponding C constant is `F_SETLKWTIMEOUT`. -// @_alwaysEmitIntoClient -// public internal static var setOFDLockWaitTimout: Command { Command(_F_OFD_SETLKWTIMEOUT) } diff --git a/Sources/System/FileLock.swift b/Sources/System/FileLock.swift index 3c245bdf..4119e40a 100644 --- a/Sources/System/FileLock.swift +++ b/Sources/System/FileLock.swift @@ -123,6 +123,18 @@ extension FileDescriptor.FileLock { public static var none: Self { Self(rawValue: CInterop.CShort(truncatingIfNeeded: F_UNLCK)) } + + /// Shared (alias for `read`) + @_alwaysEmitIntoClient + public static var shared: Self { .read } + + /// Exclusive (alias for `write`) + @_alwaysEmitIntoClient + public static var exclusive: Self { .write } + + /// Unlock (alias for `none`) + @_alwaysEmitIntoClient + public static var unlock: Self { .none } } } @@ -131,7 +143,8 @@ extension FileDescriptor { /// /// If the open file description already has a lock, the old lock is /// replaced. If the lock cannot be set because it is blocked by an existing lock, - /// this will wait until the lock can be set. + /// that is if the syscall would throw `.resourceTemporarilyUnavailable` + /// (aka `EAGAIN`), this will return `false`. /// /// Open file description locks are associated with an open file /// description (see `FileDescriptor.open`). Duplicated @@ -156,29 +169,31 @@ extension FileDescriptor { /// - retryOnInterrupt: Whether to retry the operation if it throws /// ``Errno/interrupted``. The default is `true`. Pass `false` to try /// only once and throw an error upon interruption. + /// - Returns: `true` if the lock was aquired, `false` otherwise /// - /// The corresponding C function is `fcntl` with `F_OFD_SETLKW`. + /// The corresponding C function is `fcntl` with `F_OFD_SETLK`. @_alwaysEmitIntoClient public func lock( _ kind: FileDescriptor.FileLock.Kind = .read, byteRange: (some RangeExpression)? = Range?.none, retryOnInterrupt: Bool = true - ) throws { + ) throws -> Bool { let (start, len) = _mapByteRangeToByteOffsets(byteRange) - try _lock( + return try _lock( kind, start: start, length: len, + wait: false, + waitUntilTimeout: false, retryOnInterrupt: retryOnInterrupt - ).get() + )?.get() != nil } - /// Try to set an advisory open file description lock. + /// Set an advisory open file description lock. /// /// If the open file description already has a lock, the old lock is - /// replaced. If the lock cannot be set because it is blocked by an existing lock, - /// that is if the syscall would throw `.resourceTemporarilyUnavailable` - /// (aka `EAGAIN`), this will return `false`. + /// replaced. If the lock cannot be set because it is blocked by an existing lock and + /// `wait` is true, this will wait until the lock can be set, otherwise returns `false`. /// /// Open file description locks are associated with an open file /// description (see `FileDescriptor.open`). Duplicated @@ -200,38 +215,38 @@ extension FileDescriptor { /// - kind: The kind of lock to set /// - byteRange: The range of bytes over which to lock. Pass /// `nil` to consider the entire file. + /// - wait: if `true` will wait until the lock can be set /// - retryOnInterrupt: Whether to retry the operation if it throws /// ``Errno/interrupted``. The default is `true`. Pass `false` to try /// only once and throw an error upon interruption. - /// - Returns: `true` if the lock was aquired, `false` otherwise /// - /// The corresponding C function is `fcntl` with `F_OFD_SETLK`. + /// The corresponding C function is `fcntl` with `F_OFD_SETLK` or `F_OFD_SETLKW`. + @discardableResult @_alwaysEmitIntoClient - public func tryLock( + public func lock( _ kind: FileDescriptor.FileLock.Kind = .read, byteRange: (some RangeExpression)? = Range?.none, + wait: Bool, retryOnInterrupt: Bool = true ) throws -> Bool { let (start, len) = _mapByteRangeToByteOffsets(byteRange) - guard let _ = try _tryLock( + return try _lock( kind, - waitUntilTimeout: false, start: start, length: len, + wait: wait, + waitUntilTimeout: false, retryOnInterrupt: retryOnInterrupt - )?.get() else { - return false - } - return true + )?.get() != nil } - #if !os(Linux) - /// Try to set an advisory open file description lock. +#if !os(Linux) + /// Set an advisory open file description lock. /// /// If the open file description already has a lock, the old lock is - /// replaced. If the lock cannot be set because it is blocked by an existing lock, - /// that is if the syscall would throw `.resourceTemporarilyUnavailable` - /// (aka `EAGAIN`), this will return `false`. + /// replaced. If the lock cannot be set because it is blocked by an existing lock and + /// `waitUntilTimeout` is true, this will wait until the lock can be set (or the operating + /// system's timeout expires), otherwise returns `false`. /// /// Open file description locks are associated with an open file /// description (see `FileDescriptor.open`). Duplicated @@ -253,33 +268,30 @@ extension FileDescriptor { /// - kind: The kind of lock to set /// - byteRange: The range of bytes over which to lock. Pass /// `nil` to consider the entire file. - /// - waitUntilTimeout: If `true`, will wait until a timeout (determined by the operating system) + /// - waitUntilTimeout: if `true` will wait until the lock can be set or a timeout expires /// - retryOnInterrupt: Whether to retry the operation if it throws /// ``Errno/interrupted``. The default is `true`. Pass `false` to try /// only once and throw an error upon interruption. - /// - Returns: `true` if the lock was aquired, `false` otherwise /// - /// The corresponding C function is `fcntl` with `F_OFD_SETLK` or `F_OFD_SETLKWTIMEOUT` . + /// The corresponding C function is `fcntl` with `F_OFD_SETLK` or `F_SETLKWTIMEOUT`. @_alwaysEmitIntoClient - public func tryLock( + public func lock( _ kind: FileDescriptor.FileLock.Kind = .read, byteRange: (some RangeExpression)? = Range?.none, waitUntilTimeout: Bool, retryOnInterrupt: Bool = true ) throws -> Bool { let (start, len) = _mapByteRangeToByteOffsets(byteRange) - guard let _ = try _tryLock( + return try _lock( kind, - waitUntilTimeout: waitUntilTimeout, start: start, length: len, + wait: false, + waitUntilTimeout: waitUntilTimeout, retryOnInterrupt: retryOnInterrupt - )?.get() else { - return false - } - return true + )?.get() != nil } - #endif +#endif /// Remove an open file description lock. /// @@ -311,49 +323,48 @@ extension FileDescriptor { @_alwaysEmitIntoClient public func unlock( byteRange: (some RangeExpression)? = Range?.none, - wait: Bool = false, // FIXME: needed? retryOnInterrupt: Bool = true ) throws { let (start, len) = _mapByteRangeToByteOffsets(byteRange) - guard let res = _tryLock( + guard try _lock( .none, - waitUntilTimeout: false, // TODO: or we wait for timeout? start: start, length: len, + wait: false, + waitUntilTimeout: false, retryOnInterrupt: retryOnInterrupt - ) else { - preconditionFailure("TODO: Unlock should always succeed?") + )?.get() != nil else { + // NOTE: Errno and syscall composition wasn't designed for the modern + // world. Releasing locks should always succeed and never be blocked + // by an existing lock held elsewhere. But there's always a chance + // that some effect (e.g. from NFS) causes `EGAIN` to be thrown for a + // different reason/purpose. Here, in the very unlikely situation + // that we somehow saw it, we convert the `nil` back to the error. + throw Errno.resourceTemporarilyUnavailable } - return try res.get() } + /// Internal lock entry point, returns `nil` if blocked by existing lock. + /// Both `wait` and `waitUntilTimeout` cannot both be true (passed as bools to avoid + /// spurious enum in the ABI). @usableFromInline internal func _lock( _ kind: FileDescriptor.FileLock.Kind, start: Int64, length: Int64, - retryOnInterrupt: Bool - ) -> Result<(), Errno> { - var lock = FileDescriptor.FileLock( - ofdType: kind, start: start, length: length) - return _fcntl(.setOFDLockWait, &lock, retryOnInterrupt: retryOnInterrupt) - } - - @usableFromInline - internal func _tryLock( - _ kind: FileDescriptor.FileLock.Kind, + wait: Bool, waitUntilTimeout: Bool, - start: Int64, - length: Int64, retryOnInterrupt: Bool ) -> Result<(), Errno>? { + precondition(!wait || !waitUntilTimeout) + let cmd: FileDescriptor.Command + if waitUntilTimeout { #if os(Linux) - precondition(!waitUntilTimeout, "`waitUntilTimeout` unavailable on Linux") + preconditionFailure("`waitUntilTimeout` unavailable on Linux") #endif - - let cmd: Control.Command - if waitUntilTimeout { cmd = .setOFDLockWaitTimout + } else if wait { + cmd = .setOFDLockWait } else { cmd = .setOFDLock } @@ -363,5 +374,6 @@ extension FileDescriptor { _fcntl(cmd, &lock, retryOnInterrupt: retryOnInterrupt)) } } -#endif + +#endif // !os(Windows) diff --git a/Sources/System/Internals/CInterop.swift b/Sources/System/Internals/CInterop.swift index 8f4b7119..0f7d8dca 100644 --- a/Sources/System/Internals/CInterop.swift +++ b/Sources/System/Internals/CInterop.swift @@ -70,7 +70,6 @@ public enum CInterop { #if !os(Windows) /// The C `struct flock` type public typealias FileLock = flock -#endif /// The C `pid_t` type public typealias PID = pid_t @@ -81,6 +80,7 @@ public enum CInterop { /// might otherwise appear. This typealias allows conversion code to be /// emitted into client. public typealias Offset = off_t +#endif } diff --git a/Sources/System/Util.swift b/Sources/System/Util.swift index a4782f10..56f4344e 100644 --- a/Sources/System/Util.swift +++ b/Sources/System/Util.swift @@ -43,7 +43,7 @@ internal func nothingOrErrno( valueOrErrno(retryOnInterrupt: retryOnInterrupt, f).map { _ in () } } -/// Promote `Errno.wouldBlcok` to `nil`. +/// Promote `Errno.wouldBlock` / `Errno.resourceTemporarilyUnavailable` to `nil`. internal func _extractWouldBlock( _ value: Result ) -> Result? { @@ -166,32 +166,9 @@ internal func _mapByteRangeToByteOffsets( let start = br.lowerBound == Int64.min ? 0 : br.lowerBound - let len: Int64 = { - if br.upperBound == Int64.max { - // l_len == 0 means until end of file - return 0 - } - return br.upperBound - start - }() - return (start, len) - - /* - - let len: Int64 = { - if byteRange.upperBound == Int64.max { - // l_len == 0 means until end of file - 0 - } else { - byteRange.upperBound - start - } - }() - - let len = if byteRange.upperbound == Int64.max { - // l_len == 0 means until end of file - 0 - } else { - byteRange.upperBound - start - } - - */ + if br.upperBound == Int64.max { + // l_len == 0 means until end of file + return (start, 0) + } + return (start, br.upperBound - start) } diff --git a/Tests/SystemTests/FileLockTest.swift b/Tests/SystemTests/FileLockTest.swift index 0abff10d..35e4ef75 100644 --- a/Tests/SystemTests/FileLockTest.swift +++ b/Tests/SystemTests/FileLockTest.swift @@ -32,26 +32,26 @@ extension FileOperationsTest { let dup_B = try ofd_B.duplicate() // A(read) -> A(write) -> FAIL: B(read/write) - XCTAssertTrue(try ofd_A.tryLock(.read)) - XCTAssertTrue(try ofd_A.tryLock(.write)) - XCTAssertTrue(try dup_A.tryLock(.write)) // redundant, but works - XCTAssertFalse(try ofd_B.tryLock(.read)) - XCTAssertFalse(try ofd_B.tryLock(.write)) - XCTAssertFalse(try dup_B.tryLock(.write)) + XCTAssertTrue(try ofd_A.lock(.read)) + XCTAssertTrue(try ofd_A.lock(.write)) + XCTAssertTrue(try dup_A.lock(.write)) // redundant, but works + XCTAssertFalse(try ofd_B.lock(.read)) + XCTAssertFalse(try ofd_B.lock(.write)) + XCTAssertFalse(try dup_B.lock(.write)) try dup_A.unlock() // A(read) -> B(read) -> FAIL: A/B(write) // -> B(unlock) -> A(write) -> FAIL: B(read/write) - XCTAssertTrue(try dup_A.tryLock(.read)) - XCTAssertTrue(try ofd_B.tryLock(.read)) - XCTAssertFalse(try ofd_A.tryLock(.write)) - XCTAssertFalse(try dup_A.tryLock(.write)) - XCTAssertFalse(try ofd_B.tryLock(.write)) - XCTAssertFalse(try dup_B.tryLock(.write)) + XCTAssertTrue(try dup_A.lock(.read)) + XCTAssertTrue(try ofd_B.lock(.read)) + XCTAssertFalse(try ofd_A.lock(.write)) + XCTAssertFalse(try dup_A.lock(.write)) + XCTAssertFalse(try ofd_B.lock(.write)) + XCTAssertFalse(try dup_B.lock(.write)) try dup_B.unlock() - XCTAssertTrue(try ofd_A.tryLock(.write)) - XCTAssertFalse(try dup_B.tryLock(.read)) - XCTAssertFalse(try ofd_B.tryLock(.write)) + XCTAssertTrue(try ofd_A.lock(.write)) + XCTAssertFalse(try dup_B.lock(.read)) + XCTAssertFalse(try ofd_B.lock(.write)) try dup_A.unlock() /// Byte ranges @@ -61,17 +61,17 @@ extension FileOperationsTest { // -> FAIL: B(read, 17..<18), A(read 60..<70) // -> A(unlock, 11..<12) -> B(read, 11..<12) -> A(read, 11..<12) // -> FAIL A/B(write, 11..<12) - XCTAssertTrue(try ofd_A.tryLock(.read, byteRange: ..<50)) - XCTAssertTrue(try ofd_B.tryLock(.write, byteRange: 50...)) - XCTAssertTrue(try ofd_A.tryLock(.write, byteRange: 10..<20)) - XCTAssertTrue(try ofd_B.tryLock(.read, byteRange: 40..<50)) - XCTAssertFalse(try ofd_B.tryLock(.read, byteRange: 17..<18)) - XCTAssertFalse(try ofd_A.tryLock(.read, byteRange: 60..<70)) + XCTAssertTrue(try ofd_A.lock(.read, byteRange: ..<50)) + XCTAssertTrue(try ofd_B.lock(.write, byteRange: 50...)) + XCTAssertTrue(try ofd_A.lock(.write, byteRange: 10..<20)) + XCTAssertTrue(try ofd_B.lock(.read, byteRange: 40..<50)) + XCTAssertFalse(try ofd_B.lock(.read, byteRange: 17..<18)) + XCTAssertFalse(try ofd_A.lock(.read, byteRange: 60..<70)) try dup_A.unlock(byteRange: 11..<12) - XCTAssertTrue(try ofd_B.tryLock(.read, byteRange: 11..<12)) - XCTAssertTrue(try ofd_A.tryLock(.read, byteRange: 11..<12)) - XCTAssertFalse(try ofd_B.tryLock(.write, byteRange: 11..<12)) - XCTAssertFalse(try ofd_A.tryLock(.write, byteRange: 11..<12)) + XCTAssertTrue(try ofd_B.lock(.read, byteRange: 11..<12)) + XCTAssertTrue(try ofd_A.lock(.read, byteRange: 11..<12)) + XCTAssertFalse(try ofd_B.lock(.write, byteRange: 11..<12)) + XCTAssertFalse(try ofd_A.lock(.write, byteRange: 11..<12)) } func testFileLocksWaiting() { diff --git a/Tests/SystemTests/FileOperationsTest.swift b/Tests/SystemTests/FileOperationsTest.swift index 14908f08..5591d01a 100644 --- a/Tests/SystemTests/FileOperationsTest.swift +++ b/Tests/SystemTests/FileOperationsTest.swift @@ -83,27 +83,6 @@ final class FileOperationsTest: XCTestCase { }, ] -// #if !os(Windows) -// syscallTestCases.append(contentsOf: [ -// // flock -// MockTestCase(name: "flock", .interruptable, rawFD, LOCK_SH) { retryOnInterrupt in -// _ = try fd.lock(exclusive: false, nonBlocking: false, retryOnInterrupt: retryOnInterrupt) -// }, -// MockTestCase(name: "flock", .interruptable, rawFD, LOCK_SH | LOCK_NB) { retryOnInterrupt in -// _ = try fd.lock(exclusive: false, nonBlocking: true, retryOnInterrupt: retryOnInterrupt) -// }, -// MockTestCase(name: "flock", .interruptable, rawFD, LOCK_EX) { retryOnInterrupt in -// _ = try fd.lock(exclusive: true, nonBlocking: false, retryOnInterrupt: retryOnInterrupt) -// }, -// MockTestCase(name: "flock", .interruptable, rawFD, LOCK_EX | LOCK_NB) { retryOnInterrupt in -// _ = try fd.lock(exclusive: true, nonBlocking: true, retryOnInterrupt: retryOnInterrupt) -// }, -// MockTestCase(name: "flock", .interruptable, rawFD, LOCK_UN) { retryOnInterrupt in -// _ = try fd.unlock(retryOnInterrupt: retryOnInterrupt) -// }, -// ]) -// #endif - for test in syscallTestCases { test.runAllTests() } }