Add a ProcessManager interface for managing processes by pid file #293
Conversation
Reviewer's Guide by SourceryThis pull request introduces a Sequence diagram for finding and killing a processsequenceDiagram
participant P as Process
participant OS as Operating System
P->>P: Pid()
P->>OS: ReadFile(PidFilePath())
OS-->>P: pid (string)
P->>P: Atoi(pid)
P->>OS: FindProcess(pid)
OS-->>P: process, error
alt error != nil
P-->>P: Return error
else
P->>OS: Signal(SIGKILL)
OS-->>P: error
alt error != nil
P-->>P: Return error
else
P->>OS: Wait()
OS-->>P: _, error
alt error != nil
P-->>P: Return error
else
P-->>P: Return nil
end
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vyasgun - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a
Close()method to theProcessManagerinterface to handle cleanup operations like removing the pid file. - The
Newfunction doesn't return an error, even though the signature allows it; consider removing the error return or adding a check.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
vyasgun
force-pushed
the
pr/pid-management
branch
from
112f790 to
4a6f13b
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @vyasgun - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a
Close()method to theProcessstruct to handle cleaning up resources like the pid file. - The
Kill()method should probably remove the pid file after successfully killing the process.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pkg/process/process.go
Outdated
| func New(name, pidFilePath string) (*Process, error) { | ||
| return &Process{name: name, pidFilePath: pidFilePath}, nil |
There was a problem hiding this comment.
suggestion: Reconsider New() returning an error.
Since the constructor never fails, it might be cleaner to have New() simply return *Process. If error checking is anticipated in future extensions, a brief comment explaining the design decision would be helpful.
Suggested implementation:
func New(name, pidFilePath string) *Process { // Currently always succeeds; change signature if error handling is needed in the future. return &Process{name: name, pidFilePath: pidFilePath}
There was a problem hiding this comment.
Good advice! the error is not needed, no reason to add it.
There was a problem hiding this comment.
Sounds good—let’s simplify the constructor. We can drop the error return and update any callers/tests accordingly. For example:
func New(name, pidFilePath string) *Process {
return &Process{name: name, pidFilePath: pidFilePath}
}Then:
- Remove the
errorfrom the signature. - Delete the unused
github.com/pkg/errorsimport if there are no other error-wrapping calls in that file. - Update any tests or code that expected two return values from
New.
If you agree, go ahead and push that change. Otherwise, let me know if you see any reason to keep the error return in place for future-proofing.
pkg/process/process.go
Outdated
| func (p *Process) Pid() (int, error) { | ||
| data, err := os.ReadFile(p.PidFilePath()) | ||
| if err != nil { | ||
| return -1, err | ||
| } | ||
| pidStr := strings.TrimSpace(string(data)) | ||
| pid, err := strconv.Atoi(pidStr) |
There was a problem hiding this comment.
suggestion: Wrap conversion error with more context.
While errors from strconv.Atoi are returned directly, adding contextual information (similar to the wrapping used elsewhere) could facilitate debugging if the PID file is malformed. This would improve consistency in error handling.
There was a problem hiding this comment.
It may help - need to test invalid pid with real code using the library and see if the error makes sense or we need to add some context.
There was a problem hiding this comment.
That sounds reasonable. To get a feel for whether additional context is useful, you could write a quick invalid‐PID test:
func TestPidMalformed(t *testing.T) {
tmp := t.TempDir()
pidFile := filepath.Join(tmp, "test.pid")
// write something non‐numeric
os.WriteFile(pidFile, []byte("notanumber"), 0644)
p, _ := New("dummy", pidFile)
_, err := p.Pid()
if err == nil {
t.Fatal("expected error when reading malformed PID")
}
t.Log("got error:", err)
}If you run that and see something like strconv.Atoi: parsing "notanumber": invalid syntax, you’ll notice it doesn’t tell you which file failed. We could wrap it like this:
func (p *Process) Pid() (int, error) {
data, err := os.ReadFile(p.PidFilePath())
if err != nil {
return -1, err
}
pidStr := strings.TrimSpace(string(data))
- pid, err := strconv.Atoi(pidStr)
- return pid, err
+ pid, err := strconv.Atoi(pidStr)
+ if err != nil {
+ return -1, errors.Wrapf(err, "parsing PID from file %s", p.PidFilePath())
+ }
+ return pid, nil
}That way the user sees both the parse error and the file path. Let me know if that addresses your concern or if you’d prefer a different error message format.
There was a problem hiding this comment.
Why add an old dependency (errors.Wrapf) when we can use standard library (fmt.Errorf("...: %w", err))?
There was a problem hiding this comment.
You’re absolutely right—since we’re targeting Go 1.13+ we can just use the stdlib’s fmt.Errorf with %w and drop pkg/errors entirely. For example:
-import "github.com/pkg/errors"
+import "fmt"
func (p *Process) Pid() (int, error) {
data, err := os.ReadFile(p.PidFilePath())
if err != nil {
return -1, err
}
pidStr := strings.TrimSpace(string(data))
- pid, err := strconv.Atoi(pidStr)
- if err != nil {
- return -1, errors.Wrapf(err, "parsing PID from file %s", p.PidFilePath())
- }
- return pid, nil
+ pid, err := strconv.Atoi(pidStr)
+ if err != nil {
+ return -1, fmt.Errorf("parsing PID from file %s: %w", p.PidFilePath(), err)
+ }
+ return pid, nil
}Then you can remove the github.com/pkg/errors import entirely. This keeps us on the standard library’s error-wrapping and avoids bringing in an extra dependency. Let me know if that works for you!
vyasgun
force-pushed
the
pr/pid-management
branch
from
4a6f13b to
adb05d0
Compare
pkg/process/process.go
Outdated
| if err != nil { | ||
| return false | ||
| } | ||
| if err := proc.Signal(syscall.Signal(0)); err != nil { |
There was a problem hiding this comment.
Why syscall.Signal(0)? Could this use one of the SIGXXX constants from https://pkg.go.dev/syscall#pkg-constants ? (search for SIGABRT)
There was a problem hiding this comment.
Is it safe to call SIGABRT on the running process? On searching I found sending 0 is the safest way to check a process is running
There was a problem hiding this comment.
I mentioned SIGABRT as a way to find the relevant section, I was not suggesting to use this signal :)
However, if FindProcess() found it, I’d assume it means the process is running, no need to check again that it’s running ?
This commit adds a process package with Manager interface and Process struct The interface has the following methods: - ReadPidFile() (int, error) - Name() string - PidFilePath() string - Exists() bool - Terminate() error - Kill() error - FindProcess() (*os.Process, error) - WritePidFile(pid int) error The commit also adds tests for process lifecycle with a dummy process
vyasgun
force-pushed
the
pr/pid-management
branch
from
adb05d0 to
5d6ee02
Compare
| } | ||
| if !exists { | ||
| return nil, fmt.Errorf("process not found") | ||
| } |
There was a problem hiding this comment.
It will be hard to tell if the process does not exist when we return this error. We need to return which is easy to check like os.ErrNotExist for this special case.
| } | ||
| if proc == nil { | ||
| return nil, fmt.Errorf("process not found") | ||
| } |
There was a problem hiding this comment.
Why check if the process exists with process.PidExists(int32(pid)) if we get the same information by creating a process with process.NewProcess(int32(pid))?
Check how PidExists is implemented, I guess it create a process internally.
| } | ||
| if name != p.Name() { | ||
| return nil, fmt.Errorf("pid %d is stale, and is being used by %s", pid, name) | ||
| } |
There was a problem hiding this comment.
The pid is stale. How the user can tell that it can delete the pidfile? We need an error that the user can check, meaning that there is no such process matching name, pid and executable path.
| return nil, fmt.Errorf("cannot find process exe: %v", err) | ||
| } | ||
| if exe != p.ExecutablePath() { | ||
| return nil, fmt.Errorf("pid %d is stale, and is being used by %s", pid, exe) |
|
|
||
| func (p *Process) Exists() bool { | ||
| proc, err := p.FindProcess() | ||
| return err == nil && proc != nil |
There was a problem hiding this comment.
This may be wrong - maybe the process exists but we don't have permissions to get the details?
In minikube we return (bool, error) so it is easy to tell if the process exists or we cannot tell.
https://github.com/kubernetes/minikube/blob/89b97e001600434322b135de9631570fd24bad22/pkg/minikube/process/process.go#L53
| // Try to kill the non-existent process | ||
| // This should result in an error | ||
| err = managedProcess.Kill() | ||
| assert.Error(t, err) |
There was a problem hiding this comment.
Kill will be easier to use if it is idempotent. Trying to kill a process that does not exist is not an error from the caller point of view.
| @@ -0,0 +1,99 @@ | |||
| package process | |||
There was a problem hiding this comment.
Looks like we need only public names, so we can use process_test package. This will be better tests since the test code will be the same as real code using this package.
| @@ -0,0 +1,119 @@ | |||
| package process | |||
| return pid, nil | ||
| } | ||
|
|
||
| func (p *Process) FindProcess() (*process.Process, error) { |
There was a problem hiding this comment.
Does it need to be public or it an be a private helper? Exposing the underlying process package in this packages means we can never change the implementation, for example using another package to validate the process executable path.
This commit does the following:
Fixes: #278
Summary by Sourcery
Implement a ProcessManager interface for managing processes using pid files
New Features:
Tests: