NetworkAddressChange.Unix: use async Socket API to wait for events.

src/libraries/Common/src/Interop/Unix/System.Native/Interop.NetworkChange.cs

@@ -17,12 +17,9 @@ public enum NetworkChangeKind
         }
 
         [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_CreateNetworkChangeListenerSocket")]
-        public static unsafe partial Error CreateNetworkChangeListenerSocket(int* socket);
-
-        [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_CloseNetworkChangeListenerSocket")]
-        public static partial Error CloseNetworkChangeListenerSocket(int socket);
+        public static unsafe partial Error CreateNetworkChangeListenerSocket(IntPtr* socket);
 
         [GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadEvents")]
-        public static unsafe partial void ReadEvents(int socket, delegate* unmanaged<int, NetworkChangeKind, void> onNetworkChange);
+        public static unsafe partial Error ReadEvents(SafeHandle socket, delegate* unmanaged<IntPtr, NetworkChangeKind, void> onNetworkChange);
     }
 }

src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj

@@ -117,6 +117,7 @@
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.MapTcpState.cs" Link="Common\Interop\Unix\System.Native\Interop.MapTcpState.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\Interop.Errors.cs" Link="Common\Interop\CoreLib\Unix\Interop.Errors.cs" />
     <Compile Include="$(CommonPath)System\IO\RowConfigReader.cs" Link="Common\System\IO\RowConfigReader.cs" />
+    <Compile Include="$(CommonPath)System\Net\Logging\NetEventSource.Common.cs" Link="Common\System\Net\Logging\NetEventSource.Common.cs" />
   </ItemGroup>
   <!-- Linux (other than Android) -->
   <ItemGroup Condition="'$(TargetsLinux)' == 'true' and '$(TargetsAndroid)' != 'true'">

src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkAddressChange.Unix.cs

@@ -2,17 +2,35 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections.Generic;
+using System.ComponentModel;
 using System.Diagnostics;
+using System.Net.Sockets;
 using System.Runtime.InteropServices;
 using System.Threading;
+using System.Threading.Tasks;
 
 namespace System.Net.NetworkInformation
 {
     // Linux implementation of NetworkChange
     public partial class NetworkChange
     {
-        private static volatile SocketWrapper? s_socket;
-        // Lock controlling access to delegate subscriptions, socket initialization, availability-changed state and timer.
+        private static Socket? s_socket;
+
+        private static Socket? Socket
+        {
+            get
+            {
+                Debug.Assert(Monitor.IsEntered(s_gate));
+                return s_socket;
+            }
+            set
+            {
+                Debug.Assert(Monitor.IsEntered(s_gate));
+                s_socket = value;
+            }
+        }
+
+        // Lock controlling access to delegate subscriptions, socket, availability-changed state and timer.
         private static readonly object s_gate = new object();
 
         // The "leniency" window for NetworkAvailabilityChanged socket events.
@@ -33,7 +51,7 @@ public static event NetworkAddressChangedEventHandler? NetworkAddressChanged
                 {
                     lock (s_gate)
                     {
-                        if (s_socket == null)
+                        if (Socket == null)
                         {
                             CreateSocket();
                         }
@@ -50,8 +68,8 @@ public static event NetworkAddressChangedEventHandler? NetworkAddressChanged
                     {
                         if (s_addressChangedSubscribers.Count == 0 && s_availabilityChangedSubscribers.Count == 0)
                         {
-                            Debug.Assert(s_socket == null,
-                                "s_socket is not null, but there are no subscribers to NetworkAddressChanged or NetworkAvailabilityChanged.");
+                            Debug.Assert(Socket == null,
+                                "Socket is not null, but there are no subscribers to NetworkAddressChanged or NetworkAvailabilityChanged.");
                             return;
                         }
 
@@ -73,7 +91,7 @@ public static event NetworkAvailabilityChangedEventHandler? NetworkAvailabilityC
                 {
                     lock (s_gate)
                     {
-                        if (s_socket == null)
+                        if (Socket == null)
                         {
                             CreateSocket();
                         }
@@ -112,8 +130,8 @@ public static event NetworkAvailabilityChangedEventHandler? NetworkAvailabilityC
                     {
                         if (s_addressChangedSubscribers.Count == 0 && s_availabilityChangedSubscribers.Count == 0)
                         {
-                            Debug.Assert(s_socket == null,
-                                "s_socket is not null, but there are no subscribers to NetworkAddressChanged or NetworkAvailabilityChanged.");
+                            Debug.Assert(Socket == null,
+                                "Socket is not null, but there are no subscribers to NetworkAddressChanged or NetworkAvailabilityChanged.");
                             return;
                         }
 
@@ -139,54 +157,75 @@ public static event NetworkAvailabilityChangedEventHandler? NetworkAvailabilityC
 
         private static unsafe void CreateSocket()
         {
-            Debug.Assert(s_socket == null, "s_socket is not null, must close existing socket before opening another.");
-            int newSocket;
+            Debug.Assert(Monitor.IsEntered(s_gate));
+            Debug.Assert(Socket == null, "Socket is not null, must close existing socket before opening another.");
+            IntPtr newSocket;
             Interop.Error result = Interop.Sys.CreateNetworkChangeListenerSocket(&newSocket);
             if (result != Interop.Error.SUCCESS)
             {
                 string message = Interop.Sys.GetLastErrorInfo().GetErrorMessage();
                 throw new NetworkInformationException(message);
             }
 
-            s_socket = new SocketWrapper(newSocket);
+            Socket = new Socket(new SafeSocketHandle(newSocket, ownsHandle: true));
 
-            new Thread(args => LoopReadSocket((SocketWrapper)args!))
-            {
-                IsBackground = true,
-                Name = ".NET Network Address Change"
-            }.UnsafeStart(s_socket);
+            // Don't capture ExecutionContext.
+            ThreadPool.UnsafeQueueUserWorkItem(
+                static socket => ReadEventsAsync(socket),
+                Socket, preferLocal: false);
         }
 
         private static void CloseSocket()
         {
-            Debug.Assert(s_socket != null, "s_socket was null when CloseSocket was called.");
-            Interop.Error result = Interop.Sys.CloseNetworkChangeListenerSocket(s_socket != null ? s_socket.Socket : -1);
-            if (result != Interop.Error.SUCCESS)
-            {
-                string message = Interop.Sys.GetLastErrorInfo().GetErrorMessage();
-                throw new NetworkInformationException(message);
-            }
-
-            s_socket = null;
+            Debug.Assert(Monitor.IsEntered(s_gate));
+            Debug.Assert(Socket != null, "Socket was null when CloseSocket was called.");
+            Socket.Dispose();
+            Socket = null;
         }
 
-        private static unsafe void LoopReadSocket(SocketWrapper initiallyCreatedSocket)
+        private static async void ReadEventsAsync(Socket socket)
         {
-            while (s_socket == initiallyCreatedSocket) //if both references are equal then s_socket was not changed
+            try
             {
-                //we can continue processing events
-                Interop.Sys.ReadEvents(initiallyCreatedSocket.Socket, &ProcessEvent);
+                while (true)
+                {
+                    // Wait for data to become available.
+                    await socket.ReceiveAsync(Array.Empty<byte>(), SocketFlags.None).ConfigureAwait(false);
+
+                    Interop.Error result = ReadEvents(socket);
+
+                    if (result != Interop.Error.SUCCESS ||
+                        result != Interop.Error.EAGAIN)
+                    {
+                        throw new Win32Exception(result.Info().RawErrno);
+                    }
+                }
+            }
+            catch (ObjectDisposedException)
+            { } // Socket disposed.
+            catch (SocketException se) when (se.SocketErrorCode == SocketError.OperationAborted)
+            { } // ReceiveAsync aborted by disposing Socket.
+            catch (Exception ex)
+            {
+                // Unexpected error.
+                Debug.Fail($"Unexpected error: {ex}");
+                if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(null, ex);
             }
+
+            static unsafe Interop.Error ReadEvents(Socket socket)
+                => Interop.Sys.ReadEvents(socket.SafeHandle, &ProcessEvent);
         }
 
         [UnmanagedCallersOnly]
-        private static void ProcessEvent(int socket, Interop.Sys.NetworkChangeKind kind)
+        private static void ProcessEvent(IntPtr socket, Interop.Sys.NetworkChangeKind kind)
         {
             if (kind != Interop.Sys.NetworkChangeKind.None)
             {
                 lock (s_gate)
                 {
-                    if (s_socket != null && socket == s_socket.Socket)
+                    // It's safe to compare raw handle values because ProcessEvents gets
+                    // called from ReadEvents which holds a reference on the SafeHandle.
+                    if (Socket != null && socket == Socket.Handle)
                     {
                         OnSocketEvent(kind);
                     }
@@ -291,19 +330,5 @@ private static void OnAvailabilityTimerFired(object? state)
                 }
             }
         }
-
-        // SocketWrapper class was introduced to prevent event thread leaks on Linux
-        // as described on GitHub: https://github.com/dotnet/runtime/issues/63788
-        // For each CreateSocket() new s_socket instance is created and we pass it to ".NET Network Address Change" thread.
-        // We then continue loop in LoopReadSocket method as long as both references are equal.
-        private class SocketWrapper
-        {
-            public SocketWrapper(int socket)
-            {
-                Socket = socket;
-            }
-
-            public int Socket { get; }
-        }
     }
 }

src/libraries/System.Net.NetworkInformation/tests/FunctionalTests/NetworkAddressChangedTests.cs

@@ -2,9 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using Xunit;
-using System.Diagnostics;
-using System.Threading;
-using System.Linq;
 
 namespace System.Net.NetworkInformation.Tests
 {
@@ -26,31 +23,5 @@ public void NetworkAddressChanged_JustRemove_Success()
         {
             NetworkChange.NetworkAddressChanged -= _addressHandler;
         }
-
-        [PlatformSpecific(TestPlatforms.Linux)]
-        [OuterLoop("May take several seconds")]
-        [ConditionalFact(nameof(SupportsGettingThreadsWithPsCommand))]
-        public void NetworkAddressChanged_AddRemoveMultipleTimes_CheckForLeakingThreads()
-        {
-            for (int i = 1; i <= 10; i++)
-            {
-                NetworkChange.NetworkAddressChanged += _addressHandler;
-                NetworkChange.NetworkAddressChanged -= _addressHandler;
-            }
-
-            Thread.Sleep(2000); //allow some time for threads to exit
-
-            //We are searching for threads containing ".NET Network Ad"
-            //because ps command trims actual thread name ".NET Network Address Change".
-            //This thread is created in:
-            //  src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkAddressChange.Unix.cs
-            int numberOfNetworkAddressChangeThreads = ProcessUtil.GetProcessThreadsWithPsCommand(Process.GetCurrentProcess().Id)
-                .Where(e => e.IndexOf(".NET Network Ad") > 0).Count();
-
-            Assert.Equal(0, numberOfNetworkAddressChangeThreads); //there should be no threads because there are no event subscribers
-        }
-
-        private static bool SupportsGettingThreadsWithPsCommand
-            => TestConfiguration.SupportsGettingThreadsWithPsCommand;
     }
 }

src/libraries/System.Net.NetworkInformation/tests/FunctionalTests/System.Net.NetworkInformation.Functional.Tests.csproj

@@ -40,8 +40,6 @@
     <Compile Include="PhysicalAddressTest.cs" />
     <Compile Include="ConnectionsParsingTests.cs" />
     <Compile Include="StatisticsParsingTests.cs" />
-    <Compile Include="ProcessUtil.cs" />
-    <Compile Include="TestConfiguration.cs" />
     <!-- Common test files -->
     <Compile Include="$(CommonTestPath)System\Net\Http\TestHelper.cs"
              Link="Common\System\Net\Http\TestHelper.cs" />

src/native/libs/System.Native/entrypoints.c

@@ -126,7 +126,6 @@ static const Entry s_sysNative[] =
     DllImportEntry(SystemNative_GetAllMountPoints)
     DllImportEntry(SystemNative_ReadEvents)
     DllImportEntry(SystemNative_CreateNetworkChangeListenerSocket)
-    DllImportEntry(SystemNative_CloseNetworkChangeListenerSocket)
     DllImportEntry(SystemNative_GetHostEntryForName)
     DllImportEntry(SystemNative_FreeHostEntry)
     DllImportEntry(SystemNative_GetNameInfo)