-
Notifications
You must be signed in to change notification settings - Fork 818
Fix InvalidOperationException thrown from Process.KillTree #360
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
Conversation
d5a5e18
to
b740305
Compare
🆙 📅 reverted my last attempt and rebased on dev |
|
||
public static void KillTree(this Process process, TimeSpan timeout) | ||
{ | ||
string stdout; | ||
var pid = process.Id; |
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.
nit: trim spaces.
timeout, | ||
out stdout); | ||
out var _); |
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.
If this code is a flakiness source should we consider adding logging support?
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.
Source of flakiness was my bad change, which I reverted in d204d18
|
||
if (exitCode == 0 && !string.IsNullOrEmpty(stdout)) | ||
if (!string.IsNullOrEmpty(stdout)) |
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.
Not a fan of using stdout for this.
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.
Me neither. I investigated alternatives.
On Linux, we could use procfs and read /proc/$pid/stat
.
On macOS, we would need to p/invoke to a native method. There is no good method to easily find parent ID of a given process, so we would need a little native library with this code:
#include <libproc.h>
int getppid(const pid_t pid)
{
proc_bsdinfo info;
proc_pidinfo(pid, PROC_PIDTBSDINFO, 0, &info, sizeof(info));
return info.pbi_ppid;
}
|
{ | ||
var startInfo = new ProcessStartInfo | ||
{ | ||
FileName = fileName, | ||
Arguments = arguments, | ||
RedirectStandardOutput = true, | ||
UseShellExecute = false | ||
RedirectStandardError = true, |
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.
We don't use stderr or out anywhere, just return a bool if the process didn't exit.
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.
stdout is used by pgrep to find the list of child processes
I'm capturing stderr (to suppress) because this was always producing random console output in dotnet-watch such as kill 1234 failed: no such process
* Convert functional tests to run using TestServer * Allow functional tests to run in parallel. Fixes dotnet#360 Fixes dotnet#390
Attempt to fix "System.InvalidOperationException : Cannot process request because the process (5064) has exited." on Windows.