-
Notifications
You must be signed in to change notification settings - Fork 49
Adding FileLock #361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding FileLock #361
Conversation
e3cc983
to
e55967c
Compare
1ad88a3
to
e5a6c8b
Compare
@swift-ci test macOS |
48e6ef7
to
b410967
Compare
Sources/SwiftlyCore/FileLock.swift
Outdated
self.filePath = path | ||
do { | ||
let fileURL = URL(fileURLWithPath: self.filePath.string) | ||
let contents = getpid().description.data(using: .utf8) ?? Data() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use https://developer.apple.com/documentation/foundation/processinfo/processidentifier instead of getpid()
? One fewer obstacle towards getting swiftly to work on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored @jakepetroules
b410967
to
689434a
Compare
@swift-ci test macOS |
ea8e37a
to
858ec90
Compare
@swift-ci test macOS |
4359607
to
4ff4945
Compare
@swift-ci test macOS |
Sources/SwiftlyCore/FileLock.swift
Outdated
var localizedDescription: String { | ||
switch self { | ||
case let .cannotAcquireLock(path): | ||
return "Cannot acquire lock at \(path). Another process may be holding the lock." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): You can turn this into an actionable error by indicating that the user can remove the file if they are sure that there are no other processes running with the lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
acb796a
to
f4c28a4
Compare
@swift-ci test macOS |
var errorDescription: String? { | ||
switch self { | ||
case let .cannotAcquireLock(path): | ||
return "Cannot acquire lock at \(path). Another process may be holding the lock. If you are sure no other processes are running, you can manually remove the lock file at \(path)." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: The path is mentioned twice in the error message when it is probably only needed once.
Sources/SwiftlyCore/FileLock.swift
Outdated
} | ||
} | ||
|
||
// Timeout reached, throw an error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: This check to reconstruct the error may not be needed if you keep the last error, or use the Swift Result I linked above. It's fine as it is though.
|
||
@Suite struct FileLockTests { | ||
@Test("FileLock creation writes process ID to lock file") | ||
func testFileLockCreation() async throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: These are really good unit tests: simple, and reliable. Sometimes lock testing tries to simulate race conditions, which can sacrifice reliability of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is looking really good and very nearly ready to be merged. There's just one blocking issue, and one language check override to do, and it should be done.
f4c28a4
to
20d1898
Compare
20d1898
to
cdf027a
Compare
@swift-ci test macOS |
Fixes Issue #142
Serialize concurrent swiftly install commands with a simple file lock.
Adds a lightweight FileLock (using
Data.write(to: fileURL, options: .withoutOverwriting)
) and wraps the install routine so only one process installs at a time