Repository navigation
Conversation
Fall back to scanning tracked children when waitid reports a child that cannot be reaped, avoiding a SIGCHLD livelock with stopped children on macOS. Add isolated synchronous and asynchronous regression coverage and restore class-level test parallelism on macOS. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process |
|
cc @tmds |
| // The child may be unmanaged, or stopped rather than exited (macOS waitid | ||
| // can report stopped children despite WEXITED). Scan all managed children |
There was a problem hiding this comment.
suggestion for between the parentheses:
"on macOS, WaitIdAnyExitedNoHangNoWait can report stopped children, despite not including WSTOPPED when calling waitid"
adamsitnik
left a comment
There was a problem hiding this comment.
The fix is very simple and provides reliable test 👍
Before I hit the approve button, I need to get a final confirmation of how exactly it works (I need to understand risk, as I want to backport it to .NET 11)
Big thanks for your help @steveisok and @tmds !
|
|
||
| try | ||
| { | ||
| Assert.Equal(0, Interop.Sys.Kill(stoppedPid, Interop.Sys.GetPlatformSIGSTOP())); |
There was a problem hiding this comment.
I wanted to suggest using Process.Signal(PosixSignal) here but then I realized that PosixSignal dos not expose SIGSTOP ;)
| [PlatformSpecific(TestPlatforms.OSX)] | ||
| [InlineData(false)] | ||
| [InlineData(true)] | ||
| public void WaitForExit_StoppedChild_DoesNotPreventReapingOtherChildren(bool useAsync) |
There was a problem hiding this comment.
At the first look, this test seems to be a hack. But after studying my own #133948 I think it's currently the best way to test the hang in deterministic way without spawning too many processes. So it's great!👍
| // unlikely: This is not a managed Process, so we are not responsible for reaping. | ||
| // Fall back to checking all Processes. | ||
| // The child may be unmanaged, or stopped rather than exited (macOS waitid | ||
| // can report stopped children despite WEXITED). Scan all managed children |
There was a problem hiding this comment.
I would extend what @tmds has suggested with the mention of SA_NOCLDSTOP which is the thing I've used in #128598 hoping it's going to be enough
| // can report stopped children despite WEXITED). Scan all managed children | |
| // can report stopped children despite not including WSTOPPED when calling waitid | |
| // and using SA_NOCLDSTOP when registering the signal handler). Scan all managed children |
| { | ||
| if (s_childProcessWaitStates.TryGetValue(pid, out ProcessWaitState? pws)) | ||
| if (s_childProcessWaitStates.TryGetValue(pid, out ProcessWaitState? pws) && | ||
| pws.TryReapChild(configureConsole)) |
There was a problem hiding this comment.
@tmds @steveisok Just to make sure I have good understanding of this fix, as I would like to backport it to .NET 11.
- When registering SIGCHILD, we let OS know we are not interested in SIGSTOP by using SA_NOCLDSTOP:
- macOS ignores it and as soon we SIGSTOP a child (for example, by using
Process.Kill(entireProcessTree: true), we receive SIGCHILD. - In SIGCHILD handler, we check for the ID of the child process that has exited and has not been reaped. On macOS, the pid of the stopped process is returned, we fail to reap it and keep looping forever.
- With this fix, we are going to read the pid of the process that was stopped, fail to reap it and fall back to checking all other running child processes for exit. We won't loop forever, but we also won't reap it.
- For every next SIGCHILD, we are going to repeat that, as the signal won't be reaped (marked as observed).
- If the SIGSTOP was sent by
Process.Kill(entireProcessTree: true), the process is going to be terminated soon, and then we are going to reap it and stop falling to checking all child processes?
There was a problem hiding this comment.
That is correct.
To avoid the checkAll behavior during the Process.Kill, we'd need to consume the STOPPED state.
There was a problem hiding this comment.
Yes, that matches the fix’s behavior. While the stopped status remains observable, subsequent SIGCHLD callbacks can take the scan-all fallback. Once the child is killed and reaped, it no longer causes that fallback. The key is releasing the shared locks so process-tree termination and unrelated Process operations can continue. As tmds notes, consuming the stopped status would avoid those repeated scans; this fix leaves that status untouched.
Document the omitted WSTOPPED option and the SIGCHLD handler's SA_NOCLDSTOP registration without changing behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fixes #135294.
On macOS,
waitid(WEXITED | WNOHANG | WNOWAIT)can report a stopped child even thoughwaitpid(WNOHANG)cannot reap it. The SIGCHLD handler repeatedly observes the same child while holding shared process-management locks, blocking other Process operations.Fall back to scanning all tracked children when the reported child cannot be reaped, rather than continuing the no-progress loop. This guard lives in the shared Unix implementation; successful reaping retains the existing fast path, and the scan reuses the existing nonblocking fallback for unknown children.
Resolves #135294
Note
This pull request description and code changes were generated with GitHub Copilot assistance.