diff --git a/src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.netcoreapp.cs b/src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.netcoreapp.cs index c5be9291ffd44c..71c72b3564f1ec 100644 --- a/src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.netcoreapp.cs +++ b/src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.netcoreapp.cs @@ -38,11 +38,7 @@ private void HandleWindowsShutdown(PosixSignalContext context) // for SIGTERM on Windows we must block this thread until the application is finished // otherwise the process will be killed immediately on return from this handler - // don't allow Dispose to unregister handlers, since Windows has a lock that prevents the unregistration while this handler is running - // just leak these, since the process is exiting - _sigIntRegistration = null; - _sigQuitRegistration = null; - _sigTermRegistration = null; + UnregisterShutdownHandlers(); ApplicationLifetime.StopApplication(); diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs index d89f422a2065d1..8ccd4682d31b80 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -81,6 +81,33 @@ private void AssertNonZeroAllZeroDarwin(long value) } } + private static void AssertRemoteProcessStandardOutputLine(RemoteInvokeHandle remoteHandle, string expectedMessage, int timeout) + { + using CancellationTokenSource cts = new(); + + Task readTask = remoteHandle.Process.StandardOutput.ReadLineAsync(cts.Token).AsTask(); + Task timeoutTask = Task.Delay(timeout, cts.Token); + + Task finishedTask = Task.WhenAny(readTask, timeoutTask).ConfigureAwait(false).GetAwaiter().GetResult(); + cts.Cancel(); // cancel the slower one + + if (finishedTask == readTask) + { + if (readTask.Result != null) + { + Assert.Equal(expectedMessage, readTask.Result); + } + else + { + throw new XunitException($"StandardOutput was closed before observing expected message '{expectedMessage}'"); + } + } + else + { + throw new XunitException($"Expected message '{expectedMessage}' was not observed on remote process within specified time."); + } + } + public static IEnumerable SignalTestData() { if (OperatingSystem.IsWindows()) @@ -141,17 +168,99 @@ public void TestCreateNewProcessGroup_HandlerReceivesExpectedSignal(PosixSignal arg: $"{signal}", remoteInvokeOptions); - while (!remoteHandle.Process.StandardOutput.ReadLine().EndsWith(PosixSignalRegistrationCreatedMessage)) + AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalRegistrationCreatedMessage, WaitInMS); + + try + { + SendSignal(signal, remoteHandle.Process.Id); + + Assert.True(remoteHandle.Process.WaitForExit(WaitInMS)); + Assert.Equal(0, remoteHandle.Process.ExitCode); + } + finally + { + // If sending the signal fails, we want to kill the process ASAP + // to prevent RemoteExecutor's timeout from hiding it. + remoteHandle.Process.Kill(); + } + } + + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + // Limited only to signals which terminates process by default, and are supported on all platfroms by SendSignal + [InlineData(PosixSignal.SIGINT)] + [InlineData(PosixSignal.SIGQUIT)] + public void SignalHandler_CanDisposeInHandler(PosixSignal signal) + { + const string PosixSignalRegistrationCreatedMessage = "PosixSignalRegistration created."; + const string PosixSignalHandlerStartedMessage = "PosixSignalRegistration handler started."; + const string PosixSignalHandlerDisposedMessage = "PosixSignalRegistration disposed."; + + var remoteInvokeOptions = new RemoteInvokeOptions { CheckExitCode = false }; + remoteInvokeOptions.StartInfo.RedirectStandardOutput = true; + if (OperatingSystem.IsWindows()) { - Thread.Sleep(20); + remoteInvokeOptions.StartInfo.CreateNewProcessGroup = true; } + using RemoteInvokeHandle remoteHandle = RemoteExecutor.Invoke( + (signalStr) => + { + PosixSignal expectedSignal = Enum.Parse(signalStr); + using ManualResetEvent receivedSignalEvent = new ManualResetEvent(false); + ReEnableCtrlCHandlerIfNeeded(expectedSignal); + + PosixSignalRegistration? p = null; + p = PosixSignalRegistration.Create(expectedSignal, (ctx) => + { + Console.WriteLine(PosixSignalHandlerStartedMessage); + Assert.Equal(expectedSignal, ctx.Signal); + + Assert.NotNull(p); + p?.Dispose(); + + // Used for checking that Dispose returned and did not get stuck + Console.WriteLine(PosixSignalHandlerDisposedMessage); + + receivedSignalEvent.Set(); + ctx.Cancel = true; + }); + + Console.WriteLine(PosixSignalRegistrationCreatedMessage); + + // Wait for signal which unregisters itself + Assert.True(receivedSignalEvent.WaitOne(WaitInMS)); + + // Wait for second signal which should terminate process by default system handler + Thread.Sleep(WaitInMS); + + // If we did not terminated yet, return failure exit code + return -1; + }, + arg: $"{signal}", + remoteInvokeOptions); + + try { + AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalRegistrationCreatedMessage, WaitInMS); + + SendSignal(signal, remoteHandle.Process.Id); + + AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalHandlerStartedMessage, WaitInMS); + AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalHandlerDisposedMessage, WaitInMS); + SendSignal(signal, remoteHandle.Process.Id); Assert.True(remoteHandle.Process.WaitForExit(WaitInMS)); - Assert.Equal(0, remoteHandle.Process.ExitCode); + Assert.True(remoteHandle.Process.StandardOutput.EndOfStream); + if (OperatingSystem.IsWindows()) + { + Assert.Equal(unchecked((int)0xC000013A), remoteHandle.Process.ExitCode); // STATUS_CONTROL_C_EXIT + } + else + { + Assert.Equal(128 + (int)signal, remoteHandle.Process.ExitCode); + } } finally { diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs index b176506f042af6..e5fbd3ceee16de 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs @@ -10,6 +10,18 @@ public sealed partial class PosixSignalRegistration { private static readonly Dictionary> s_registrations = new(); + /// + /// Serializes concurrent calls to make the emptiness check and + /// the subsequent token insertion atomic. + /// + private static readonly object s_registerLock = new(); + + /// + /// Runtime can generate multiple addresses to the same function. To ensure that registering and + /// unregistering always use the same instance, we capture it in this static field. + /// + private static readonly unsafe delegate* unmanaged s_handlerRoutineAddr = &HandlerRoutine; + private static unsafe PosixSignalRegistration Register(PosixSignal signal, Action handler) { int signo = signal switch @@ -24,26 +36,56 @@ private static unsafe PosixSignalRegistration Register(PosixSignal signal, Actio var token = new Token(signal, signo, handler); var registration = new PosixSignalRegistration(token); - lock (s_registrations) + lock (s_registerLock) { - if (s_registrations.Count == 0 && - !Interop.Kernel32.SetConsoleCtrlHandler(&HandlerRoutine, Add: true)) + bool registerCtrlHandler = false; + lock (s_registrations) { - throw Win32Marshal.GetExceptionForLastWin32Error(); + if (s_registrations.Count == 0) + { + registerCtrlHandler = true; + } } - if (!s_registrations.TryGetValue(signo, out List? tokens)) + // All SetConsoleCtrlHandler calls must happen outside s_registrations locked section + // otherwise we risk AB/BA deadlock between it and internal critical section in OS. + + if (registerCtrlHandler) { - s_registrations[signo] = tokens = new List(); + // User may reset registrations externally by direct calls of Free/Attach/AllocConsole. + // We do not know if it is currently registered or not. To prevent duplicate + // registration, we try unregister existing one first. + if (!Interop.Kernel32.SetConsoleCtrlHandler(s_handlerRoutineAddr, Add: false)) + { + // Returns ERROR_INVALID_PARAMETER if it was not registered. Throw for everything else. + int error = Marshal.GetLastPInvokeError(); + if (error != Interop.Errors.ERROR_INVALID_PARAMETER) + { + throw Win32Marshal.GetExceptionForWin32Error(error); + } + } + + if (!Interop.Kernel32.SetConsoleCtrlHandler(s_handlerRoutineAddr, Add: true)) + { + throw Win32Marshal.GetExceptionForLastWin32Error(); + } } - tokens.Add(token); + lock (s_registrations) + { + if (!s_registrations.TryGetValue(signo, out List? tokens)) + { + s_registrations[signo] = tokens = new List(); + } + + tokens.Add(token); + } } return registration; } - private unsafe void Unregister() + private void Unregister() { lock (s_registrations) { @@ -58,19 +100,6 @@ private unsafe void Unregister() { s_registrations.Remove(token.SigNo); } - - if (s_registrations.Count == 0 && - !Interop.Kernel32.SetConsoleCtrlHandler(&HandlerRoutine, Add: false)) - { - // Ignore errors due to the handler no longer being registered; this can happen, for example, with - // direct use of Alloc/Attach/FreeConsole which result in the table of control handlers being reset. - // Throw for everything else. - int error = Marshal.GetLastPInvokeError(); - if (error != Interop.Errors.ERROR_INVALID_PARAMETER) - { - throw Win32Marshal.GetExceptionForWin32Error(error); - } - } } } }