Skip to content
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.

Sign up for GitHub

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

[process][darwin][freebsd][linux][openbsd] Make process.Children not reliant on pgrep #1706

Merged
merged 1 commit into from
Sep 11, 2024
Merged

[process][darwin][freebsd][linux][openbsd] Make process.Children not reliant on pgrep #1706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 0 additions & 20 deletions internal/common/common_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,23 +40,3 @@ func CallLsofWithContext(ctx context.Context, invoke Invoker, pid int32, args ..
}
return ret, nil
}

func CallPgrepWithContext(ctx context.Context, invoke Invoker, pid int32) ([]int32, error) {
out, err := invoke.CommandWithContext(ctx, "pgrep", "-P", strconv.Itoa(int(pid)))
if err != nil {
return []int32{}, err
}
lines := strings.Split(string(out), "\n")
ret := make([]int32, 0, len(lines))
for _, l := range lines {
if len(l) == 0 {
continue
}
i, err := strconv.ParseInt(l, 10, 32)
if err != nil {
continue
}
ret = append(ret, int32(i))
}
return ret, nil
}
2 changes: 1 addition & 1 deletion process/process.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (

var (
invoke common.Invoker = common.Invoke{}
ErrorNoChildren = errors.New("process does not have children")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that ErrorNoChildren is a practically impossible error definition because when there are no children, pgrep exits with status 1. Therefore, it would be fine to remove this definition. However, since this definition has already been published, removing it would cause compilation failures.

So, how about adding a comment like Deprecated: This error is practically non-occurring? This way, it will be noted in the documentation and flagged by some linters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest to still keep the variable definition, but to never return it when there are no child processes found? Or to still return it when there are no child processes?

Keep in mind the API contract was already broken as this error was never documented in the Children() function comment and more importantly it was only returned on linux.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest to still keep the variable definition, but to never return it when there are no child processes found? Or to still return it when there are no child processes?

I am thinking of keeping this variable defined, and an empty list and nil would be returned. It means ErrorNoChildren is never used.

Keep in mind the API contract was already broken as this error was never documented in the Children() function comment and more importantly it was only returned on Linux.

You are indeed correct that the error is not mentioned in the Children() function comment. However, unfortunately, pkg.go.dev displays the Linux definition by default, and this variable is included in that documentation. There may be users who are only looking at this variable and use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

ErrorNoChildren = errors.New("process does not have children") // Deprecated: ErrorNoChildren is never returned by process.Children(), check its returned []*Process slice length instead
ErrorProcessNotRunning = errors.New("process does not exist")
ErrorNotPermitted = errors.New("operation not permitted")
)
Expand Down
18 changes: 11 additions & 7 deletions process/process_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"context"
"fmt"
"path/filepath"
"sort"
"strconv"
"strings"

Expand Down Expand Up @@ -233,18 +234,21 @@ func convertCPUTimes(s string) (ret float64, err error) {
}

func (p *Process) ChildrenWithContext(ctx context.Context) ([]*Process, error) {
pids, err := common.CallPgrepWithContext(ctx, invoke, p.Pid)
procs, err := ProcessesWithContext(ctx)
if err != nil {
return nil, err
return nil, nil
}
ret := make([]*Process, 0, len(pids))
for _, pid := range pids {
np, err := NewProcessWithContext(ctx, pid)
ret := make([]*Process, 0, len(procs))
for _, proc := range procs {
ppid, err := proc.PpidWithContext(ctx)
if err != nil {
return nil, err
continue
}
if ppid == p.Pid {
ret = append(ret, proc)
}
ret = append(ret, np)
}
sort.Slice(ret, func(i, j int) bool { return ret[i].Pid < ret[j].Pid })
return ret, nil
}

Expand Down
18 changes: 11 additions & 7 deletions process/process_freebsd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"context"
"errors"
"path/filepath"
"sort"
"strconv"
"strings"

Expand Down Expand Up @@ -269,18 +270,21 @@ func (p *Process) MemoryInfoWithContext(ctx context.Context) (*MemoryInfoStat, e
}

func (p *Process) ChildrenWithContext(ctx context.Context) ([]*Process, error) {
pids, err := common.CallPgrepWithContext(ctx, invoke, p.Pid)
procs, err := ProcessesWithContext(ctx)
if err != nil {
return nil, err
return nil, nil
}
ret := make([]*Process, 0, len(pids))
for _, pid := range pids {
np, err := NewProcessWithContext(ctx, pid)
ret := make([]*Process, 0, len(procs))
for _, proc := range procs {
ppid, err := proc.PpidWithContext(ctx)
if err != nil {
return nil, err
continue
}
if ppid == p.Pid {
ret = append(ret, proc)
}
ret = append(ret, np)
}
sort.Slice(ret, func(i, j int) bool { return ret[i].Pid < ret[j].Pid })
return ret, nil
}

Expand Down
32 changes: 23 additions & 9 deletions process/process_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"math"
"os"
"path/filepath"
"sort"
"strconv"
"strings"

Expand Down Expand Up @@ -338,21 +339,34 @@ func (p *Process) PageFaultsWithContext(ctx context.Context) (*PageFaultsStat, e
}

func (p *Process) ChildrenWithContext(ctx context.Context) ([]*Process, error) {
pids, err := common.CallPgrepWithContext(ctx, invoke, p.Pid)
statFiles, err := filepath.Glob(common.HostProcWithContext(ctx, "[0-9]*/stat"))
if err != nil {
return nil, err
}
if len(pids) == 0 {
return nil, ErrorNoChildren
}
ret := make([]*Process, 0, len(pids))
for _, pid := range pids {
np, err := NewProcessWithContext(ctx, pid)
ret := make([]*Process, 0, len(statFiles))
for _, statFile := range statFiles {
statContents, err := os.ReadFile(statFile)
if err != nil {
return nil, err
continue
}
fields := splitProcStat(statContents)
pid, err := strconv.ParseInt(fields[1], 10, 32)
if err != nil {
continue
}
ppid, err := strconv.ParseInt(fields[4], 10, 32)
if err != nil {
continue
}
if int32(ppid) == p.Pid {
np, err := NewProcessWithContext(ctx, int32(pid))
if err != nil {
continue
}
ret = append(ret, np)
}
ret = append(ret, np)
}
sort.Slice(ret, func(i, j int) bool { return ret[i].Pid < ret[j].Pid })
return ret, nil
}

Expand Down
18 changes: 11 additions & 7 deletions process/process_openbsd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"fmt"
"io"
"path/filepath"
"sort"
"strconv"
"strings"
"unsafe"
Expand Down Expand Up @@ -286,18 +287,21 @@ func (p *Process) MemoryInfoWithContext(ctx context.Context) (*MemoryInfoStat, e
}

func (p *Process) ChildrenWithContext(ctx context.Context) ([]*Process, error) {
pids, err := common.CallPgrepWithContext(ctx, invoke, p.Pid)
procs, err := ProcessesWithContext(ctx)
if err != nil {
return nil, err
return nil, nil
}
ret := make([]*Process, 0, len(pids))
for _, pid := range pids {
np, err := NewProcessWithContext(ctx, pid)
ret := make([]*Process, 0, len(procs))
for _, proc := range procs {
ppid, err := proc.PpidWithContext(ctx)
if err != nil {
return nil, err
continue
}
if ppid == p.Pid {
ret = append(ret, proc)
}
ret = append(ret, np)
}
sort.Slice(ret, func(i, j int) bool { return ret[i].Pid < ret[j].Pid })
return ret, nil
}

Expand Down
Loading