Fix System.Net.NetworkInformation.NetworkChange deadlock (#26259)

User code can subscribe to the NetworkAvailabilityChanged event. If a Network Address changed event get triggered while the user code is unsubscribing from the NetworkAvailabilityChanged event, there will be a deadlock.

The deadlock can happen because two threads are holding two different locks for AddressChangeListener and AvailabilityChangeListener, and they are waiting for each other to release its lock first. The fix is using a global lock to replace two locks, so that we don’t allow operations on AddressChangeListener and AvailabilityChangeListener happen at the same time.

Without fixing this, users can frequently see the deadlock when they put their computer to sleep, re-awaken it, then call the code that unsubscribes from NetworkAvailabilityChanged event.

This deadlock is only specific to Windows implementation. For Unix implementations (Linux & OSX), their current implementation is to use a global lock object to protect both NetworkAddressChangedEventHandler & NetworkAvailabilityChangedEventHandler.

This issue was reported in internal bug 162830.

Author: Caesar1995

src/System.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkAddressChange.Windows.cs

@@ -15,6 +15,8 @@ public class NetworkChange
         //introduced for supporting design-time loading of System.Windows.dll
         [Obsolete("This API supports the .NET Framework infrastructure and is not intended to be used directly from your code.", true)]
         public static void RegisterNetworkChange(NetworkChange nc) { }
+        
+        private static readonly object s_globalLock = new object();
 
         public static event NetworkAvailabilityChangedEventHandler NetworkAvailabilityChanged
         {
@@ -42,7 +44,6 @@ public static event NetworkAddressChangedEventHandler NetworkAddressChanged
 
         internal static class AvailabilityChangeListener
         {
-            private static readonly object s_syncObject = new object();
             private static readonly Dictionary<NetworkAvailabilityChangedEventHandler, ExecutionContext> s_availabilityCallerArray =
                 new Dictionary<NetworkAvailabilityChangedEventHandler, ExecutionContext>();
             private static readonly NetworkAddressChangedEventHandler s_addressChange = ChangedAddress;
@@ -56,36 +57,44 @@ private static void RunHandlerCallback(object state)
 
             private static void ChangedAddress(object sender, EventArgs eventArgs)
             {
-                lock (s_syncObject)
+                Dictionary<NetworkAvailabilityChangedEventHandler, ExecutionContext> copy = null;
+
+                lock (s_globalLock)
                 {
                     bool isAvailableNow = SystemNetworkInterface.InternalGetIsNetworkAvailable();
 
+                    // If there is an Availability Change, need to execute user callbacks.
                     if (isAvailableNow != s_isAvailable)
                     {
                         s_isAvailable = isAvailableNow;
 
-                        var s_copy =
+                        copy =
                             new Dictionary<NetworkAvailabilityChangedEventHandler, ExecutionContext>(s_availabilityCallerArray);
+                    }
+                }
 
-                        foreach (var handler in s_copy.Keys)
+                // Executing user callbacks if Availability Change event occured.
+                if (copy != null)
+                {
+                    foreach (var entry in copy)
+                    {
+                        NetworkAvailabilityChangedEventHandler handler = entry.Key;
+                        ExecutionContext context = entry.Value;
+                        if (context == null)
                         {
-                            ExecutionContext context = s_copy[handler];
-                            if (context == null)
-                            {
-                                handler(null, new NetworkAvailabilityEventArgs(s_isAvailable));
-                            }
-                            else
-                            {
-                                ExecutionContext.Run(context, s_RunHandlerCallback, handler);
-                            }
+                            handler(null, new NetworkAvailabilityEventArgs(s_isAvailable));
+                        }
+                        else
+                        {
+                            ExecutionContext.Run(context, s_RunHandlerCallback, handler);
                         }
                     }
                 }
             }
 
             internal static void Start(NetworkAvailabilityChangedEventHandler caller)
             {
-                lock (s_syncObject)
+                lock (s_globalLock)
                 {
                     if (s_availabilityCallerArray.Count == 0)
                     {
@@ -102,7 +111,7 @@ internal static void Start(NetworkAvailabilityChangedEventHandler caller)
 
             internal static void Stop(NetworkAvailabilityChangedEventHandler caller)
             {
-                lock (s_syncObject)
+                lock (s_globalLock)
                 {
                     s_availabilityCallerArray.Remove(caller);
                     if (s_availabilityCallerArray.Count == 0)
@@ -132,7 +141,9 @@ internal static unsafe class AddressChangeListener
             // Callback fired when an address change occurs.
             private static void AddressChangedCallback(object stateObject, bool signaled)
             {
-                lock (s_callerArray)
+                Dictionary<NetworkAddressChangedEventHandler, ExecutionContext> copy;
+
+                lock (s_globalLock)
                 {
                     // The listener was canceled, which would only happen if we aren't listening for more events.
                     s_isPending = false;
@@ -145,7 +156,7 @@ private static void AddressChangedCallback(object stateObject, bool signaled)
                     s_isListening = false;
 
                     // Need to copy the array so the callback can call start and stop
-                    var copy = new Dictionary<NetworkAddressChangedEventHandler, ExecutionContext>(s_callerArray);
+                    copy = new Dictionary<NetworkAddressChangedEventHandler, ExecutionContext>(s_callerArray);
 
                     try
                     {
@@ -156,10 +167,15 @@ private static void AddressChangedCallback(object stateObject, bool signaled)
                     {
                         if (NetEventSource.IsEnabled) NetEventSource.Error(null, nie);
                     }
+                }
 
-                    foreach (var handler in copy.Keys)
+                // Release the lock before calling into user callback.
+                if (copy.Count > 0)
+                {
+                    foreach (var entry in copy)
                     {
-                        ExecutionContext context = copy[handler];
+                        NetworkAddressChangedEventHandler handler = entry.Key;
+                        ExecutionContext context = entry.Value;
                         if (context == null)
                         {
                             handler(null, EventArgs.Empty);
@@ -189,7 +205,7 @@ internal static void UnsafeStart(NetworkAddressChangedEventHandler caller)
 
             private static void StartHelper(NetworkAddressChangedEventHandler caller, bool captureContext, StartIPOptions startIPOptions)
             {
-                lock (s_callerArray)
+                lock (s_globalLock)
                 {
                     // Setup changedEvent and native overlapped struct.
                     if (s_ipv4Socket == null)
@@ -315,7 +331,7 @@ private static void StartHelper(NetworkAddressChangedEventHandler caller, bool c
 
             internal static void Stop(NetworkAddressChangedEventHandler caller)
             {
-                lock (s_callerArray)
+                lock (s_globalLock)
                 {
                     s_callerArray.Remove(caller);
                     if (s_callerArray.Count == 0 && s_isListening)