Merge | TdsParserStateObject PacketHandle usage (#3353)

* Port PacketHandle to netfx

* Remove PacketHandle alias shim

* Align netfx use of PacketHandle

* Add high-level summary of PacketHandle types

Author: edwardneal

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

@@ -878,8 +878,8 @@
     <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\LocalDb\LocalDbApi.Windows.cs">
       <Link>Microsoft\Data\SqlClient\LocalDb\LocalDbApi.Windows.cs</Link>
     </Compile>
-    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\PacketHandle.netcore.Windows.cs">
-      <Link>Microsoft\Data\SqlClient\PacketHandle.netcore.Windows.cs</Link>
+    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\PacketHandle.Windows.cs">
+      <Link>Microsoft\Data\SqlClient\PacketHandle.Windows.cs</Link>
     </Compile>
     <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\SessionHandle.Windows.cs">
       <Link>Microsoft\Data\SqlClient\SessionHandle.Windows.cs</Link>

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj

@@ -447,6 +447,9 @@
     <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\Packet.cs">
       <Link>Microsoft\Data\SqlClient\Packet.cs</Link>
     </Compile>
+    <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\PacketHandle.Windows.cs">
+      <Link>Microsoft\Data\SqlClient\PacketHandle.Windows.cs</Link>
+    </Compile>
     <Compile Include="$(CommonSourceRoot)Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs">
       <Link>Microsoft\Data\SqlClient\ParameterPeekAheadValue.cs</Link>
     </Compile>

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.netfx.cs

@@ -18,8 +18,6 @@
 
 namespace Microsoft.Data.SqlClient
 {
-    using PacketHandle = IntPtr;
-
     internal partial class TdsParserStateObject
     {
         protected SNIHandle _sessionHandle = null;              // the SNI handle we're to work on
@@ -138,27 +136,35 @@ internal void CreatePhysicalSNIHandle(
                 ipPreference, cachedDNSInfo, hostNameInCertificate);
         }
 
-        internal bool IsPacketEmpty(PacketHandle readPacket) => readPacket == default;
+        internal bool IsPacketEmpty(PacketHandle readPacket)
+        {
+            Debug.Assert(readPacket.Type == PacketHandle.NativePointerType || readPacket.Type == 0, "unexpected packet type when requiring NativePointer");
+            return IntPtr.Zero == readPacket.NativePointer;
+        }
 
         internal PacketHandle ReadSyncOverAsync(int timeoutRemaining, out uint error)
         {
             SNIHandle handle = Handle ?? throw ADP.ClosedConnectionError();
-            PacketHandle readPacket = default;
-            error = SniNativeWrapper.SniReadSyncOverAsync(handle, ref readPacket, timeoutRemaining);
-            return readPacket;
+            IntPtr readPacketPtr = IntPtr.Zero;
+            error = SniNativeWrapper.SniReadSyncOverAsync(handle, ref readPacketPtr, timeoutRemaining);
+            return PacketHandle.FromNativePointer(readPacketPtr);
         }
 
         internal PacketHandle ReadAsync(SessionHandle handle, out uint error)
         {
-            PacketHandle readPacket = default;
-            error = SniNativeWrapper.SniReadAsync(handle.NativeHandle, ref readPacket);
-            return readPacket;
+            IntPtr readPacketPtr = IntPtr.Zero;
+            error = SniNativeWrapper.SniReadAsync(handle.NativeHandle, ref readPacketPtr);
+            return PacketHandle.FromNativePointer(readPacketPtr);
         }
 
         internal uint CheckConnection() => SniNativeWrapper.SniCheckConnection(Handle);
 
-        internal void ReleasePacket(PacketHandle syncReadPacket) => SniNativeWrapper.SniPacketRelease(syncReadPacket);
-        
+        internal void ReleasePacket(PacketHandle syncReadPacket)
+        {
+            Debug.Assert(syncReadPacket.Type == PacketHandle.NativePointerType, "unexpected packet type when requiring NativePointer");
+            SniNativeWrapper.SniPacketRelease(syncReadPacket.NativePointer);
+        }
+
         [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
         internal int DecrementPendingCallbacks(bool release)
         {
@@ -377,7 +383,13 @@ private void ReadSniError(TdsParserStateObject stateObj, uint error)
 
         private uint GetSniPacket(PacketHandle packet, ref uint dataSize)
         {
-            return SniNativeWrapper.SniPacketGetData(packet, _inBuff, ref dataSize);
+            return SniPacketGetData(packet, _inBuff, ref dataSize);
+        }
+
+        private uint SniPacketGetData(PacketHandle packet, byte[] _inBuff, ref uint dataSize)
+        {
+            Debug.Assert(packet.Type == PacketHandle.NativePointerType, "unexpected packet type when requiring NativePointer");
+            return SniNativeWrapper.SniPacketGetData(packet.NativePointer, _inBuff, ref dataSize);
         }
 
         public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error)
@@ -411,7 +423,7 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error)
             bool processFinallyBlock = true;
             try
             {
-                Debug.Assert(IntPtr.Zero == packet || IntPtr.Zero != packet && source != null, "AsyncResult null on callback");
+                Debug.Assert(CheckPacket(packet, source), "AsyncResult null on callback");
 
                 if (_parser.MARSOn)
                 {
@@ -481,6 +493,13 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error)
             }
         }
 
+        private bool CheckPacket(PacketHandle packet, TaskCompletionSource<object> source)
+        {
+            Debug.Assert(packet.Type == PacketHandle.NativePointerType, "unexpected packet type when requiring NativePointer");
+            IntPtr ptr = packet.NativePointer;
+            return IntPtr.Zero == ptr || IntPtr.Zero != ptr && source != null;
+        }
+
 #pragma warning disable 420 // a reference to a volatile field will not be treated as volatile
 
         public void WriteAsyncCallback(IntPtr key, PacketHandle packet, uint sniError)
@@ -654,7 +673,7 @@ internal Task WritePacket(byte flushMode, bool canAccumulate = false)
 
 #pragma warning disable 420 // a reference to a volatile field will not be treated as volatile
 
-        private Task SNIWritePacket(SNIPacket packet, out uint sniError, bool canAccumulate, bool callerHasConnectionLock, bool asyncClose = false)
+        private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccumulate, bool callerHasConnectionLock, bool asyncClose = false)
         {
             // Check for a stored exception
             Exception delayedException = Interlocked.Exchange(ref _delayedWriteAsyncCallbackException, null);
@@ -696,7 +715,8 @@ private Task SNIWritePacket(SNIPacket packet, out uint sniError, bool canAccumul
             }
             finally
             {
-                sniError = SniNativeWrapper.SniWritePacket(Handle, packet, sync);
+                Debug.Assert(packet.Type == PacketHandle.NativePacketType, "unexpected packet type when requiring NativePacket");
+                sniError = SniNativeWrapper.SniWritePacket(Handle, packet.NativePacket, sync);
             }
 
             if (sniError == TdsEnums.SNI_SUCCESS_IO_PENDING)
@@ -790,7 +810,15 @@ private Task SNIWritePacket(SNIPacket packet, out uint sniError, bool canAccumul
 
 #pragma warning restore 420
 
-        internal bool IsValidPacket(PacketHandle packetPointer) => packetPointer != default;
+        internal bool IsValidPacket(PacketHandle packetPointer)
+        {
+            Debug.Assert(packetPointer.Type == PacketHandle.NativePointerType || packetPointer.Type == PacketHandle.NativePacketType, "unexpected packet type when requiring NativePointer");
+            return (
+                (packetPointer.Type == PacketHandle.NativePointerType && packetPointer.NativePointer != IntPtr.Zero)
+                ||
+                (packetPointer.Type == PacketHandle.NativePacketType && packetPointer.NativePacket != null)
+            );
+        }
 
         // Sends an attention signal - executing thread will consume attn.
         internal void SendAttention(bool mustTakeWriteLock = false, bool asyncClose = false)
@@ -805,10 +833,7 @@ internal void SendAttention(bool mustTakeWriteLock = false, bool asyncClose = fa
                     return;
                 }
 
-                SNIPacket attnPacket = new SNIPacket(Handle);
-                _sniAsyncAttnPacket = attnPacket;
-
-                SniNativeWrapper.SniPacketSetData(attnPacket, SQL.AttentionHeader, TdsEnums.HEADER_LEN, null, null);
+                PacketHandle attnPacket = CreateAndSetAttentionPacket();
 
                 RuntimeHelpers.PrepareConstrainedRegions();
                 try
@@ -868,11 +893,20 @@ internal void SendAttention(bool mustTakeWriteLock = false, bool asyncClose = fa
             }
         }
 
+        internal PacketHandle CreateAndSetAttentionPacket()
+        {
+            SNIPacket attnPacket = new SNIPacket(Handle);
+            _sniAsyncAttnPacket = attnPacket;
+            SniNativeWrapper.SniPacketSetData(attnPacket, SQL.AttentionHeader, TdsEnums.HEADER_LEN, null, null);
+            return PacketHandle.FromNativePacket(attnPacket);
+        }
+
         private Task WriteSni(bool canAccumulate)
         {
             // Prepare packet, and write to packet.
-            SNIPacket packet = GetResetWritePacket();
-            SniNativeWrapper.SniPacketSetData(packet, _outBuff, _outBytesUsed, _securePasswords, _securePasswordOffsetsInBuffer);
+            PacketHandle packet = GetResetWritePacket();
+            SNIPacket nativePacket = packet.NativePacket;
+            SniNativeWrapper.SniPacketSetData(nativePacket, _outBuff, _outBytesUsed, _securePasswords, _securePasswordOffsetsInBuffer);
 
             Debug.Assert(Parser.Connection._parserLock.ThreadMayHaveLock(), "Thread is writing without taking the connection lock");
             Task task = SNIWritePacket(packet, out _, canAccumulate, callerHasConnectionLock: true);
@@ -923,7 +957,7 @@ private Task WriteSni(bool canAccumulate)
             return task;
         }
 
-        internal SNIPacket GetResetWritePacket()
+        internal PacketHandle GetResetWritePacket()
         {
             if (_sniPacket != null)
             {
@@ -936,7 +970,7 @@ internal SNIPacket GetResetWritePacket()
                     _sniPacket = _writePacketCache.Take(Handle);
                 }
             }
-            return _sniPacket;
+            return PacketHandle.FromNativePacket(_sniPacket);
         }
 
         internal void ClearAllWritePackets()
@@ -953,8 +987,10 @@ internal void ClearAllWritePackets()
             }
         }
 
-        private IntPtr AddPacketToPendingList(SNIPacket packet)
+        private PacketHandle AddPacketToPendingList(PacketHandle packetToAdd)
         {
+            Debug.Assert(packetToAdd.Type == PacketHandle.NativePacketType, "unexpected packet type when requiring NativePacket");
+            SNIPacket packet = packetToAdd.NativePacket;
             Debug.Assert(packet == _sniPacket, "Adding a packet other than the current packet to the pending list");
             _sniPacket = null;
             IntPtr pointer = packet.DangerousGetHandle();
@@ -964,16 +1000,17 @@ private IntPtr AddPacketToPendingList(SNIPacket packet)
                 _pendingWritePackets.Add(pointer, packet);
             }
 
-            return pointer;
+            return PacketHandle.FromNativePointer(pointer);
         }
 
-        private void RemovePacketFromPendingList(IntPtr pointer)
+        private void RemovePacketFromPendingList(PacketHandle ptr)
         {
-            SNIPacket recoveredPacket;
+            Debug.Assert(ptr.Type == PacketHandle.NativePointerType, "unexpected packet type when requiring NativePointer");
+            IntPtr pointer = ptr.NativePointer;
 
             lock (_writePacketLockObject)
             {
-                if (_pendingWritePackets.TryGetValue(pointer, out recoveredPacket))
+                if (_pendingWritePackets.TryGetValue(pointer, out SNIPacket recoveredPacket))
                 {
                     _pendingWritePackets.Remove(pointer);
                     _writePacketCache.Add(recoveredPacket);
@@ -1033,6 +1070,6 @@ private void SniWriteStatisticsAndTracing()
             SqlClientEventSource.Log.TryAdvancedTraceBinEvent("TdsParser.WritePacket | INFO | ADV | State Object Id {0}, Packet sent. Out buffer: {1}, Out Bytes Used: {2}", ObjectID, _outBuff, _outBytesUsed);
         }
 
-        protected PacketHandle EmptyReadPacket => default;
+        protected PacketHandle EmptyReadPacket => PacketHandle.FromNativePointer(default);
     }
 }

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.netcore.Windows.cs renamed to src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.Windows.cs

@@ -2,8 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#if NET
-
 using System;
 
 namespace Microsoft.Data.SqlClient
@@ -20,23 +18,49 @@ namespace Microsoft.Data.SqlClient
 
     internal readonly ref struct PacketHandle
     {
+        /// <summary>
+        /// PacketHandle is transporting a native pointer. The NativePointer field is valid.
+        /// A PacketHandle has this type when managed code is referencing a pointer to a
+        /// packet which has been read from the native SNI layer.
+        /// </summary>
         public const int NativePointerType = 1;
+        /// <summary>
+        /// PacketHandle is transporting a native packet. The NativePacket field is valid.
+        /// A PacketHandle has this type when managed code is directly referencing a packet
+        /// which is due to be passed to the native SNI layer.
+        /// </summary>
         public const int NativePacketType = 2;
+#if NET
+        /// <summary>
+        /// PacketHandle is transporting a managed packet. The ManagedPacket field is valid.
+        /// A PacketHandle used by the managed SNI layer will always have this type.
+        /// </summary>
         public const int ManagedPacketType = 3;
 
         public readonly SNI.SNIPacket ManagedPacket;
+#endif
         public readonly SNIPacket NativePacket;
         public readonly IntPtr NativePointer;
         public readonly int Type;
 
+#if NET
         private PacketHandle(IntPtr nativePointer, SNIPacket nativePacket, SNI.SNIPacket managedPacket, int type)
         {
             Type = type;
             ManagedPacket = managedPacket;
             NativePointer = nativePointer;
             NativePacket = nativePacket;
         }
+#else
+        private PacketHandle(IntPtr nativePointer, SNIPacket nativePacket, int type)
+        {
+            Type = type;
+            NativePointer = nativePointer;
+            NativePacket = nativePacket;
+        }
+#endif
 
+#if NET
         public static PacketHandle FromManagedPacket(SNI.SNIPacket managedPacket) =>
             new PacketHandle(default, default, managedPacket, ManagedPacketType);
 
@@ -45,9 +69,12 @@ public static PacketHandle FromNativePointer(IntPtr nativePointer) =>
 
         public static PacketHandle FromNativePacket(SNIPacket nativePacket) =>
             new PacketHandle(default, nativePacket, default, NativePacketType);
+#else
+        public static PacketHandle FromNativePointer(IntPtr nativePointer) =>
+            new PacketHandle(nativePointer, default, NativePointerType);
 
-
+        public static PacketHandle FromNativePacket(SNIPacket nativePacket) =>
+            new PacketHandle(default, nativePacket, NativePacketType);
+#endif
     }
 }
-
-#endif

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/PacketHandle.netcore.Unix.cs

@@ -18,8 +18,22 @@ namespace Microsoft.Data.SqlClient
 
     internal readonly ref struct PacketHandle
     {
+        /// <summary>
+        /// PacketHandle is transporting a native pointer. The NativePointer field is valid.
+        /// A PacketHandle has this type when managed code is referencing a pointer to a
+        /// packet which has been read from the native SNI layer.
+        /// </summary>
         public const int NativePointerType = 1;
+        /// <summary>
+        /// PacketHandle is transporting a native packet. The NativePacket field is valid.
+        /// A PacketHandle has this type when managed code is directly referencing a packet
+        /// which is due to be passed to the native SNI layer.
+        /// </summary>
         public const int NativePacketType = 2;
+        /// <summary>
+        /// PacketHandle is transporting a managed packet. The ManagedPacket field is valid.
+        /// A PacketHandle used by the managed SNI layer will always have this type.
+        /// </summary>
         public const int ManagedPacketType = 3;
 
         public readonly SNI.SNIPacket ManagedPacket;

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserSafeHandles.Windows.cs

@@ -107,11 +107,7 @@ private static void ReadDispatcher(IntPtr key, IntPtr packet, uint error)
 
                 if (stateObj != null)
                 {
-#if NETFRAMEWORK
-                    stateObj.ReadAsyncCallback(IntPtr.Zero, packet, error);
-#else
                     stateObj.ReadAsyncCallback(IntPtr.Zero, PacketHandle.FromNativePointer(packet), error);
-#endif // NETFRAMEWORK
                 }
             }
         }
@@ -132,11 +128,7 @@ private static void WriteDispatcher(IntPtr key, IntPtr packet, uint error)
 
                 if (stateObj != null)
                 {
-#if NETFRAMEWORK
-                    stateObj.WriteAsyncCallback(IntPtr.Zero, packet, error);
-#else
                     stateObj.WriteAsyncCallback(IntPtr.Zero, PacketHandle.FromNativePointer(packet), error);
-#endif // NETFRAMEWORK
                 }
             }
         }

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.Multiplexer.cs

@@ -7,9 +7,6 @@
 
 namespace Microsoft.Data.SqlClient
 {
-#if NETFRAMEWORK
-    using PacketHandle = IntPtr;
-#endif
     partial class TdsParserStateObject
     {
         private Packet _partialPacket;
@@ -513,12 +510,7 @@ public void ProcessSniPacketCompat(PacketHandle packet, uint error)
             else
             {
                 uint dataSize = 0;
-                
-                #if NETFRAMEWORK
-                uint getDataError = SniNativeWrapper.SniPacketGetData(packet, _inBuff, ref dataSize);
-                #else
                 uint getDataError = SniPacketGetData(packet, _inBuff, ref dataSize);
-                #endif
 
                 if (getDataError == TdsEnums.SNI_SUCCESS)
                 {

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

@@ -19,7 +19,6 @@
 namespace Microsoft.Data.SqlClient
 {
 #if NETFRAMEWORK
-    using PacketHandle = IntPtr;
     using RuntimeHelpers = System.Runtime.CompilerServices.RuntimeHelpers;
 #endif