From: Anton Firszov <Anton.Firszov@microsoft.com>
Date: Fri, 19 Jun 2020 12:20:59 +0000 (+0200)
Subject: Move epoll event handling to a non-inlined method (#37138)
X-Git-Tag: submit/tizen/20210909.063632~7265
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=85a97b64d7a86e1c8747479a7f2c6df19cf3acd6;p=platform%2Fupstream%2Fdotnet%2Fruntime.git

Move epoll event handling to a non-inlined method (#37138)
---

diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
index 99380b3..e3f5735 100644
--- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
+++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
@@ -184,14 +184,11 @@ namespace System.Net.Sockets
         {
             try
             {
-                Interop.Sys.SocketEvent* buffer = _buffer;
-                ConcurrentDictionary<IntPtr, SocketAsyncContextWrapper> handleToContextMap = _handleToContextMap;
-                ConcurrentQueue<SocketIOEvent> eventQueue = _eventQueue;
-                SocketAsyncContext? context = null;
+                SocketEventHandler handler = new SocketEventHandler(this);
                 while (true)
                 {
                     int numEvents = EventBufferCount;
-                    Interop.Error err = Interop.Sys.WaitForSocketEvents(_port, buffer, &numEvents);
+                    Interop.Error err = Interop.Sys.WaitForSocketEvents(_port, handler.Buffer, &numEvents);
                     if (err != Interop.Error.SUCCESS)
                     {
                         throw new InternalException(err);
@@ -200,43 +197,7 @@ namespace System.Net.Sockets
                     // The native shim is responsible for ensuring this condition.
                     Debug.Assert(numEvents > 0, $"Unexpected numEvents: {numEvents}");
 
-                    bool enqueuedEvent = false;
-                    foreach (var socketEvent in new ReadOnlySpan<Interop.Sys.SocketEvent>(buffer, numEvents))
-                    {
-                        IntPtr handle = socketEvent.Data;
-
-                        if (handleToContextMap.TryGetValue(handle, out SocketAsyncContextWrapper contextWrapper) && (context = contextWrapper.Context) != null)
-                        {
-                            if (context.PreferInlineCompletions)
-                            {
-                                context.HandleEventsInline(socketEvent.Events);
-                            }
-                            else
-                            {
-                                Interop.Sys.SocketEvents events = context.HandleSyncEventsSpeculatively(socketEvent.Events);
-
-                                if (events != Interop.Sys.SocketEvents.None)
-                                {
-                                    var ev = new SocketIOEvent(context, events);
-                                    eventQueue.Enqueue(ev);
-                                    enqueuedEvent = true;
-
-                                    // This is necessary when the JIT generates unoptimized code (debug builds, live debugging,
-                                    // quick JIT, etc.) to ensure that the context does not remain referenced by this method, as
-                                    // such code may keep the stack location live for longer than necessary
-                                    ev = default;
-                                }
-                            }
-
-                            // This is necessary when the JIT generates unoptimized code (debug builds, live debugging,
-                            // quick JIT, etc.) to ensure that the context does not remain referenced by this method, as
-                            // such code may keep the stack location live for longer than necessary
-                            context = null;
-                            contextWrapper = default;
-                        }
-                    }
-
-                    if (enqueuedEvent)
+                    if (handler.HandleSocketEvents(numEvents))
                     {
                         ScheduleToProcessEvents();
                     }
@@ -326,6 +287,56 @@ namespace System.Net.Sockets
             }
         }
 
+        // The JIT is allowed to arbitrarily extend the lifetime of locals, which may retain SocketAsyncContext references,
+        // indirectly preventing Socket instances to be finalized, despite being no longer referenced by user code.
+        // To avoid this, the event handling logic is delegated to a non-inlined processing method.
+        // See discussion: https://github.com/dotnet/runtime/issues/37064
+        // SocketEventHandler holds an on-stack cache of SocketAsyncEngine members needed by the handler method.
+        private readonly struct SocketEventHandler
+        {
+            public Interop.Sys.SocketEvent* Buffer { get; }
+
+            private readonly ConcurrentDictionary<IntPtr, SocketAsyncContextWrapper> _handleToContextMap;
+            private readonly ConcurrentQueue<SocketIOEvent> _eventQueue;
+
+            public SocketEventHandler(SocketAsyncEngine engine)
+            {
+                Buffer = engine._buffer;
+                _handleToContextMap = engine._handleToContextMap;
+                _eventQueue = engine._eventQueue;
+            }
+
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public bool HandleSocketEvents(int numEvents)
+            {
+                bool enqueuedEvent = false;
+                foreach (var socketEvent in new ReadOnlySpan<Interop.Sys.SocketEvent>(Buffer, numEvents))
+                {
+                    if (_handleToContextMap.TryGetValue(socketEvent.Data, out SocketAsyncContextWrapper contextWrapper))
+                    {
+                        SocketAsyncContext context = contextWrapper.Context;
+
+                        if (context.PreferInlineCompletions)
+                        {
+                            context.HandleEventsInline(socketEvent.Events);
+                        }
+                        else
+                        {
+                            Interop.Sys.SocketEvents events = context.HandleSyncEventsSpeculatively(socketEvent.Events);
+
+                            if (events != Interop.Sys.SocketEvents.None)
+                            {
+                                _eventQueue.Enqueue(new SocketIOEvent(context, events));
+                                enqueuedEvent = true;
+                            }
+                        }
+                    }
+                }
+
+                return enqueuedEvent;
+            }
+        }
+
         // struct wrapper is used in order to improve the performance of the epoll thread hot path by up to 3% of some TechEmpower benchmarks
         // the goal is to have a dedicated generic instantiation and using:
         // System.Collections.Concurrent.ConcurrentDictionary`2[System.IntPtr,System.Net.Sockets.SocketAsyncContextWrapper]::TryGetValueInternal(!0,int32,!1&)
diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs
index ee81d7c..e15249c 100644
--- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs
+++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/DisposedSocketTests.cs
@@ -751,7 +751,6 @@ namespace System.Net.Sockets.Tests
         [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
         [InlineData(false)]
         [InlineData(true)]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/35846", TestPlatforms.AnyUnix)]
         public async Task NonDisposedSocket_SafeHandlesCollected(bool clientAsync)
         {
             List<WeakReference> handles = await CreateHandlesAsync(clientAsync);