Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -95,46 +95,88 @@ private bool GetHasExited(bool refresh)
}

private void KillTree(ref List<Exception>? exceptions)
{
// First, stop and collect the entire process tree before killing any process.
// This is necessary because killing a parent process may cause children with
// PR_SET_PDEATHSIG (KillOnParentExit) to be killed by the kernel immediately,
// making it impossible to discover their descendants afterward.
List<Process> stoppedProcesses = [];
try
{
StopTree(ref exceptions, stoppedProcesses);
}
catch (Exception ex)
{
(exceptions ??= new List<Exception>()).Add(ex);
}
finally
{
// Kill all stopped processes even if StopTree threw partway through.
foreach (Process process in stoppedProcesses)
{
int killResult = Interop.Sys.Kill(process._processId, Interop.Sys.GetPlatformSignalNumber(PosixSignal.SIGKILL));
if (killResult != 0)
{
Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo();
// Ignore 'process no longer exists' error.
if (errorInfo.Error != Interop.Error.ESRCH)
{
(exceptions ??= new List<Exception>()).Add(new Win32Exception(errorInfo.RawErrno));
}
}

if (process != this)
{
process.Dispose();
Comment thread
adamsitnik marked this conversation as resolved.
}
}
}
}

/// <summary>
/// Recursively stops all processes in the tree (depth-first) and collects them into a flat list.
/// Stopping (SIGSTOP) before enumerating children ensures we can discover the entire tree
/// before any kill signals cause cascading terminations.
/// </summary>
/// <returns>
/// <see langword="true"/> if the process was successfully stopped and added to
/// <paramref name="stoppedProcesses"/>; <see langword="false"/> if the process had already
/// exited or could not be stopped, in which case the caller is responsible for disposing it.
/// </returns>
private bool StopTree(ref List<Exception>? exceptions, List<Process> stoppedProcesses)
{
// If the process has exited, we can no longer determine its children.
// If we know the process has exited, stop already.
if (GetHasExited(refresh: false))
{
return;
return false;
}

// Stop the process, so it won't start additional children.
// This is best effort: kill can return before the process is stopped.
int stopResult = Interop.Sys.Kill(_processId, Interop.Sys.GetPlatformSIGSTOP());
if (stopResult != 0)
{
Interop.Error error = Interop.Sys.GetLastError();
Interop.ErrorInfo errorInfo = Interop.Sys.GetLastErrorInfo();
// Ignore 'process no longer exists' error.
if (error != Interop.Error.ESRCH)
if (errorInfo.Error != Interop.Error.ESRCH)
{
(exceptions ??= new List<Exception>()).Add(new Win32Exception());
(exceptions ??= new List<Exception>()).Add(new Win32Exception(errorInfo.RawErrno));
}
return;
return false;
}

List<Process> children = GetChildProcesses();
stoppedProcesses.Add(this);

int killResult = Interop.Sys.Kill(_processId, Interop.Sys.GetPlatformSignalNumber(PosixSignal.SIGKILL));
if (killResult != 0)
List<Process> children = GetChildProcesses();
foreach (Process childProcess in children)
{
Interop.Error error = Interop.Sys.GetLastError();
// Ignore 'process no longer exists' error.
if (error != Interop.Error.ESRCH)
if (!childProcess.StopTree(ref exceptions, stoppedProcesses))
{
(exceptions ??= new List<Exception>()).Add(new Win32Exception());
childProcess.Dispose();
}
}
Comment thread
adamsitnik marked this conversation as resolved.

foreach (Process childProcess in children)
{
childProcess.KillTree(ref exceptions);
childProcess.Dispose();
}
return true;
}

/// <summary>Discards any information about the associated process.</summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.InteropServices;
using System.Threading;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;

Expand Down Expand Up @@ -145,6 +146,65 @@ public void KillOnParentExit_KillsTheChild_WhenParentIsKilled(bool enabled, bool
}
}

[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData(true)]
[InlineData(false)]
public void KillEntireProcessTree_KillsGrandchild_WhenIntermediateChildHasKillOnParentExit(bool enabled)
{
// Mimics the scenario: Process A -> Process B (KillOnParentExit) -> Process C
// Killing A with entireProcessTree:true should kill C regardless of KillOnParentExit setting.
RemoteInvokeOptions parentOptions = new() { CheckExitCode = false };
parentOptions.StartInfo.RedirectStandardOutput = true;
parentOptions.StartInfo.RedirectStandardInput = true;

using RemoteInvokeHandle parentHandle = RemoteExecutor.Invoke(
(enabledStr) =>
{
// This is "Process A". Start "Process B" with KillOnParentExit.
// Process B will start "Process C" and report C's PID.
using Process child = CreateProcess(() =>
{
// This is "Process B". Start "Process C" (long-running, no KillOnParentExit).
using Process grandChild = CreateProcessLong();
grandChild.Start();
Console.WriteLine(grandChild.Id);

// Block until killed
Thread.Sleep(Timeout.Infinite);
return RemoteExecutor.SuccessExitCode;
});
child.StartInfo.KillOnParentExit = bool.Parse(enabledStr);
child.StartInfo.RedirectStandardOutput = true;
child.Start();

// Read grandchild PID from Process B and forward to test
string grandChildPidStr = child.StandardOutput.ReadLine();
Console.WriteLine(grandChildPidStr);

// Block until killed
Thread.Sleep(Timeout.Infinite);
},
enabled.ToString(),
parentOptions);

int grandChildPid = int.Parse(parentHandle.Process.StandardOutput.ReadLine());
using Process grandchild = Process.GetProcessById(grandChildPid);

try
{
// Kill Process A with entireProcessTree: true
parentHandle.Process.Kill(entireProcessTree: true);

Assert.True(parentHandle.Process.WaitForExit(WaitInMS));
// The grandchild (Process C) should also be killed
Assert.True(grandchild.WaitForExit(WaitInMS));
}
finally
{
grandchild.Kill();
}
Comment thread
adamsitnik marked this conversation as resolved.
}

[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData(true, true)]
[InlineData(true, false)]
Expand Down
4 changes: 2 additions & 2 deletions src/native/libs/System.Native/pal_signal.c
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ void SystemNative_SetTerminalInvalidationHandler(TerminalInvalidationCallback ca

installed = InstallSignalHandler(SIGCONT, SA_RESTART);
assert(installed);
installed = InstallSignalHandler(SIGCHLD, SA_RESTART);
installed = InstallSignalHandler(SIGCHLD, SA_RESTART | SA_NOCLDSTOP);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to other code reviewers: after making the code send SIGSTOP to all processes in its tree, some of the tests started to time out on macOS. copilot was giving me really strange explanations of it so I've asked @jozkee for help with debugging offline (I don't have a mac machine that can build dotnet/runtime), I got following update:

Details
I re-triggered the hang and sampled the stuck runner process ( sample <pid> ). The blocked/spinning thread is not in  proc_pidinfo  — in fact  proc_pidinfo  appears zero times in the whole sample. The PR thread's "proc_pidinfo blocks on a suspended process" theory does not match what's actually happening.
 
What's actually hung: the  .NET SigHandler  thread is busy-spinning (root process stuck in state  T ), tight-looping over the child-reaping syscalls:
 
Thread ...: .NET SigHandler
  SignalHandlerLoop  (libSystem.Native)
    ├─ SystemNative_WaitPidExitedNoHang        → __wait4_nocancel
    └─ SystemNative_WaitIdAnyExitedNoHangNoWait → __waitid_nocancel
       (interleaved with GetErrNo / SetLastPInvokeError — i.e. a hot retry loop)
 
Meanwhile the runner's main thread is idle ( LowLevelMonitor_Wait  →  __psynch_cvwait ) — just waiting for the test task that never finishes.
 
Interpretation: The test SIGSTOPs process A (61506), which is a direct child of the test/runner process. That makes the kernel deliver  SIGCHLD  (job-control, child stopped) to the runtime. The runtime's SIGCHLD reaping loop peeks with  waitid(WEXITED | WNOHANG | WNOWAIT)  and reaps with  wait4  — neither of which consumes a stopped (not exited) child notification. So the notification is never cleared and the SigHandler thread spins forever calling  waitid / wait4 , burning CPU, and the  Kill(entireProcessTree:true)  call never completes → test hangs.
 
So the true macOS root cause is: SIGSTOP'ing a child of the .NET process wedges the runtime's SIGCHLD child-reaping loop into a busy spin, not a  proc_pidinfo  block. This still points to the same fix direction (don't leave the tree SIGSTOP'd on macOS / avoid the two-phase stop on non-Linux), but for a different underlying reason than the PR describes.

Which lead me to conclusion that if we use SA_NOCLDSTOP here we can receive SIGCHILD only when a child process has exited or has been killed, not stopped and/or resumed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From the above, my understanding is that we're adding SA_NOCLDSTOP to avoid getting signalled for stopped children. This avoids getting stuck in a loop because macOS returns stopped, non-exited children from this check (even though we don't include WSTOPPED in the waitid):

while (CheckInterrupted(result = waitid(P_ALL, 0, &siginfo, WEXITED | WNOHANG | WNOWAIT)));
if (result == 0)
{
// When there are no waitable children and WNOHANG is specified,
// waitid may return zero with si_pid unchanged.
assert(siginfo.si_pid == 0 || // no waitable child
siginfo.si_signo == SIGCHLD); // waitable child
result = siginfo.si_pid;
}

Fix lgtm.

assert(installed);
installed = InstallSignalHandler(SIGWINCH, SA_RESTART);
assert(installed);
Expand All @@ -526,7 +526,7 @@ void SystemNative_RegisterForSigChld(SigChldCallback callback)
{
g_sigChldCallback = callback;

installed = InstallSignalHandler(SIGCHLD, SA_RESTART);
installed = InstallSignalHandler(SIGCHLD, SA_RESTART | SA_NOCLDSTOP);
assert(installed);
}
pthread_mutex_unlock(&lock);
Expand Down
Loading