[process][darwin][freebsd][linux][openbsd] Make process.Children not reliant on pgrep - #1706
Conversation
|
Tested only on linux and openbsd |
shirou
left a comment
shirou
left a comment
There was a problem hiding this comment.
Thank you so much! I have confirmed that the expected behavior occurs on FreeBSD 13.3 and Ubuntu 20.04. This means, in the current master branch implementation, where an error exit status 1 would be returned, nil is now returned.
I have added a comment regarding the removal of the error definition. I would appreciate it if you could consider it.
|
|
||
| var ( | ||
| invoke common.Invoker = common.Invoke{} | ||
| ErrorNoChildren = errors.New("process does not have children") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…reliant on pgrep pgrep -P $PID exits with status of 1 (and nothing in stdout nor stderr) both if a process doesn't exist or it doesn't have child processes, so we don't use it anymore on these OSes. We sort PIDs as pgrep did. Also deprecate the ErrorNoChildren error when there are no child processes, this is erroneous (simply check for the length of the returned slice, plus this is not an error per se), this was only returned on linux anyway. Fixes shirou#1698
shirou
left a comment
shirou
left a comment
There was a problem hiding this comment.
LGTM! Thank you so much for your awesome work!
2 participants
pgrep -P $PID exits with status of 1 (and nothing in stdout nor stderr) both if a process doesn't exist or it doesn't have child processes, so we don't use it anymore on these OSes. We sort PIDs as pgrep did.
Also remove the ErrorNoChildren error when there are no child processes, this is erroneous (simply check for the length of the returned slice, plus this is not an error per se), this was only returned on linux anyway.
Fixes #1698