Skip to content
This repository was archived by the owner on Apr 20, 2023. It is now read-only.

Implement child process reaping. #10720

Merged
merged 4 commits into from
Feb 8, 2019
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
15 changes: 13 additions & 2 deletions TestAssets/TestProjects/TestAppThatWaits/Program.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using System;
using System.Diagnostics;
using System.IO;
using System.Runtime.InteropServices;
using System.Threading;

namespace TestAppThatWaits
Expand All @@ -9,16 +11,25 @@ class Program
static void Main(string[] args)
{
Console.CancelKeyPress += HandleCancelKeyPress;
AppDomain.CurrentDomain.ProcessExit += HandleProcessExit;

Console.WriteLine(Process.GetCurrentProcess().Id);
Console.Out.Flush();
Console.Read();
Thread.Sleep(10000);

Thread.Sleep(Timeout.Infinite);
}

static void HandleCancelKeyPress(object sender, ConsoleCancelEventArgs e)
{
Console.WriteLine("Interrupted!");
AppDomain.CurrentDomain.ProcessExit -= HandleProcessExit;
Environment.Exit(42);
}

static void HandleProcessExit(object sender, EventArgs args)
{
Console.WriteLine("Terminating!");
Environment.ExitCode = 43;
}
}
}
31 changes: 12 additions & 19 deletions src/Microsoft.DotNet.Cli.Utils/Command.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public class Command : ICommand
private readonly Process _process;

private StreamForwarder _stdOut;

private StreamForwarder _stdErr;

private bool _running = false;
Expand All @@ -40,8 +40,6 @@ public CommandResult Execute()

_process.EnableRaisingEvents = true;

Console.CancelKeyPress += HandleCancelKeyPress;

#if DEBUG
var sw = Stopwatch.StartNew();

Expand All @@ -51,20 +49,21 @@ public CommandResult Execute()
{
_process.Start();

Reporter.Verbose.WriteLine(string.Format(
LocalizableStrings.ProcessId,
_process.Id));
using (new ProcessReaper(_process))
{
Reporter.Verbose.WriteLine(string.Format(
LocalizableStrings.ProcessId,
_process.Id));

var taskOut = _stdOut?.BeginRead(_process.StandardOutput);
var taskErr = _stdErr?.BeginRead(_process.StandardError);
_process.WaitForExit();
var taskOut = _stdOut?.BeginRead(_process.StandardOutput);
var taskErr = _stdErr?.BeginRead(_process.StandardError);
_process.WaitForExit();

taskOut?.Wait();
taskErr?.Wait();
taskOut?.Wait();
taskErr?.Wait();
}
}

Console.CancelKeyPress -= HandleCancelKeyPress;

var exitCode = _process.ExitCode;

#if DEBUG
Expand Down Expand Up @@ -215,11 +214,5 @@ private void ThrowIfRunning([CallerMemberName] string memberName = null)
memberName));
}
}

private void HandleCancelKeyPress(object sender, ConsoleCancelEventArgs e)
{
// Ignore SIGINT/SIGQUIT so that the child can process the signal
e.Cancel = true;
}
}
}
80 changes: 80 additions & 0 deletions src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
using System;
using System.Runtime.InteropServices;
using Microsoft.Win32.SafeHandles;

namespace Microsoft.DotNet.Cli.Utils
{
internal static class NativeMethods
{
internal static class Windows
{
internal enum JobObjectInfoClass : uint
{
JobObjectExtendedLimitInformation = 9,
}

[Flags]
internal enum JobObjectLimitFlags : uint
{
JobObjectLimitKillOnJobClose = 0x2000,
}

[StructLayout(LayoutKind.Sequential)]
internal struct JobObjectBasicLimitInformation
{
public Int64 PerProcessUserTimeLimit;
public Int64 PerJobUserTimeLimit;
public JobObjectLimitFlags LimitFlags;
public UIntPtr MinimumWorkingSetSize;
public UIntPtr MaximumWorkingSetSize;
public UInt32 ActiveProcessLimit;
public UIntPtr Affinity;
public UInt32 PriorityClass;
public UInt32 SchedulingClass;
}

[StructLayout(LayoutKind.Sequential)]
internal struct IoCounters
{
public UInt64 ReadOperationCount;
public UInt64 WriteOperationCount;
public UInt64 OtherOperationCount;
public UInt64 ReadTransferCount;
public UInt64 WriteTransferCount;
public UInt64 OtherTransferCount;
}

[StructLayout(LayoutKind.Sequential)]
internal struct JobObjectExtendedLimitInformation
{
public JobObjectBasicLimitInformation BasicLimitInformation;
public IoCounters IoInfo;
public UIntPtr ProcessMemoryLimit;
public UIntPtr JobMemoryLimit;
public UIntPtr PeakProcessMemoryUsed;
public UIntPtr PeakJobMemoryUsed;
}

[DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)]
internal static extern SafeWaitHandle CreateJobObjectW(IntPtr lpJobAttributes, string lpName);

[DllImport("kernel32.dll", SetLastError = true)]
internal static extern bool SetInformationJobObject(IntPtr hJob, JobObjectInfoClass jobObjectInformationClass, IntPtr lpJobObjectInformation, UInt32 cbJobObjectInformationLength);

[DllImport("kernel32.dll", SetLastError = true)]
internal static extern bool AssignProcessToJobObject(IntPtr hJob, IntPtr hProcess);
}

internal static class Posix
{
[DllImport("libc", SetLastError = true)]
internal static extern int getpgid(int pid);

[DllImport("libc", SetLastError = true)]
internal static extern int kill(int pid, int sig);

internal const int SIGINT = 2;
internal const int SIGTERM = 15;
}
}
}
163 changes: 163 additions & 0 deletions src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
using System;
using System.ComponentModel;
using System.Diagnostics;
using System.Runtime.InteropServices;
using Microsoft.DotNet.PlatformAbstractions;
using Microsoft.Win32.SafeHandles;

using RuntimeEnvironment = Microsoft.DotNet.PlatformAbstractions.RuntimeEnvironment;

namespace Microsoft.DotNet.Cli.Utils
{
/// <summary>
/// Responsible for reaping a target process if the current process terminates.
/// </summary>
/// <remarks>
/// On Windows, a job object will be used to ensure the termination of the target
/// process (and its tree) even if the current process is rudely terminated.
///
/// On POSIX systems, the reaper will handle SIGTERM and attempt to forward the
/// signal to the target process only.
///
/// The reaper also suppresses SIGINT in the current process to allow the target
/// process to handle the signal.
/// </remarks>
internal class ProcessReaper : IDisposable
{
/// <summary>
/// Creates a new process reaper.
/// </summary>
/// <param name="process">The target process to reap if the current process terminates. The process must already be started.</param>
public ProcessReaper(Process process)
{
_process = process;

if (RuntimeEnvironment.OperatingSystemPlatform == Platform.Windows)
{
_job = AssignProcessToJobObject(_process.Handle);
}
else
{
AppDomain.CurrentDomain.ProcessExit += HandleProcessExit;
}

Console.CancelKeyPress += HandleCancelKeyPress;
}

public void Dispose()
{
if (RuntimeEnvironment.OperatingSystemPlatform == Platform.Windows)
{
if (_job != null)
{
// Clear the kill on close flag because the child process terminated successfully
// If this fails, then we have no choice but to terminate any remaining processes in the job
SetKillOnJobClose(_job.DangerousGetHandle(), false);

_job.Dispose();
_job = null;
}
}
else
{
AppDomain.CurrentDomain.ProcessExit -= HandleProcessExit;
}

Console.CancelKeyPress -= HandleCancelKeyPress;
}

private static void HandleCancelKeyPress(object sender, ConsoleCancelEventArgs e)
{
// Ignore SIGINT/SIGQUIT so that the process can handle the signal
e.Cancel = true;
}

private static SafeWaitHandle AssignProcessToJobObject(IntPtr process)
{
var job = NativeMethods.Windows.CreateJobObjectW(IntPtr.Zero, null);
if (job == null || job.IsInvalid)
{
return null;
}

if (!SetKillOnJobClose(job.DangerousGetHandle(), true))
{
job.Dispose();
return null;
}

if (!NativeMethods.Windows.AssignProcessToJobObject(job.DangerousGetHandle(), process))
{
job.Dispose();
return null;
}

return job;
}

private void HandleProcessExit(object sender, EventArgs args)
{
var currentPid = Process.GetCurrentProcess().Id;
bool sendChildSIGTERM = true;

// First, try to SIGTERM our process group
// If the pgid is not the same as pid, then this process is not the root of the group
if (NativeMethods.Posix.getpgid(currentPid) == currentPid)
Copy link
Member

Choose a reason for hiding this comment

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

There is no code that calls `setpgid. Why deal with this specifically?

Copy link
Author

@peterhuene peterhuene Feb 12, 2019

Choose a reason for hiding this comment

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

If this process is a child of another that doesn't setpgid (e.g. another .NET process that spawns dotnet), then this check will be false. It was to avoid a failing call to kill for the group. If you think that's unnecessary and I should just call kill and check for ESRCH for the group, I'm fine with changing it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering when this would be true, so my comment was more suggesting to remove the whole if block.

Copy link
Author

Choose a reason for hiding this comment

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

Most shells setpgid though, right?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. If I start dotnet from a bash script though, they are not the same.
This may cause subtle differences in behaviour.
If we remove this, things will still work, and should be consistent independent of being process group leader or not.

Copy link
Author

@peterhuene peterhuene Feb 13, 2019

Choose a reason for hiding this comment

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

So unfortunately setpgid must occur before the exec* call (for valid reasons), and I'm not going to duplicate the complex process creation code that the framework does just for this fix.

As such, I think I'll remove the pg kill.

Copy link
Member

Choose a reason for hiding this comment

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

So unfortunately setpgid must occur before the exec* call (for valid reasons), and I'm not going to duplicate the complex process creation code

Setting it before calling Process.Start would be enough.
That said, I think we should keep it simple and not change the process group unless we need to.

Copy link
Author

@peterhuene peterhuene Feb 14, 2019

Choose a reason for hiding this comment

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

Doesn't setpgid require a fork prior to being called? I thought the framework only forks as a result of Process.Start (seems to happen in StartCore in the Unix implementation.

Or did you meant to setpgid the parent dotnet process before spawning the child? Given that executing from a shell usually means we're already in our own process group, setpgid would be ineffectual. In the case where the dotnet process is not in its own group, then it would probably be unwise to break out.

The only consistent way to do it would to be setpgid the child prior to the exec, which we can't do unless CoreFX implements dotnet/corefx#17412.

Copy link
Member

Choose a reason for hiding this comment

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

Or did you meant to setpgid the parent dotnet process before spawning the child?

That is what I meant, so the child would inherit it from the parent.

In the case where the dotnet process is not in its own group, then it would probably be unwise to break out.

The only consistent way to do it would to be setpgid the child prior to the exec, which we can't do unless CoreFX implements dotnet/corefx#17412.

It's more tricky than I thought. If the dotnet process would change its own pgid, it would no longer receive signals (e.g. SIGINT) from the console. So setpgid needs to occur between fork and exec of the child. And the parent needs to take care of propagating the signals to the children.

Copy link
Contributor

@nguerrera nguerrera Feb 15, 2019

Choose a reason for hiding this comment

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

Plus, no managed code can safely run between fork and exec. If we ever need something there, we’d need API changes in the BCL. (Édit: I see that was already pointed out.)

{
if (NativeMethods.Posix.kill(-currentPid, NativeMethods.Posix.SIGTERM) == 0)
{
// Successfully sent the signal to the entire group; don't send again to child
sendChildSIGTERM = false;
}
}

if (sendChildSIGTERM && NativeMethods.Posix.kill(_process.Id, NativeMethods.Posix.SIGTERM) != 0)
Copy link
Member

Choose a reason for hiding this comment

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

If you want to be safe for 'pid recycling', you can add a WaitForExit(0) and do nothing if the process has already exited.

Copy link
Author

Choose a reason for hiding this comment

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

I'll add that. Thanks!

{
// Couldn't send the signal, don't wait
return;
}

// If SIGTERM was ignored by the target, then we'll still wait
_process.WaitForExit();

Environment.ExitCode = _process.ExitCode;
}

private static bool SetKillOnJobClose(IntPtr job, bool value)
{
var information = new NativeMethods.Windows.JobObjectExtendedLimitInformation
{
BasicLimitInformation = new NativeMethods.Windows.JobObjectBasicLimitInformation
{
LimitFlags = (value ? NativeMethods.Windows.JobObjectLimitFlags.JobObjectLimitKillOnJobClose : 0)
}
};

var length = Marshal.SizeOf(typeof(NativeMethods.Windows.JobObjectExtendedLimitInformation));
var informationPtr = Marshal.AllocHGlobal(length);

try
{
Marshal.StructureToPtr(information, informationPtr, false);

if (!NativeMethods.Windows.SetInformationJobObject(
job,
NativeMethods.Windows.JobObjectInfoClass.JobObjectExtendedLimitInformation,
informationPtr,
(uint)length))
{
return false;
}

return true;
}
finally
{
Marshal.FreeHGlobal(informationPtr);
}
}

private Process _process;
private SafeWaitHandle _job;
}
}
23 changes: 15 additions & 8 deletions src/Microsoft.DotNet.Cli.Utils/ProcessStartInfoExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ public static int Execute(this ProcessStartInfo startInfo)
};

process.Start();
process.WaitForExit();

using (new ProcessReaper(process))
{
process.WaitForExit();
}

return process.ExitCode;
}
Expand All @@ -43,16 +47,19 @@ public static int ExecuteAndCaptureOutput(this ProcessStartInfo startInfo, out s

process.Start();

var taskOut = outStream.BeginRead(process.StandardOutput);
var taskErr = errStream.BeginRead(process.StandardError);
using (new ProcessReaper(process))
{
var taskOut = outStream.BeginRead(process.StandardOutput);
var taskErr = errStream.BeginRead(process.StandardError);

process.WaitForExit();
process.WaitForExit();

taskOut.Wait();
taskErr.Wait();
taskOut.Wait();
taskErr.Wait();

stdOut = outStream.CapturedOutput;
stdErr = errStream.CapturedOutput;
stdOut = outStream.CapturedOutput;
stdErr = errStream.CapturedOutput;
}

return process.ExitCode;
}
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.DotNet.Cli.Utils/Properties/AssemblyInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
[assembly: AssemblyMetadataAttribute("Serviceable", "True")]
[assembly: InternalsVisibleTo("dotnet, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("dotnet.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("dotnet-run.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("Microsoft.DotNet.Configurer, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("Microsoft.DotNet.Tools.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
[assembly: InternalsVisibleTo("Microsoft.DotNet.Cli.Utils.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
Expand Down
Loading