ProcessID enhacement #8618
ProcessID enhacement #8618
Conversation
johnbute
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
xedin,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
owenv,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
|
Still need to add tests + rename to something clearer. if you have any suggestions on naming, please let me know :) |
|
@swift-ci please test |
|
@swift-ci please test |
johnbute
force-pushed
the
feature/pid-enhancement
branch
from
0e93697 to
1963d73
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
Sources/Basics/Concurrency/PID.swift
Outdated
| // | ||
| // This source file is part of the Swift open source project | ||
| // | ||
| // Copyright (c) 2023 Apple Inc. and the Swift project authors |
There was a problem hiding this comment.
nit: I'm unsure as to whether this code was pre-existing elsewhere, but if this is entirely new then I'd suggest pasting this updated header from swift.org! :)
johnbute
force-pushed
the
feature/pid-enhancement
branch
from
10b1dd4 to
670f56a
Compare
|
@swift-ci please test |
| // Check if the file exists | ||
| let filePath = self.lockFilePath.pathString | ||
| guard FileManager.default.fileExists(atPath: filePath) else { | ||
| print("File does not exist at path: \(filePath)") |
There was a problem hiding this comment.
issue (blocking): I think that these print statements need to be emitted through observability scopes instead of printing directly to stdout here.
| @@ -533,6 +533,45 @@ final class SwiftCommandStateTests: CommandsTestCase { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func testPIDFileHandlerLifecycle() async throws { | |||
There was a problem hiding this comment.
suggestion: These new tests are validating the PIDFile structure. Can we move the tests to a new location? Maybe under Tests/Basics/Concurrency/PIDTests.swift
Also, consider using Swift Testing instead of XCTest
| do { | ||
| try self.pidManipulator.deletePIDFile() | ||
| } catch { | ||
| self.observabilityScope.emit(warning: "Failed to delete PID file: \(error)") |
There was a problem hiding this comment.
thought: this message may be misleading. As a developer, I might wonder "what is the PID file" . What is the side effect of the existence of this file? Will the next "write" fail? if not, can we omit notifying the user?
| } | ||
|
|
||
| /// Read the pid file | ||
| public func readPID() -> Int32? { |
There was a problem hiding this comment.
suggestion (non-blocking): consider adding small tests that would validate all code paths in isolation.
| } | ||
|
|
||
| /// Delete PID file at URL | ||
| public func deletePIDFile() throws { |
There was a problem hiding this comment.
suggestion (non-blocking): consider adding small (ie: unit) tests that validate this function in isolation:
self.lockFilePathexists before calling functionself.locFilePathdoes no exists before calling function
|
|
||
| // Write current PID | ||
| let currentPID = pidHandler.getCurrentPID() | ||
| try pidHandler.writePID(pid: currentPID) |
There was a problem hiding this comment.
suggestion: can we add a test where multiple writePID(...) are calls sequentially?
Displays the PID of the SwiftPM instance currently holding the lock
Motivation:
Fixes #8528
Modifications:
Added a new abstraction for handling a .pid file which contains the pID of the currently running SwiftPM instance.
introduced the pidfileManipulator protocol and its implementation in pidFile struct to handle reading, writing, and deleting the PID file
Updated SwiftCommandState to use the pidFileManipulator to handle the pid file when attempting to acquire or release the lock.
Result:
Enhancement of knowing which processes are currently holding the lock + automatic cleanup