From 2aff93b0ca95f77e31ac18e1975490398f4e17dc Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 20 Mar 2019 21:22:25 -0400 Subject: [PATCH] Make Process.{Safe}Handle be a waitable event handle on Unix The documentation for Process.{Safe}Handle state that its value can be used to initialize a SafeWaitHandle that can then be used with a method like WaitHandle.WaitAny. However, on Unix, the SafeProcessHandle we currently return from Process.SafeHandle just uses the process ID as its faux handle value, since on Unix there isn't actually an OS process handle that's waitable in this fashion. We already have to manufacture a ManualResetEvent to serve as such a wait handle, though. as we use that for WaitForExit, raising the Exited event, and so on. As such, we can change SafeProcessHandle to be backed by that MRE's handle rather than by the process ID. --- .../SafeHandles/SafeProcessHandle.Unix.cs | 30 ++++++++++++------- .../SafeHandles/SafeProcessHandle.Windows.cs | 2 -- .../Win32/SafeHandles/SafeProcessHandle.cs | 4 +-- .../src/System/Diagnostics/Process.Unix.cs | 8 +++-- .../src/System/Diagnostics/Process.cs | 6 ++-- .../System/Diagnostics/ProcessManager.Unix.cs | 2 +- .../tests/ProcessTests.cs | 19 ++++++++++++ .../tests/System/ExitCodeTests.Unix.cs | 4 +-- 8 files changed, 50 insertions(+), 25 deletions(-) diff --git a/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs b/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs index eff131c23ce2..e25e6d95fb38 100644 --- a/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs +++ b/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs @@ -12,29 +12,37 @@ ===========================================================*/ using System; -using System.Runtime.InteropServices; -using System.Security; namespace Microsoft.Win32.SafeHandles { public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid { - private const int DefaultInvalidHandleValue = -1; + // On Windows, SafeProcessHandle represents the actual OS handle for the process. + // On Unix, there's no such concept. Instead, the implementation manufactures + // a WaitHandle that it manually sets when the process completes; SafeProcessHandle + // then just wraps that same WaitHandle instance. This allows consumers that use + // Process.{Safe}Handle to initalize and use a WaitHandle to successfull use it on + // Unix as well to wait for the process to complete. - internal SafeProcessHandle(int processId) : - this((IntPtr)processId) - { - } + private readonly SafeWaitHandle _handle; + private readonly bool _releaseRef; - public override bool IsInvalid + internal SafeProcessHandle(int processId, SafeWaitHandle handle) : + this(handle.DangerousGetHandle(), ownsHandle: false) { - get { return ((int)handle) < 0; } // Unix processes have non-negative ID values + ProcessId = processId; + _handle = handle; + handle.DangerousAddRef(ref _releaseRef); } + internal int ProcessId { get; } + protected override bool ReleaseHandle() { - // Nop. We don't actually hold handles to a process, as there's no equivalent - // concept on Unix. SafeProcessHandle justs wrap a process ID. + if (_releaseRef) + { + _handle.DangerousRelease(); + } return true; } } diff --git a/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs b/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs index e1b274a60823..e886b245bc63 100644 --- a/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs +++ b/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs @@ -19,8 +19,6 @@ namespace Microsoft.Win32.SafeHandles { public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid { - private const int DefaultInvalidHandleValue = 0; - protected override bool ReleaseHandle() { return Interop.Kernel32.CloseHandle(handle); diff --git a/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.cs b/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.cs index 471ddfd8cd86..b2713b80dff0 100644 --- a/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.cs +++ b/src/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.cs @@ -13,8 +13,6 @@ using System; using System.Diagnostics; -using System.Runtime.InteropServices; -using System.Security; namespace Microsoft.Win32.SafeHandles { @@ -23,7 +21,7 @@ public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvali internal static readonly SafeProcessHandle InvalidHandle = new SafeProcessHandle(); internal SafeProcessHandle() - : this(new IntPtr(DefaultInvalidHandleValue)) + : this(IntPtr.Zero) { } diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 590a8eced78f..c6cc13236698 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -159,7 +159,8 @@ private void EnsureWatchingForExit() { if (!_watchingForExit) { - Debug.Assert(_haveProcessHandle, "Process.EnsureWatchingForExit called with no process handle"); + Debug.Assert(_waitHandle == null); + Debug.Assert(_registeredWaitHandle == null); Debug.Assert(Associated, "Process.EnsureWatchingForExit called with no associated process"); _watchingForExit = true; try @@ -334,7 +335,8 @@ private SafeProcessHandle GetProcessHandle() } EnsureState(State.HaveNonExitedId | State.IsLocal); - return new SafeProcessHandle(_processId); + EnsureWatchingForExit(); + return new SafeProcessHandle(_processId, _waitHandle.SafeWaitHandle); } /// @@ -491,7 +493,7 @@ private bool ForkAndExecProcess( // Store the child's information into this Process object. Debug.Assert(childPid >= 0); SetProcessId(childPid); - SetProcessHandle(new SafeProcessHandle(childPid)); + SetProcessHandle(GetProcessHandle()); return true; } diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index a7b8a24f1836..efb5afa08a75 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs @@ -119,7 +119,7 @@ public SafeProcessHandle SafeHandle get { EnsureState(State.Associated); - return OpenProcessHandle(); + return GetOrOpenProcessHandle(); } } @@ -657,7 +657,7 @@ public bool EnableRaisingEvents { if (value) { - OpenProcessHandle(); + GetOrOpenProcessHandle(); EnsureWatchingForExit(); } else @@ -1130,7 +1130,7 @@ public void Refresh() /// Opens a long-term handle to the process, with all access. If a handle exists, /// then it is reused. If the process has exited, it throws an exception. /// - private SafeProcessHandle OpenProcessHandle() + private SafeProcessHandle GetOrOpenProcessHandle() { if (!_haveProcessHandle) { diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Unix.cs index 6621d9afe749..5209a1bbc53b 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Unix.cs @@ -56,7 +56,7 @@ public static int[] GetProcessIds(string machineName) /// The process ID. public static int GetProcessIdFromHandle(SafeProcessHandle processHandle) { - return (int)processHandle.DangerousGetHandle(); // not actually dangerous; just wraps a process ID + return processHandle.ProcessId; } private static bool IsRemoteMachineCore(string machineName) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.cs b/src/System.Diagnostics.Process/tests/ProcessTests.cs index d7994ed87679..290d763048ab 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.cs @@ -12,6 +12,7 @@ using System.Security; using System.Text; using System.Threading; +using Microsoft.Win32.SafeHandles; using Xunit; using Xunit.Sdk; @@ -946,6 +947,24 @@ public void TestSafeHandle() Assert.False(_process.SafeHandle.IsInvalid); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Handle_CreateEvent_BlocksUntilProcessCompleted(bool useSafeHandle) + { + using (RemoteInvokeHandle h = RemoteInvoke(() => Console.ReadLine(), new RemoteInvokeOptions { StartInfo = new ProcessStartInfo() { RedirectStandardInput = true } })) + using (var mre = new ManualResetEvent(false)) + { + mre.SetSafeWaitHandle(new SafeWaitHandle(useSafeHandle ? h.Process.SafeHandle.DangerousGetHandle() : h.Process.Handle, ownsHandle: false)); + + Assert.False(mre.WaitOne(millisecondsTimeout: 0), "Event should not yet have been set."); + + h.Process.StandardInput.WriteLine(); // allow child to complete + + Assert.True(mre.WaitOne(FailWaitTimeoutMilliseconds), "Event should have been set."); + } + } + [Fact] public void SafeHandle_GetNotStarted_ThrowsInvalidOperationException() { diff --git a/src/System.Runtime/tests/System/ExitCodeTests.Unix.cs b/src/System.Runtime/tests/System/ExitCodeTests.Unix.cs index 6a9671dffc12..d96e56592894 100644 --- a/src/System.Runtime/tests/System/ExitCodeTests.Unix.cs +++ b/src/System.Runtime/tests/System/ExitCodeTests.Unix.cs @@ -49,7 +49,7 @@ public void SigTermExitCode(int? exitCodeOnSigterm) Assert.Equal("Application started", processOutput); // Send SIGTERM - int rv = kill(process.Handle.ToInt32(), SIGTERM); + int rv = kill(process.Id, SIGTERM); Assert.Equal(0, rv); // Process exits in a timely manner @@ -61,4 +61,4 @@ public void SigTermExitCode(int? exitCodeOnSigterm) } } } -} \ No newline at end of file +}