-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Fix Process.ExitCode on mac for killed processes #29407
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Runtime.InteropServices; | ||
|
|
||
| internal static partial class Interop | ||
| { | ||
| internal static partial class Sys | ||
| { | ||
| /// <summary> | ||
| /// Reaps a terminated child. | ||
| /// </summary> | ||
| /// <returns> | ||
| /// 1) when a child is reaped, its process id is returned | ||
| /// 2) if pid is not a child or there are no unwaited-for children, -1 is returned (errno=ECHILD) | ||
| /// 3) if the child has not yet terminated, 0 is returned | ||
| /// 4) on error, -1 is returned. | ||
| /// </returns> | ||
| [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_WaitPidExitedNoHang", SetLastError = true)] | ||
| internal static extern int WaitPidExitedNoHang(int pid, out int exitCode); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -450,34 +450,46 @@ extern "C" void SystemNative_SysLog(SysLogPriority priority, const char* message | |
| syslog(static_cast<int>(priority), message, arg1); | ||
| } | ||
|
|
||
| extern "C" int32_t SystemNative_WaitIdExitedNoHang(int32_t pid, int32_t* exitCode, int32_t keepWaitable) | ||
| extern "C" int32_t SystemNative_WaitIdAnyExitedNoHangNoWait() | ||
| { | ||
| assert(exitCode != nullptr); | ||
|
|
||
| siginfo_t siginfo; | ||
| int32_t result; | ||
| idtype_t idtype = pid == -1 ? P_ALL : P_PID; | ||
| int options = WEXITED | WNOHANG; | ||
| if (keepWaitable != 0) | ||
| { | ||
| options |= WNOWAIT; | ||
| } | ||
| while (CheckInterrupted(result = waitid(idtype, static_cast<id_t>(pid), &siginfo, options))); | ||
| if (idtype == P_ALL && result == -1 && errno == ECHILD) | ||
| while (CheckInterrupted(result = waitid(P_ALL, 0, &siginfo, WEXITED | WNOHANG | WNOWAIT))); | ||
| if (result == -1 && errno == ECHILD) | ||
| { | ||
| // The calling process has no existing unwaited-for child processes. | ||
| result = 0; | ||
| } | ||
| else if (result == 0 && siginfo.si_signo == SIGCHLD) | ||
| { | ||
| if (siginfo.si_code == CLD_EXITED) | ||
| result = siginfo.si_pid; | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| extern "C" int32_t SystemNative_WaitPidExitedNoHang(int32_t pid, int32_t* exitCode) | ||
| { | ||
| assert(exitCode != nullptr); | ||
|
|
||
| int32_t result; | ||
| int status; | ||
| while (CheckInterrupted(result = waitpid(pid, &status, WNOHANG))); | ||
| if (result > 0) | ||
| { | ||
| if (WIFEXITED(status)) | ||
| { | ||
| // the child terminated normally. | ||
| *exitCode = WEXITSTATUS(status); | ||
| } | ||
| else if (WIFSIGNALED(status)) | ||
| { | ||
| *exitCode = siginfo.si_status; | ||
| // child process was terminated by a signal. | ||
| *exitCode = 128 + WTERMSIG(status); | ||
| } | ||
| else | ||
| { | ||
| *exitCode = 128 + siginfo.si_status; | ||
| assert(false); | ||
| } | ||
| result = siginfo.si_pid; | ||
| } | ||
| return result; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need all of this logic in native code? In 2.0, wasn't most of it in managed and we just P/Invoked for waitpid, WIFEXITED, etc.? We've been trying to keep the shims as thin as possible.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, 2.0 had pinvoke for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. We can keep it here in this PR, but I do think it'd be good to revisit it subsequently. I think we've been a bit lax about the intent of the shims and we've ended up with more native code than is desirable. |
||
| } | ||
|
|
||
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.
Did you mean to change the pid from -1 to 0? From the man page, that would seem to restrict this to only be for child processes in the same group.
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.
id should be ignored for P_ALL. I've changed from -1 to 0 because id_t is unsigned and it gave me a compile error if I didn't add a cast.
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.
Where is that documented? I'm not seeing it.
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.
Linux man page: https://linux.die.net/man/3/waitid
http://pubs.opengroup.org/onlinepubs/9699919799/functions/waitid.html has the same line.
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.
Ok, thanks.