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..784804bd995034 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs @@ -81,6 +81,31 @@ private void AssertNonZeroAllZeroDarwin(long value) } } + private static void AssertRemoteProcessStandardOutputLine(RemoteInvokeHandle remoteHandle, string expectedMessage, int timeout) + { + using CancellationTokenSource cts = new(timeout); + + Task readTask = remoteHandle.Process.StandardOutput.ReadLineAsync(cts.Token).AsTask(); + string? line; + try + { + line = readTask.ConfigureAwait(false).GetAwaiter().GetResult(); + } + catch (OperationCanceledException) + { + throw new XunitException($"Expected message '{expectedMessage}' was not observed on remote process within specified time."); + } + + if (line != null) + { + Assert.Equal(expectedMessage, line); + } + else + { + throw new XunitException($"StandardOutput was closed before observing expected message '{expectedMessage}'"); + } + } + public static IEnumerable SignalTestData() { if (OperatingSystem.IsWindows()) @@ -141,10 +166,7 @@ public void TestCreateNewProcessGroup_HandlerReceivesExpectedSignal(PosixSignal arg: $"{signal}", remoteInvokeOptions); - while (!remoteHandle.Process.StandardOutput.ReadLine().EndsWith(PosixSignalRegistrationCreatedMessage)) - { - Thread.Sleep(20); - } + AssertRemoteProcessStandardOutputLine(remoteHandle, PosixSignalRegistrationCreatedMessage, WaitInMS); try { @@ -161,6 +183,100 @@ public void TestCreateNewProcessGroup_HandlerReceivesExpectedSignal(PosixSignal } } + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + // Limited only to signals which terminates process by default, and are supported on all platforms 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."; + const int UnterminatedExitCode = -1; + + var remoteInvokeOptions = new RemoteInvokeOptions { CheckExitCode = false }; + remoteInvokeOptions.StartInfo.RedirectStandardOutput = true; + if (OperatingSystem.IsWindows()) + { + 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 UnterminatedExitCode; + }, + 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); + + // https://github.com/dotnet/runtime/issues/125733 + if (PlatformDetection.IsMonoRuntime && signal == PosixSignal.SIGQUIT && !PlatformDetection.IsWindows) + { + SendSignal(PosixSignal.SIGTERM, remoteHandle.Process.Id); + } + + Assert.True(remoteHandle.Process.WaitForExit(WaitInMS)); + Assert.True(remoteHandle.Process.StandardOutput.EndOfStream); + if (OperatingSystem.IsWindows()) + { + Assert.Equal(unchecked((int)0xC000013A), remoteHandle.Process.ExitCode); // STATUS_CONTROL_C_EXIT + } + else + { + // Signal numbers are platform dependent, so we can't check exact exit code + Assert.NotEqual(0, remoteHandle.Process.ExitCode); + Assert.NotEqual(UnterminatedExitCode, remoteHandle.Process.ExitCode); + } + } + finally + { + // If sending the signal fails or process did not exit on its own, we want to kill the process ASAP + // to prevent RemoteExecutor's timeout from hiding it. + remoteHandle.Process.Kill(); + } + } + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] [InlineData(-2)] [InlineData((long)int.MaxValue + 1)] 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); - } - } } } }