Add comments about _singleBufferHandleState to SocketsAsyncEventArgs.Windows.cs (#46923)

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs

@@ -15,8 +15,41 @@ public partial class SocketAsyncEventArgs : EventArgs, IDisposable
     {
         // Single buffer
         private MemoryHandle _singleBufferHandle;
+
+        /// <summary>The state of the <see cref="_singleBufferHandle"/> to track whether and when it requires disposal to unpin memory.</summary>
+        /// <remarks>
+        /// Pinning via a GCHandle (the mechanism used by Memory) has measurable overhead, and for operations like
+        /// send and receive that we want to optimize and that frequently complete synchronously, we
+        /// want to avoid such GCHandle interactions whenever possible.  To achieve that, we used `fixed`
+        /// to pin the relevant state while starting the async operation, and then only if the operation
+        /// is seen to be pending do we use Pin to create the GCHandle; this is done while the `fixed` is
+        /// still in scope, to ensure that throughout the whole operation the buffer remains pinned, while
+        /// using the much cheaper `fixed` only for the fast path.  <see cref="_singleBufferHandle"/> starts
+        /// life as None, transitions to InProcess prior to initiating the async operation, and then transitions
+        /// either back to None if the buffer never needed to be pinned, or to Set once it has been pinned. This
+        /// ensures that asynchronous completion racing with the code that is still setting up the operation
+        /// can properly clean up after pinned memory, even if it needs to wait momentarily to do so.
+        ///
+        /// Currently, only the operations that use <see cref="_singleBufferHandle"/> and <see cref="_singleBufferHandleState"/>
+        /// are cancelable, and as such <see cref="_singleBufferHandleState"/> is also used to guard the cleanup
+        /// of <see cref="_registrationToCancelPendingIO"/>.
+        /// </remarks>
         private volatile SingleBufferHandleState _singleBufferHandleState;
-        private enum SingleBufferHandleState : byte { None, InProcess, Set }
+
+        /// <summary>Defines possible states for <see cref="_singleBufferHandleState"/> in order to faciliate correct cleanup of any pinned state.</summary>
+        private enum SingleBufferHandleState : byte
+        {
+            /// <summary>No operation using <see cref="_singleBufferHandle"/> is in flight, and no cleanup of <see cref="_singleBufferHandle"/> is required.</summary>
+            None,
+            /// <summary>
+            /// An operation potentially using <see cref="_singleBufferHandle"/> is in flight, but the field hasn't yet been initialized.
+            /// It's possible <see cref="_singleBufferHandle"/> will transition to <see cref="Set"/>, and thus code needs to wait for the
+            /// value to no longer be <see cref="InProcess"/> before <see cref="_singleBufferHandle"/> can be disposed.
+            /// </summary>
+            InProcess,
+            /// <summary>The <see cref="_singleBufferHandle"/> field has been initialized and requires disposal.  It is safe to dispose of when the operation no longer needs it.</summary>
+            Set
+        }
 
         // BufferList property variables.
         // Note that these arrays are allocated and then grown as necessary, but never shrunk.
@@ -101,7 +134,7 @@ private unsafe void FreeNativeOverlapped(NativeOverlapped* overlapped)
 
         private unsafe void RegisterToCancelPendingIO(NativeOverlapped* overlapped, CancellationToken cancellationToken)
         {
-            Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.InProcess);
+            Debug.Assert(_singleBufferHandleState == SingleBufferHandleState.InProcess, "An operation must be declared in-flight in order to register to cancel it.");
             Debug.Assert(_pendingOverlappedForCancellation == null);
             _pendingOverlappedForCancellation = overlapped;
             _registrationToCancelPendingIO = cancellationToken.UnsafeRegister(s =>
@@ -243,10 +276,15 @@ private unsafe SocketError ProcessIOCPResultWithSingleBufferHandle(SocketError s
             // Return pending and we will continue in the completion port callback.
             if (_singleBufferHandleState == SingleBufferHandleState.InProcess)
             {
-                RegisterToCancelPendingIO(overlapped, cancellationToken); // must happen before we change state to Set to avoid race conditions
+                // Register for cancellation.  This must happen before we change state to Set, as once it's Set,
+                // the operation completing asynchronously could invoke cleanup, which includes disposing of the
+                // cancellation registration, and thus the registration needs to be stored prior to setting Set.
+                RegisterToCancelPendingIO(overlapped, cancellationToken);
+
                 _singleBufferHandle = _buffer.Pin();
                 _singleBufferHandleState = SingleBufferHandleState.Set;
             }
+
             return SocketError.IOPending;
         }
 
@@ -1166,10 +1204,19 @@ private void CompleteCore()
             _strongThisRef.Value = null; // null out this reference from the overlapped so this isn't kept alive artificially
             if (_singleBufferHandleState != SingleBufferHandleState.None)
             {
+                // If the state isn't None, then either it's Set, in which case there's state to cleanup,
+                // or it's InProcess, which can happen if the async operation was scheduled and actually
+                // completed asynchronously (invoking this logic) but the main thread initiating the
+                // operation stalled and hasn't yet transitioned the memory handle to being initialized,
+                // in which case we need to wait for that logic to complete initializing it so that we
+                // can safely uninitialize it.
                 CompleteCoreSpin();
             }
 
-            void CompleteCoreSpin() // separate out to help inline the fast path
+            // Separate out to help inline the CompleteCore fast path, as CompleteCore is used with all operations.
+            // We want to optimize for the case where the async operation actually completes synchronously, in particular
+            // for sends and receives.
+            void CompleteCoreSpin()
             {
                 // The operation could complete so quickly that it races with the code
                 // initiating it.  Wait until that initiation code has completed before