Avoid Blocking Finalizer Thread During Shutdown in SystemEvents (#108489

) * Add switch to enable legacy Thread.Join in SystemEvents.Shutdown and obsolete EventsThreadShutdown * code cleanup * Add regression test * Call PostQuitMessage instead of PostMessage(WM_QUIT) * Remove compat switch and address PR feedback * rebuild with /p:ApiCompatGenerateSuppressionFile=true * Call PostQuitMessage on SystemEvents thread * Try removing left/right on compatbilitysuppression.xml * Obsolete EventsThreadShutdown * Remove shutdown handling to make behavior deterministic * Adjust obsolete message Co-authored-by: Jan Kotas <[email protected]> * Add obsoletion to refs * Set IncludeInternalObsoleteAttribute in ref project * re-include Obsoletions.cs in ref project * revert 134aca8 and hardcode obsoletion * Add IncludeInternalObsoleteAttribute to System.Events project * remove Obsoletions.cs from compile include * Revert "remove Obsoletions.cs from compile include" This reverts commit 452146d. * Update suppression file * Update obsolete message Co-authored-by: Stephen Toub <[email protected]> --------- Co-authored-by: Jan Kotas <[email protected]> Co-authored-by: Viktor Hofer <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
dotnet · Nov 13, 2024 · 6d3f9b5 · 6d3f9b5
1 parent 15b3ca6
commit 6d3f9b5
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 115 deletions.
diff --git a/docs/project/list-of-diagnostics.md b/docs/project/list-of-diagnostics.md
@@ -113,6 +113,7 @@ The PR that reveals the implementation of the `<IncludeInternalObsoleteAttribute
 |  __`SYSLIB0056`__ |  LoadFrom with a custom AssemblyHashAlgorithm is obsolete. Use overloads without an AssemblyHashAlgorithm. |
 |  __`SYSLIB0057`__ | Loading certificate data through the constructor or Import is obsolete. Use X509CertificateLoader instead to load certificates. |
 |  __`SYSLIB0058`__ | KeyExchangeAlgorithm, KeyExchangeStrength, CipherAlgorithm, CipherAlgorithmStrength, HashAlgorithm and HashStrength properties of SslStream are obsolete. Use NegotiatedCipherSuite instead. |
+|  __`SYSLIB0059`__ | SystemEvents.EventsThreadShutdown callbacks are not run before the process exits. Use AppDomain.ProcessExit instead. |
 
 ## Analyzer Warnings
 

diff --git a/src/libraries/Common/src/System/Obsoletions.cs b/src/libraries/Common/src/System/Obsoletions.cs
@@ -187,6 +187,9 @@ internal static class Obsoletions
         internal const string TlsCipherAlgorithmEnumsMessage = "KeyExchangeAlgorithm, KeyExchangeStrength, CipherAlgorithm, CipherAlgorithmStrength, HashAlgorithm and HashStrength properties of SslStream are obsolete. Use NegotiatedCipherSuite instead.";
         internal const string TlsCipherAlgorithmEnumsDiagId = "SYSLIB0058";
 
+        internal const string SystemEventsEventsThreadShutdownMessage = "SystemEvents.EventsThreadShutdown callbacks are not run before the process exits. Use AppDomain.ProcessExit instead.";
+        internal const string SystemEventsEventsThreadShutdownDiagId = "SYSLIB0059";
+
         // When adding a new diagnostic ID, add it to the table in docs\project\list-of-diagnostics.md as well.
         // Keep new const identifiers above this comment.
     }

diff --git a/src/libraries/Microsoft.Win32.SystemEvents/ref/Microsoft.Win32.SystemEvents.cs b/src/libraries/Microsoft.Win32.SystemEvents/ref/Microsoft.Win32.SystemEvents.cs
@@ -59,6 +59,7 @@ public sealed partial class SystemEvents
         internal SystemEvents() { }
         public static event System.EventHandler? DisplaySettingsChanged { add { } remove { } }
         public static event System.EventHandler? DisplaySettingsChanging { add { } remove { } }
+        [System.ObsoleteAttribute("SystemEvents.EventsThreadShutdown callbacks are not run before the process exits. Use AppDomain.ProcessExit instead.", DiagnosticId = "SYSLIB0059", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
         public static event System.EventHandler? EventsThreadShutdown { add { } remove { } }
         public static event System.EventHandler? InstalledFontsChanged { add { } remove { } }
         [System.ComponentModel.BrowsableAttribute(false)]

diff --git a/src/libraries/Microsoft.Win32.SystemEvents/ref/Microsoft.Win32.SystemEvents.csproj b/src/libraries/Microsoft.Win32.SystemEvents/ref/Microsoft.Win32.SystemEvents.csproj
@@ -1,6 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
     <TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
+    <IncludeInternalObsoleteAttribute>true</IncludeInternalObsoleteAttribute>
   </PropertyGroup>
 
   <ItemGroup>

diff --git a/src/libraries/Microsoft.Win32.SystemEvents/src/CompatibilitySuppressions.xml b/src/libraries/Microsoft.Win32.SystemEvents/src/CompatibilitySuppressions.xml
@@ -1,6 +1,13 @@
 <?xml version="1.0" encoding="utf-8"?>
 <!-- https://learn.microsoft.com/dotnet/fundamentals/package-validation/diagnostic-ids -->
 <Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
+  <Suppression>
+    <DiagnosticId>CP0014</DiagnosticId>
+    <Target>E:Microsoft.Win32.SystemEvents.LowMemory:[T:System.ObsoleteAttribute]</Target>
+    <Left>lib/netstandard2.0/Microsoft.Win32.SystemEvents.dll</Left>
+    <Right>lib/netstandard2.0/Microsoft.Win32.SystemEvents.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
   <Suppression>
     <DiagnosticId>CP0015</DiagnosticId>
     <Target>E:Microsoft.Win32.SystemEvents.LowMemory:[T:System.ObsoleteAttribute]</Target>

diff --git a/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft.Win32.SystemEvents.csproj b/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft.Win32.SystemEvents.csproj
@@ -6,6 +6,7 @@
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <UseCompilerGeneratedDocXmlFile>false</UseCompilerGeneratedDocXmlFile>
     <IsPackable>true</IsPackable>
+    <IncludeInternalObsoleteAttribute>true</IncludeInternalObsoleteAttribute>
     <PackageDescription>Provides access to Windows system event notifications.
 
 Commonly Used Types:
@@ -104,6 +105,8 @@ Microsoft.Win32.SystemEvents</PackageDescription>
              Link="Common\Interop\Windows\WtsApi32\Interop.WTSUnRegisterSessionNotification.cs" />
     <Compile Include="$(CommonPath)Microsoft\Win32\SafeHandles\SafeLibraryHandle.cs"
              Link="Common\Microsoft\Win32\SafeHandles\SafeLibraryHandle.cs" />
+    <Compile Include="$(CommonPath)System\Obsoletions.cs"
+             Link="Common\System\Obsoletions.cs" />
     <Compile Include="Microsoft\Win32\PowerModeChangedEventArgs.cs" />
     <Compile Include="Microsoft\Win32\PowerModeChangedEventHandler.cs" />
     <Compile Include="Microsoft\Win32\PowerModes.cs" />

diff --git a/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft/Win32/SystemEvents.cs b/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft/Win32/SystemEvents.cs
@@ -130,17 +130,12 @@ public static event EventHandler? DisplaySettingsChanged
         ///  Occurs before the thread that listens for system events is terminated.
         ///  Delegates will be invoked on the events thread.
         /// </summary>
+        [Obsolete(Obsoletions.SystemEventsEventsThreadShutdownMessage, DiagnosticId = Obsoletions.SystemEventsEventsThreadShutdownDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
         public static event EventHandler? EventsThreadShutdown
         {
             // Really only here for GDI+ initialization and shut down
-            add
-            {
-                AddEventHandler(s_onEventsThreadShutdownEvent, value);
-            }
-            remove
-            {
-                RemoveEventHandler(s_onEventsThreadShutdownEvent, value);
-            }
+            add => AddEventHandler(s_onEventsThreadShutdownEvent, value);
+            remove => RemoveEventHandler(s_onEventsThreadShutdownEvent, value);
         }
 
         /// <summary>
@@ -464,39 +459,43 @@ private void Dispose()
         /// </summary>
         private static void EnsureSystemEvents(bool requireHandle)
         {
-            if (s_systemEvents == null)
+            if (s_systemEvents is not null)
             {
-                lock (s_procLockObject)
+                return;
+            }
+
+            lock (s_procLockObject)
+            {
+                if (s_systemEvents is not null)
                 {
-                    if (s_systemEvents == null)
-                    {
-                        // Create a new pumping thread.  We always create one even if the current thread
-                        // is STA, as there are no guarantees this thread will pump nor still be alive
-                        // for the desired duration.
+                    return;
+                }
 
-                        s_eventWindowReady = new ManualResetEvent(false);
-                        SystemEvents systemEvents = new SystemEvents();
-                        s_windowThread = new Thread(new ThreadStart(systemEvents.WindowThreadProc))
-                        {
-                            IsBackground = true,
-                            Name = ".NET System Events"
-                        };
-                        s_windowThread.Start();
-                        s_eventWindowReady.WaitOne();
+                // Create a new pumping thread. We always create one even if the current thread
+                // is STA, as there are no guarantees this thread will pump nor still be alive
+                // for the desired duration.
 
-                        // ensure this is initialized last as that will force concurrent threads calling
-                        // this method to block until after we've initialized.
-                        s_systemEvents = systemEvents;
+                s_eventWindowReady = new ManualResetEvent(false);
+                SystemEvents systemEvents = new SystemEvents();
+                s_windowThread = new Thread(new ThreadStart(systemEvents.WindowThreadProc))
+                {
+                    IsBackground = true,
+                    Name = ".NET System Events"
+                };
+                s_windowThread.Start();
+                s_eventWindowReady.WaitOne();
 
-                        if (requireHandle && s_systemEvents._windowHandle == IntPtr.Zero)
-                        {
-                            // In theory, it's not the end of the world that
-                            // we don't get system events.  Unfortunately, the main reason windowHandle == 0
-                            // is CreateWindowEx failed for mysterious reasons, and when that happens,
-                            // subsequent (and more important) CreateWindowEx calls also fail.
-                            throw new ExternalException(SR.ErrorCreateSystemEvents);
-                        }
-                    }
+                // Ensure this is initialized last as that will force concurrent threads calling
+                // this method to block until after we've initialized.
+                s_systemEvents = systemEvents;
+
+                if (requireHandle && s_systemEvents._windowHandle == IntPtr.Zero)
+                {
+                    // In theory, it's not the end of the world that
+                    // we don't get system events.  Unfortunately, the main reason windowHandle == 0
+                    // is CreateWindowEx failed for mysterious reasons, and when that happens,
+                    // subsequent (and more important) CreateWindowEx calls also fail.
+                    throw new ExternalException(SR.ErrorCreateSystemEvents);
                 }
             }
         }
@@ -694,8 +693,6 @@ private unsafe void Initialize()
                         hInstance, IntPtr.Zero);
                 }
             }
-
-            AppDomain.CurrentDomain.ProcessExit += new EventHandler(Shutdown);
         }
 
         /// <summary>
@@ -759,16 +756,15 @@ public static void InvokeOnEventsThread(Delegate method)
             {
                 int pid;
                 int thread = Interop.User32.GetWindowThreadProcessId(s_systemEvents!._windowHandle, &pid);
-                GC.KeepAlive(s_systemEvents);
                 Debug.Assert(s_windowThread == null || thread != Interop.Kernel32.GetCurrentThreadId(), "Don't call MarshaledInvoke on the system events thread");
             }
 #endif
 
-            if (s_threadCallbackList == null)
+            if (s_threadCallbackList is null)
             {
                 lock (s_eventLockObject)
                 {
-                    if (s_threadCallbackList == null)
+                    if (s_threadCallbackList is null)
                     {
                         s_threadCallbackMessage = Interop.User32.RegisterWindowMessageW("SystemEventsThreadCallbackMessage");
                         s_threadCallbackList = new Queue<Delegate>();
@@ -784,7 +780,6 @@ public static void InvokeOnEventsThread(Delegate method)
             }
 
             Interop.User32.PostMessageW(s_systemEvents!._windowHandle, s_threadCallbackMessage, IntPtr.Zero, IntPtr.Zero);
-            GC.KeepAlive(s_systemEvents);
         }
 
         /// <summary>
@@ -1073,56 +1068,6 @@ private static void RemoveEventHandler(object key, Delegate? value)
             }
         }
 
-        private static void Shutdown()
-        {
-            if (s_systemEvents != null)
-            {
-                lock (s_procLockObject)
-                {
-                    if (s_systemEvents != null)
-                    {
-                        // If we are using system events from another thread, request that it terminate
-                        if (s_windowThread != null)
-                        {
-#if DEBUG
-                            unsafe
-                            {
-                                int pid;
-                                int thread = Interop.User32.GetWindowThreadProcessId(s_systemEvents._windowHandle, &pid);
-                                Debug.Assert(thread != Interop.Kernel32.GetCurrentThreadId(), "Don't call Shutdown on the system events thread");
-
-                            }
-#endif
-                            // The handle could be valid, Zero or invalid depending on the state of the thread
-                            // that is processing the messages. We optimistically expect it to be valid to
-                            // notify the thread to shutdown. The Zero or invalid values should be present
-                            // only when the thread is already shutting down due to external factors.
-                            if (s_systemEvents._windowHandle != IntPtr.Zero)
-                            {
-                                Interop.User32.PostMessageW(s_systemEvents._windowHandle, Interop.User32.WM_QUIT, IntPtr.Zero, IntPtr.Zero);
-                                GC.KeepAlive(s_systemEvents);
-                            }
-
-                            s_windowThread.Join();
-                        }
-                        else
-                        {
-                            s_systemEvents.Dispose();
-                            s_systemEvents = null;
-                        }
-                    }
-                }
-            }
-        }
-
-#if FEATURE_CER
-        [PrePrepareMethod]
-#endif
-        private static void Shutdown(object? sender, EventArgs e)
-        {
-            Shutdown();
-        }
-
         /// <summary>
         ///  A standard Win32 window proc for our broadcast window.
         /// </summary>
@@ -1250,8 +1195,8 @@ private IntPtr WindowProc(IntPtr hWnd, int msg, nint wParam, nint lParam)
         }
 
         /// <summary>
-        ///  This is the method that runs our window thread.  This method
-        ///  creates a window and spins up a message loop.  The window
+        ///  This is the method that runs our window thread. This method
+        ///  creates a window and spins up a message loop. The window
         ///  is made visible with a size of 0, 0, so that it will trap
         ///  global broadcast messages.
         /// </summary>
@@ -1264,7 +1209,7 @@ private void WindowThreadProc()
 
                 if (_windowHandle != IntPtr.Zero)
                 {
-                    Interop.User32.MSG msg = default(Interop.User32.MSG);
+                    Interop.User32.MSG msg = default;
 
                     while (Interop.User32.GetMessageW(ref msg, _windowHandle, 0, 0) > 0)
                     {
@@ -1277,11 +1222,11 @@ private void WindowThreadProc()
             }
             catch (Exception e)
             {
-                // In case something very very wrong happend during the creation action.
+                // In case something very very wrong happened during the creation action.
                 // This will unblock the calling thread.
                 s_eventWindowReady!.Set();
 
-                if (!((e is ThreadInterruptedException) || (e is ThreadAbortException)))
+                if (e is not (ThreadInterruptedException or ThreadAbortException))
                 {
                     Debug.Fail("Unexpected thread exception in system events window thread proc", e.ToString());
                 }

diff --git a/src/libraries/Microsoft.Win32.SystemEvents/tests/ShutdownTest.cs b/src/libraries/Microsoft.Win32.SystemEvents/tests/ShutdownTest.cs
@@ -2,32 +2,37 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
-using System.Threading;
 using Microsoft.DotNet.RemoteExecutor;
 using Xunit;
 using static Interop;
 
-namespace Microsoft.Win32.SystemEventsTests
+namespace Microsoft.Win32.SystemEventsTests;
+
+public class ShutdownTest : SystemEventsTest
 {
-    public abstract class ShutdownTest : SystemEventsTest
+    [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoNorServerCore))]
+    [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
+    public void ShutdownThroughRestartManager()
     {
-        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoNorServerCore))]
-        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
-        public void ShutdownThroughRestartManager()
+        RemoteExecutor.Invoke(() =>
         {
-            RemoteExecutor.Invoke(() =>
-            {
-                // Register any event to ensure that SystemEvents get initialized
-                SystemEvents.TimeChanged += (o, e) => { };
+            // Register any event to ensure that SystemEvents get initialized
+            SystemEvents.TimeChanged += (o, e) => { };
 
-                // Fake Restart Manager behavior by sending external WM_CLOSE message
-                SendMessage(Interop.User32.WM_CLOSE, IntPtr.Zero, IntPtr.Zero);
+            // Fake Restart Manager behavior by sending external WM_CLOSE message
+            SendMessage(Interop.User32.WM_CLOSE, IntPtr.Zero, IntPtr.Zero);
+        }).Dispose();
+    }
 
-                // Emulate calling the Shutdown event
-                var shutdownMethod = typeof(SystemEvents).GetMethod("Shutdown", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic, null, new Type[0], null);
-                Assert.NotNull(shutdownMethod);
-                shutdownMethod.Invoke(null, null);
-            }).Dispose();
-        }
+    [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoNorServerCore))]
+    [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
+    public void ShutdownSuccessDespiteThreadBlock()
+    {
+        RemoteExecutor.Invoke(() =>
+        {
+            // Block the SystemEvents thread. Regression test for https://github.com/dotnet/winforms/issues/11944
+            SystemEvents.UserPreferenceChanged += (o, e) => { while (true) { } };
+            SendMessage(User32.WM_SETTINGCHANGE, IntPtr.Zero, IntPtr.Zero);
+        }).Dispose();
     }
 }