Update FileLock API #2036
Update FileLock API #2036
Conversation
Sources/Basic/Lock.swift
Outdated
| private var fd: CInt? | ||
| private var fd: FileDescriptor? | ||
|
|
||
| private let _lock = NSLock() |
There was a problem hiding this comment.
I don't think we need to make FileLock thread-safe. In the future, Workspace itself should implement thread safety for its operations.
There was a problem hiding this comment.
anything wrong with thread safety here?
There was a problem hiding this comment.
IMO it's just a little premature to add it unless there is a clear use case. It can manifest in weird bugs or can end up hiding real bugs since it'll kind of make Workspace thread-safe as a side-effect.
There was a problem hiding this comment.
If making a class (that is used once, in tests, and works just fine) thread-safe is a blocker, I don't know, I can only give up. Unless there is an actual problem spotted.
There was a problem hiding this comment.
It's not about how much it's being used right now. It's about what kind of abstraction we want to design for flock. In many cases, thread-safety will need to be implemented at a higher-level than FileLock anyway and then we end up with an unnecessary lock. Since this class abstracting flock, maybe we should see if flock itself is thread-safe. From my very little search, it doesn't look like it is explicitly marked thread-safe.
There was a problem hiding this comment.
normally this kind of access if thread safe, if you look at foundation too. since this is protecting access to the resource, I find lock appropriate to use.
There was a problem hiding this comment.
It doesn't seem thread-safe in Foundation either: https://developer.apple.com/documentation/foundation/nsdistributedlock?language=objc
There was a problem hiding this comment.
it is internally though, if you check sources
There was a problem hiding this comment.
I removed NSLock hoping this was the only obstacle.
|
The lock if for the FileLock operations only. It wont lock the workspace in any way though |
|
@aciidb0mb3r please re-review this PR. |
| @@ -85,19 +114,22 @@ public final class FileLock { | |||
| #else | |||
| // Open the lock file. | |||
| if fileDescriptor == nil { | |||
| let fd = SPMLibc.open(lockFile.pathString, O_WRONLY | O_CREAT | O_CLOEXEC, 0o666) | |||
| let fd = SPMLibc.open(path.pathString, O_WRONLY | O_CREAT | O_CLOEXEC, 0o666) | |||
There was a problem hiding this comment.
Can you update this to use the new openFile(at:) helper?
| /// that indicates whether the attempt was successful. | ||
| /// | ||
| /// - Returns: `true` if the lock was acquired, otherwise `false`. | ||
| public func `try`() -> Bool { |
There was a problem hiding this comment.
This won't compile on windows so should guard it with #if os() checks.
| let fileLock = FileLock(name: "test.lock", in: tempDir.path) | ||
|
|
||
| let lockThread = Thread { | ||
| try! SwiftPMProduct.TestSupportExecutable.execute(["fileLockLock", "test.lock", tempDir.path.pathString, "3"]) |
There was a problem hiding this comment.
Given the trivial implementation, I don't think it's worth adding a test case that will sleep for 3 seconds. It seems fine to land this PR without a test for the new try method.
|
@krzyzanowskim sorry for the slow response. is this still something you still want to pursue? |
|
@tomerd yes |
|
@krzyzanowskim cool. can you share a bit of background on this, i.e. how is this feature being used? also we'd need to rebase to catch up on changes (sorry) |
|
hey @krzyzanowskim did you want to update this PR so we can take it forward? |
|
@tomerd I may work on it, do you want this functionality in? |
|
@krzyzanowskim yes I believe we do, there area also potential touchpoint with swiftlang/swift-tools-support-core#130 which is designed to enable #2835, so ideally we come together to finalize both so we can work on the features that need this /cc @abertelrud @neonichu |
|
Would you also be interested in #2037 ? This was the tool to the actual feature I needed at that time which is workspace lock. |
|
ok computer. what just happened? |
|
@krzyzanowskim I just deleted |
|
Hi @krzyzanowskim, the Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
Update FileLock API.
This is a preparation work for workspace locking.