From d1c28f998b0b756fdd9ac35bb47491f89324b3c1 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Tue, 5 Feb 2019 15:20:24 -0800 Subject: [PATCH 1/4] Implement child process reaping. This commit implements child process reaping in the event of termination of a CLI command. On Windows, the child process is added to a job object that is set to terminate the child (and its tree) upon the termination of the parent dotnet process. On Windows 7 and Server 2008, the dotnet process cannot already be associated with a job object for the reaping to occur. On later Windows versions, a nested job will be created so the reaping will still occur. After the child process exits, the job object is closed without terminating the remaining processes in the job; this allows for the child process to spawn additional processes that outlive the child. On POSIX operating systems, a SIGTERM is intercepted and forwarded on to the child process only. Like the SIGINT forwarding, it is up to the child process to decide what to do with the SIGTERM signal (the default is to abort). Additionally, this fix further expands upon the previous fix to `dotnet run` to properly handle SIGINT so that all child processes now benefit from the fixed behavior. This means MSBuild-forwarding commands like `dotnet build` now behave as if MSBuild were directly being executed with respect to Ctrl-C handling. Fixes #7426. --- .../TestProjects/TestAppThatWaits/Program.cs | 15 +- src/Microsoft.DotNet.Cli.Utils/Command.cs | 31 ++-- .../NativeMethods.cs | 77 +++++++++ .../ProcessReaper.cs | 149 ++++++++++++++++++ .../ProcessStartInfoExtensions.cs | 23 ++- .../Properties/AssemblyInfo.cs | 1 + .../Commands/TestCommand.cs | 7 +- .../GivenDotnetRunIsInterrupted.cs | 93 ++++++++++- 8 files changed, 361 insertions(+), 35 deletions(-) create mode 100644 src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs create mode 100644 src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs diff --git a/TestAssets/TestProjects/TestAppThatWaits/Program.cs b/TestAssets/TestProjects/TestAppThatWaits/Program.cs index ce6edb7af6..d489210522 100644 --- a/TestAssets/TestProjects/TestAppThatWaits/Program.cs +++ b/TestAssets/TestProjects/TestAppThatWaits/Program.cs @@ -1,5 +1,7 @@ using System; using System.Diagnostics; +using System.IO; +using System.Runtime.InteropServices; using System.Threading; namespace TestAppThatWaits @@ -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; + } } } diff --git a/src/Microsoft.DotNet.Cli.Utils/Command.cs b/src/Microsoft.DotNet.Cli.Utils/Command.cs index 160270e95b..437ddb27b7 100644 --- a/src/Microsoft.DotNet.Cli.Utils/Command.cs +++ b/src/Microsoft.DotNet.Cli.Utils/Command.cs @@ -17,7 +17,7 @@ public class Command : ICommand private readonly Process _process; private StreamForwarder _stdOut; - + private StreamForwarder _stdErr; private bool _running = false; @@ -40,8 +40,6 @@ public CommandResult Execute() _process.EnableRaisingEvents = true; - Console.CancelKeyPress += HandleCancelKeyPress; - #if DEBUG var sw = Stopwatch.StartNew(); @@ -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 @@ -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; - } } } diff --git a/src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs b/src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs new file mode 100644 index 0000000000..03e75d08f0 --- /dev/null +++ b/src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs @@ -0,0 +1,77 @@ +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 kill(int pid, int sig); + + internal const int SIGINT = 2; + internal const int SIGTERM = 15; + } + } +} diff --git a/src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs b/src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs new file mode 100644 index 0000000000..df1a5c09ef --- /dev/null +++ b/src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs @@ -0,0 +1,149 @@ +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 +{ + /// + /// Responsible for reaping a target process if the current process terminates. + /// + /// + /// 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. + /// + internal class ProcessReaper : IDisposable + { + /// + /// Creates a new process reaper. + /// + /// The target process to reap if the current process terminates. The process must already be started. + 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) + { + // Attempt to send a SIGTERM to the process + if (NativeMethods.Posix.kill(_process.Id, NativeMethods.Posix.SIGTERM) != 0) + { + // 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; + } +} diff --git a/src/Microsoft.DotNet.Cli.Utils/ProcessStartInfoExtensions.cs b/src/Microsoft.DotNet.Cli.Utils/ProcessStartInfoExtensions.cs index 0d11313b91..30db24126f 100644 --- a/src/Microsoft.DotNet.Cli.Utils/ProcessStartInfoExtensions.cs +++ b/src/Microsoft.DotNet.Cli.Utils/ProcessStartInfoExtensions.cs @@ -21,7 +21,11 @@ public static int Execute(this ProcessStartInfo startInfo) }; process.Start(); - process.WaitForExit(); + + using (new ProcessReaper(process)) + { + process.WaitForExit(); + } return process.ExitCode; } @@ -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; } diff --git a/src/Microsoft.DotNet.Cli.Utils/Properties/AssemblyInfo.cs b/src/Microsoft.DotNet.Cli.Utils/Properties/AssemblyInfo.cs index b2f285d434..e64b2d0cfa 100644 --- a/src/Microsoft.DotNet.Cli.Utils/Properties/AssemblyInfo.cs +++ b/src/Microsoft.DotNet.Cli.Utils/Properties/AssemblyInfo.cs @@ -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")] diff --git a/test/Microsoft.DotNet.Tools.Tests.Utilities/Commands/TestCommand.cs b/test/Microsoft.DotNet.Tools.Tests.Utilities/Commands/TestCommand.cs index 7a47f48d5d..9e997a43a9 100644 --- a/test/Microsoft.DotNet.Tools.Tests.Utilities/Commands/TestCommand.cs +++ b/test/Microsoft.DotNet.Tools.Tests.Utilities/Commands/TestCommand.cs @@ -23,6 +23,8 @@ public class TestCommand public Process CurrentProcess { get; private set; } + public int TimeoutMiliseconds { get; set; } = Timeout.Infinite; + public Dictionary Environment { get; } = new Dictionary(); public event DataReceivedEventHandler ErrorDataReceived; @@ -115,7 +117,10 @@ private async Task ExecuteAsyncInternal(string executable, string await completionTask; - CurrentProcess.WaitForExit(); + if (!CurrentProcess.WaitForExit(TimeoutMiliseconds)) + { + throw new TimeoutException($"The process failed to exit after {TimeoutMiliseconds / 1000.0} seconds."); + } RemoveNullTerminator(stdOut); diff --git a/test/dotnet-run.Tests/GivenDotnetRunIsInterrupted.cs b/test/dotnet-run.Tests/GivenDotnetRunIsInterrupted.cs index 2c8f8aeda2..6b31f84ff1 100644 --- a/test/dotnet-run.Tests/GivenDotnetRunIsInterrupted.cs +++ b/test/dotnet-run.Tests/GivenDotnetRunIsInterrupted.cs @@ -5,12 +5,16 @@ using System.Diagnostics; using System.Runtime.InteropServices; using FluentAssertions; +using Microsoft.DotNet.Cli.Utils; using Microsoft.DotNet.Tools.Test.Utilities; +using Xunit.Sdk; namespace Microsoft.DotNet.Cli.Run.Tests { public class GivenDotnetRunIsInterrupted : TestBase { + private const int WaitTimeout = 30000; + // This test is Unix only for the same reason that CoreFX does not test Console.CancelKeyPress on Windows // See https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/System.Console/tests/CancelKeyPress.Unix.cs#L63-L67 [UnixOnlyFact] @@ -36,8 +40,8 @@ public void ItIgnoresSIGINT() // will inherit the current process group from the `dotnet test` process that is running this test. // We would need to fork(), setpgid(), and then execve() to break out of the current group and that is // too complex for a simple unit test. - kill(command.CurrentProcess.Id, SIGINT).Should().Be(0); // dotnet run - kill(Convert.ToInt32(e.Data), SIGINT).Should().Be(0); // TestAppThatWaits + NativeMethods.Posix.kill(command.CurrentProcess.Id, NativeMethods.Posix.SIGINT).Should().Be(0); // dotnet run + NativeMethods.Posix.kill(Convert.ToInt32(e.Data), NativeMethods.Posix.SIGINT).Should().Be(0); // TestAppThatWaits killed = true; }; @@ -52,9 +56,88 @@ public void ItIgnoresSIGINT() killed.Should().BeTrue(); } - [DllImport("libc", SetLastError = true)] - private static extern int kill(int pid, int sig); + [UnixOnlyFact] + public void ItPassesSIGTERMToChild() + { + var asset = TestAssets.Get("TestAppThatWaits") + .CreateInstance() + .WithSourceFiles(); + + var command = new RunCommand() + .WithWorkingDirectory(asset.Root.FullName); + + bool killed = false; + Process child = null; + command.OutputDataReceived += (s, e) => + { + if (killed) + { + return; + } + + child = Process.GetProcessById(Convert.ToInt32(e.Data)); + NativeMethods.Posix.kill(command.CurrentProcess.Id, NativeMethods.Posix.SIGTERM).Should().Be(0); + + killed = true; + }; + + command + .ExecuteWithCapturedOutput() + .Should() + .ExitWith(43) + .And + .HaveStdOutContaining("Terminating!"); + + killed.Should().BeTrue(); + + if (!child.WaitForExit(WaitTimeout)) + { + child.Kill(); + throw new XunitException("child process failed to terminate."); + } + } + + [WindowsOnlyFact] + public void ItTerminatesTheChildWhenKilled() + { + var asset = TestAssets.Get("TestAppThatWaits") + .CreateInstance() + .WithSourceFiles(); + + var command = new RunCommand() + .WithWorkingDirectory(asset.Root.FullName); + + bool killed = false; + Process child = null; + command.OutputDataReceived += (s, e) => + { + if (killed) + { + return; + } + + child = Process.GetProcessById(Convert.ToInt32(e.Data)); + command.CurrentProcess.Kill(); + + killed = true; + }; - private const int SIGINT = 2; + // A timeout is required to prevent the `Process.WaitForExit` call to hang if `dotnet run` failed to terminate the child on Windows. + // This is because `Process.WaitForExit()` hangs waiting for the process launched by `dotnet run` to close the redirected I/O pipes (which won't happen). + command.TimeoutMiliseconds = WaitTimeout; + + command + .ExecuteWithCapturedOutput() + .Should() + .ExitWith(-1); + + killed.Should().BeTrue(); + + if (!child.WaitForExit(WaitTimeout)) + { + child.Kill(); + throw new XunitException("child process failed to terminate."); + } + } } } From 70e0913f00c2a92215b7bf0c39bbb1476b8e25f7 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Wed, 6 Feb 2019 16:40:46 -0800 Subject: [PATCH 2/4] Delete unused code. This commit deletes the `DepsJsonCommandFactory`, `ProjectDependenciesCommandFactory`, and `PublishedPathCommandFactory` types as they are not instantiated from product code. --- .../CommandFactory/DepsJsonCommandFactory.cs | 48 ---- .../ProjectDependenciesCommandFactory.cs | 122 ----------- .../PublishedPathCommandFactory.cs | 30 --- ...GivenAProjectDependenciesCommandFactory.cs | 207 ------------------ 4 files changed, 407 deletions(-) delete mode 100644 src/dotnet/CommandFactory/DepsJsonCommandFactory.cs delete mode 100644 src/dotnet/CommandFactory/ProjectDependenciesCommandFactory.cs delete mode 100644 src/dotnet/CommandFactory/PublishedPathCommandFactory.cs delete mode 100644 test/Microsoft.DotNet.CommandFactory.Tests/GivenAProjectDependenciesCommandFactory.cs diff --git a/src/dotnet/CommandFactory/DepsJsonCommandFactory.cs b/src/dotnet/CommandFactory/DepsJsonCommandFactory.cs deleted file mode 100644 index 6f9bd24c3c..0000000000 --- a/src/dotnet/CommandFactory/DepsJsonCommandFactory.cs +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) .NET Foundation and contributors. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -using System.Collections.Generic; -using Microsoft.DotNet.Cli.Utils; -using NuGet.Frameworks; - -namespace Microsoft.DotNet.CommandFactory -{ - public class DepsJsonCommandFactory : ICommandFactory - { - private DepsJsonCommandResolver _depsJsonCommandResolver; - private string _temporaryDirectory; - private string _depsJsonFile; - private string _runtimeConfigFile; - - public DepsJsonCommandFactory( - string depsJsonFile, - string runtimeConfigFile, - string nugetPackagesRoot, - string temporaryDirectory) - { - _depsJsonCommandResolver = new DepsJsonCommandResolver(nugetPackagesRoot); - - _temporaryDirectory = temporaryDirectory; - _depsJsonFile = depsJsonFile; - _runtimeConfigFile = runtimeConfigFile; - } - - public ICommand Create( - string commandName, - IEnumerable args, - NuGetFramework framework = null, - string configuration = Constants.DefaultConfiguration) - { - var commandResolverArgs = new CommandResolverArguments() - { - CommandName = commandName, - CommandArguments = args, - DepsJsonFile = _depsJsonFile - }; - - var commandSpec = _depsJsonCommandResolver.Resolve(commandResolverArgs); - - return CommandFactoryUsingResolver.Create(commandSpec); - } - } -} diff --git a/src/dotnet/CommandFactory/ProjectDependenciesCommandFactory.cs b/src/dotnet/CommandFactory/ProjectDependenciesCommandFactory.cs deleted file mode 100644 index 7ab99b6ff8..0000000000 --- a/src/dotnet/CommandFactory/ProjectDependenciesCommandFactory.cs +++ /dev/null @@ -1,122 +0,0 @@ -// Copyright (c) .NET Foundation and contributors. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -using System.Collections.Generic; -using Microsoft.DotNet.Cli.Utils; -using Microsoft.DotNet.PlatformAbstractions; -using NuGet.Frameworks; - -namespace Microsoft.DotNet.CommandFactory -{ - public class ProjectDependenciesCommandFactory : ICommandFactory - { - private readonly NuGetFramework _nugetFramework; - private readonly string _configuration; - private readonly string _outputPath; - private readonly string _buildBasePath; - private readonly string _projectDirectory; - - public ProjectDependenciesCommandFactory( - NuGetFramework nugetFramework, - string configuration, - string outputPath, - string buildBasePath, - string projectDirectory) - { - _nugetFramework = nugetFramework; - _configuration = configuration; - _outputPath = outputPath; - _buildBasePath = buildBasePath; - _projectDirectory = projectDirectory; - - if (_configuration == null) - { - _configuration = Constants.DefaultConfiguration; - } - } - - public ICommand Create( - string commandName, - IEnumerable args, - NuGetFramework framework = null, - string configuration = null) - { - if (string.IsNullOrEmpty(configuration)) - { - configuration = _configuration; - } - - if (framework == null) - { - framework = _nugetFramework; - } - - var commandSpec = FindProjectDependencyCommands( - commandName, - args, - configuration, - framework, - _outputPath, - _buildBasePath, - _projectDirectory); - - return CommandFactoryUsingResolver.Create(commandSpec); - } - - private CommandSpec FindProjectDependencyCommands( - string commandName, - IEnumerable commandArgs, - string configuration, - NuGetFramework framework, - string outputPath, - string buildBasePath, - string projectDirectory) - { - var commandResolverArguments = new CommandResolverArguments - { - CommandName = commandName, - CommandArguments = commandArgs, - Framework = framework, - Configuration = configuration, - OutputPath = outputPath, - BuildBasePath = buildBasePath, - ProjectDirectory = projectDirectory - }; - - var commandResolver = GetProjectDependenciesCommandResolver(framework); - - var commandSpec = commandResolver.Resolve(commandResolverArguments); - if (commandSpec == null) - { - throw new CommandUnknownException(commandName); - } - - return commandSpec; - } - - private ICommandResolver GetProjectDependenciesCommandResolver(NuGetFramework framework) - { - var environment = new EnvironmentProvider(); - - if (framework.IsDesktop()) - { - IPlatformCommandSpecFactory platformCommandSpecFactory = null; - if (RuntimeEnvironment.OperatingSystemPlatform == Platform.Windows) - { - platformCommandSpecFactory = new WindowsExePreferredCommandSpecFactory(); - } - else - { - platformCommandSpecFactory = new GenericPlatformCommandSpecFactory(); - } - - return new OutputPathCommandResolver(environment, platformCommandSpecFactory); - } - else - { - var packagedCommandSpecFactory = new PackagedCommandSpecFactory(); - return new ProjectDependenciesCommandResolver(environment, packagedCommandSpecFactory); - } - } - } -} diff --git a/src/dotnet/CommandFactory/PublishedPathCommandFactory.cs b/src/dotnet/CommandFactory/PublishedPathCommandFactory.cs deleted file mode 100644 index efe5c9d4e7..0000000000 --- a/src/dotnet/CommandFactory/PublishedPathCommandFactory.cs +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (c) .NET Foundation and contributors. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -using System.Collections.Generic; -using Microsoft.DotNet.Cli.Utils; -using NuGet.Frameworks; - -namespace Microsoft.DotNet.CommandFactory -{ - public class PublishedPathCommandFactory : ICommandFactory - { - private readonly string _publishDirectory; - private readonly string _applicationName; - - public PublishedPathCommandFactory(string publishDirectory, string applicationName) - { - _publishDirectory = publishDirectory; - _applicationName = applicationName; - } - - public ICommand Create( - string commandName, - IEnumerable args, - NuGetFramework framework = null, - string configuration = Constants.DefaultConfiguration) - { - return CommandFactoryUsingResolver.Create(commandName, args, framework, configuration, _publishDirectory, _applicationName); - } - } -} diff --git a/test/Microsoft.DotNet.CommandFactory.Tests/GivenAProjectDependenciesCommandFactory.cs b/test/Microsoft.DotNet.CommandFactory.Tests/GivenAProjectDependenciesCommandFactory.cs deleted file mode 100644 index d579a245d0..0000000000 --- a/test/Microsoft.DotNet.CommandFactory.Tests/GivenAProjectDependenciesCommandFactory.cs +++ /dev/null @@ -1,207 +0,0 @@ -// Copyright (c) .NET Foundation and contributors. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -using System; -using System.IO; -using FluentAssertions; -using Microsoft.DotNet.TestFramework; -using Microsoft.DotNet.CommandFactory; -using Microsoft.DotNet.Tools.Test.Utilities; -using NuGet.Frameworks; -using Xunit; -using Microsoft.DotNet.Tools.Tests.Utilities; -using Microsoft.DotNet.Cli.Utils; - -namespace Microsoft.DotNet.Tests -{ - public class GivenAProjectDependenciesCommandFactory : TestBase - { - private static readonly NuGetFramework s_desktopTestFramework = FrameworkConstants.CommonFrameworks.Net451; - - private RepoDirectoriesProvider _repoDirectoriesProvider; - - public GivenAProjectDependenciesCommandFactory() - { - _repoDirectoriesProvider = new RepoDirectoriesProvider(); - Environment.SetEnvironmentVariable( - Constants.MSBUILD_EXE_PATH, - Path.Combine(_repoDirectoriesProvider.Stage2Sdk, "MSBuild.dll")); - } - - [WindowsOnlyFact] - public void It_resolves_desktop_apps_defaulting_to_Debug_Configuration() - { - var configuration = "Debug"; - - var testInstance = TestAssets.Get(TestAssetKinds.DesktopTestProjects, "AppWithProjTool2Fx") - .CreateInstance() - .WithSourceFiles() - .WithNuGetConfig(_repoDirectoriesProvider.TestPackages); - - var restoreCommand = new RestoreCommand() - .WithWorkingDirectory(testInstance.Root) - .ExecuteWithCapturedOutput() - .Should().Pass(); - - var buildCommand = new BuildCommand() - .WithWorkingDirectory(testInstance.Root) - .WithConfiguration(configuration) - .WithCapturedOutput() - .Execute() - .Should().Pass(); - - var factory = new ProjectDependenciesCommandFactory( - s_desktopTestFramework, - null, - null, - null, - testInstance.Root.FullName); - - var command = factory.Create("dotnet-desktop-and-portable", null); - - command.CommandName.Should().Contain(testInstance.Root.GetDirectory("bin", configuration).FullName); - - Path.GetFileName(command.CommandName).Should().Be("dotnet-desktop-and-portable.exe"); - } - - [WindowsOnlyFact] - public void It_resolves_desktop_apps_when_configuration_is_Debug() - { - var configuration = "Debug"; - - var testInstance = TestAssets.Get(TestAssetKinds.DesktopTestProjects, "AppWithProjTool2Fx") - .CreateInstance() - .WithSourceFiles() - .WithNuGetConfig(_repoDirectoriesProvider.TestPackages); - - var restoreCommand = new RestoreCommand() - .WithWorkingDirectory(testInstance.Root) - .ExecuteWithCapturedOutput() - .Should().Pass(); - - var buildCommand = new BuildCommand() - .WithWorkingDirectory(testInstance.Root) - .WithConfiguration(configuration) - .Execute() - .Should().Pass(); - - var factory = new ProjectDependenciesCommandFactory( - s_desktopTestFramework, - configuration, - null, - null, - testInstance.Root.FullName); - - var command = factory.Create("dotnet-desktop-and-portable", null); - - command.CommandName.Should().Contain(testInstance.Root.GetDirectory("bin", configuration).FullName); - Path.GetFileName(command.CommandName).Should().Be("dotnet-desktop-and-portable.exe"); - } - - [WindowsOnlyFact] - public void It_resolves_desktop_apps_when_configuration_is_Release() - { - var configuration = "Debug"; - - var testInstance = TestAssets.Get(TestAssetKinds.DesktopTestProjects, "AppWithProjTool2Fx") - .CreateInstance() - .WithSourceFiles() - .WithNuGetConfig(_repoDirectoriesProvider.TestPackages); - - var restoreCommand = new RestoreCommand() - .WithWorkingDirectory(testInstance.Root) - .ExecuteWithCapturedOutput() - .Should().Pass(); - - var buildCommand = new BuildCommand() - .WithWorkingDirectory(testInstance.Root) - .WithConfiguration(configuration) - .WithCapturedOutput() - .Execute() - .Should().Pass(); - - var factory = new ProjectDependenciesCommandFactory( - s_desktopTestFramework, - configuration, - null, - null, - testInstance.Root.FullName); - - var command = factory.Create("dotnet-desktop-and-portable", null); - - command.CommandName.Should().Contain(testInstance.Root.GetDirectory("bin", configuration).FullName); - - Path.GetFileName(command.CommandName).Should().Be("dotnet-desktop-and-portable.exe"); - } - - [WindowsOnlyFact] - public void It_resolves_desktop_apps_using_configuration_passed_to_create() - { - var configuration = "Debug"; - - var testInstance = TestAssets.Get(TestAssetKinds.DesktopTestProjects, "AppWithProjTool2Fx") - .CreateInstance() - .WithSourceFiles() - .WithNuGetConfig(_repoDirectoriesProvider.TestPackages); - - var restoreCommand = new RestoreCommand() - .WithWorkingDirectory(testInstance.Root) - .ExecuteWithCapturedOutput() - .Should().Pass(); - - var buildCommand = new BuildCommand() - .WithWorkingDirectory(testInstance.Root) - .WithConfiguration(configuration) - .WithCapturedOutput() - .Execute() - .Should().Pass(); - - var factory = new ProjectDependenciesCommandFactory( - s_desktopTestFramework, - "Debug", - null, - null, - testInstance.Root.FullName); - - var command = factory.Create("dotnet-desktop-and-portable", null, configuration: configuration); - - command.CommandName.Should().Contain(testInstance.Root.GetDirectory("bin", configuration).FullName); - - Path.GetFileName(command.CommandName).Should().Be("dotnet-desktop-and-portable.exe"); - } - - [Fact] - public void It_resolves_tools_whose_package_name_is_different_than_dll_name() - { - Environment.SetEnvironmentVariable( - Constants.MSBUILD_EXE_PATH, - Path.Combine(new RepoDirectoriesProvider().Stage2Sdk, "MSBuild.dll")); - - var configuration = "Debug"; - - var testInstance = TestAssets.Get("AppWithDirectDepWithOutputName") - .CreateInstance() - .WithSourceFiles() - .WithRestoreFiles(); - - var buildCommand = new BuildCommand() - .WithProjectDirectory(testInstance.Root) - .WithConfiguration(configuration) - .WithCapturedOutput() - .Execute() - .Should().Pass(); - - var factory = new ProjectDependenciesCommandFactory( - NuGetFrameworks.NetCoreApp30, - configuration, - null, - null, - testInstance.Root.FullName); - - var command = factory.Create("dotnet-tool-with-output-name", null); - - command.CommandArgs.Should().Contain( - Path.Combine("toolwithoutputname", "1.0.0", "lib", "netcoreapp3.0", "dotnet-tool-with-output-name.dll")); - } - } -} From 2f8193fa98a2bc5542e5fd05bfb5ad7d691376f1 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Thu, 7 Feb 2019 13:14:42 -0800 Subject: [PATCH 3/4] Send SIGTERM to process group. This commit implements sending SIGTERM to the process group if the current process is the root of a group. This ensures that all descendant processes receive the SIGTERM signal. If the current process is not at the root (e.g. a child of another root), then only the direct child will be sent the SIGTERM signal. --- .../NativeMethods.cs | 3 +++ .../ProcessReaper.cs | 18 ++++++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs b/src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs index 03e75d08f0..f14b512875 100644 --- a/src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs +++ b/src/Microsoft.DotNet.Cli.Utils/NativeMethods.cs @@ -67,6 +67,9 @@ internal struct JobObjectExtendedLimitInformation 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); diff --git a/src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs b/src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs index df1a5c09ef..90e1e4bfc3 100644 --- a/src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs +++ b/src/Microsoft.DotNet.Cli.Utils/ProcessReaper.cs @@ -97,8 +97,21 @@ private static SafeWaitHandle AssignProcessToJobObject(IntPtr process) private void HandleProcessExit(object sender, EventArgs args) { - // Attempt to send a SIGTERM to the process - if (NativeMethods.Posix.kill(_process.Id, NativeMethods.Posix.SIGTERM) != 0) + 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) + { + 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) { // Couldn't send the signal, don't wait return; @@ -143,6 +156,7 @@ private static bool SetKillOnJobClose(IntPtr job, bool value) Marshal.FreeHGlobal(informationPtr); } } + private Process _process; private SafeWaitHandle _job; } From 68e3d217c0411bfef5e2a2e1dd3a06db79396f12 Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Thu, 7 Feb 2019 16:40:13 -0800 Subject: [PATCH 4/4] Fix race in DotnetUnderTest.FullPath on Windows. There's a race in `DotnetUnderTest.FullPath` on Windows that may result in the path to dotnet to have a `.exe.exe` suffix when running tests in parallel. The root of the problem is that the method is not thread safe and relies on static state to calculate the result; the state may have already changed as a result of another thread of execution. Rather than lock or otherwise change this code, this fix simply removes the dependency on the static state to do the calculation. The resulting method is still not thread safe, but now no longer matters if the full path is calculated concurrently. --- .../DotnetUnderTest.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/test/Microsoft.DotNet.Tools.Tests.Utilities/DotnetUnderTest.cs b/test/Microsoft.DotNet.Tools.Tests.Utilities/DotnetUnderTest.cs index 705607f207..7175e9aa45 100644 --- a/test/Microsoft.DotNet.Tools.Tests.Utilities/DotnetUnderTest.cs +++ b/test/Microsoft.DotNet.Tools.Tests.Utilities/DotnetUnderTest.cs @@ -26,12 +26,7 @@ public static string FullName { _pathToDotnetUnderTest = Path.Combine( new RepoDirectoriesProvider().DotnetRoot, - "dotnet"); - - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - _pathToDotnetUnderTest = $"{_pathToDotnetUnderTest}.exe"; - } + $"dotnet{Constants.ExeSuffix}"); } return _pathToDotnetUnderTest;