From 8fa8c265584b1a53a25ff794e6e5784fa11acc41 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 20 Jan 2021 01:28:44 +0000 Subject: [PATCH 1/4] safety copy sessio handle and ensure any dropped packet is cleared --- .../SqlClient/TdsParserStateObjectManaged.cs | 40 +++++++++++++++++-- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs index 9e9e281a32..091c9520ad 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs @@ -71,14 +71,46 @@ internal override void AssignPendingDNSInfo(string userProtocol, string DNSCache internal void ReadAsyncCallback(SNIPacket packet, uint error) { - ReadAsyncCallback(IntPtr.Zero, PacketHandle.FromManagedPacket(packet), error); - _sessionHandle?.ReturnPacket(packet); + SNIHandle sessionHandle = _sessionHandle; + if (sessionHandle != null) + { + ReadAsyncCallback(IntPtr.Zero, PacketHandle.FromManagedPacket(packet), error); + sessionHandle?.ReturnPacket(packet); + } + else + { + // clear the packet and drop it to GC because we no longer know how to return it to the correct owner + // this can only happen if a packet is in-flight when the _sessionHandle is cleared + try + { + packet.Release(); + } + catch + { + } + } } internal void WriteAsyncCallback(SNIPacket packet, uint sniError) { - WriteAsyncCallback(IntPtr.Zero, PacketHandle.FromManagedPacket(packet), sniError); - _sessionHandle?.ReturnPacket(packet); + SNIHandle sessionHandle = _sessionHandle; + if (sessionHandle != null) + { + WriteAsyncCallback(IntPtr.Zero, PacketHandle.FromManagedPacket(packet), sniError); + sessionHandle?.ReturnPacket(packet); + } + else + { + // clear the packet and drop it to GC because we no longer know how to return it to the correct owner + // this can only happen if a packet is in-flight when the _sessionHandle is cleared + try + { + packet.Release(); + } + catch + { + } + } } protected override void RemovePacketFromPendingList(PacketHandle packet) From 68f038bfa9060e36100a39113f3f8be41049dd05 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 20 Jan 2021 09:52:37 +0000 Subject: [PATCH 2/4] remove unneeded error handling and add debug assertion to catch incorrectly dropped packets --- .../Microsoft/Data/SqlClient/SNI/SNIPacket.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs index 0ab375b129..431140cf61 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs @@ -2,7 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -// #define TRACE_HISTORY // this is used for advanced debugging when you need to trace the entire lifetime of a single packet, be very careful with it + // #define TRACE_HISTORY // this is used for advanced debugging when you need to trace the entire lifetime of a single packet, be very careful with it using System; using System.Buffers; @@ -41,15 +41,15 @@ internal struct History { public enum Direction { - Rent=0, - Return=1, + Rent = 0, + Return = 1, } public Direction Action; public int RefCount; public string Stack; } - + internal List _history = null; /// @@ -62,7 +62,7 @@ public SNIPacket(SNIHandle owner,int id) : this() { #if TRACE_HISTORY - _history = new List(); + _history = new List(); #endif _id = id; _owner = owner; @@ -284,6 +284,12 @@ private void ReadFromStreamAsyncContinuation(Task t, object state) } callback(this, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); +#if DEBUG + if (!IsInvalid) + { + Debug.Fail("unreleased packet will be dropped to GC"); + } +#endif } /// From ac3805b6861286246a6dc2c86a7cbdda72b1207c Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 20 Jan 2021 20:19:15 +0000 Subject: [PATCH 3/4] remove try catch --- .../SqlClient/TdsParserStateObjectManaged.cs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs index 091c9520ad..06c32d0e4d 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs @@ -81,13 +81,7 @@ internal void ReadAsyncCallback(SNIPacket packet, uint error) { // clear the packet and drop it to GC because we no longer know how to return it to the correct owner // this can only happen if a packet is in-flight when the _sessionHandle is cleared - try - { - packet.Release(); - } - catch - { - } + packet.Release(); } } @@ -103,13 +97,7 @@ internal void WriteAsyncCallback(SNIPacket packet, uint sniError) { // clear the packet and drop it to GC because we no longer know how to return it to the correct owner // this can only happen if a packet is in-flight when the _sessionHandle is cleared - try - { - packet.Release(); - } - catch - { - } + packet.Release(); } } From 4f79d5028e794ab82a3c833de6a664ffca5fd08d Mon Sep 17 00:00:00 2001 From: Wraith Date: Wed, 20 Jan 2021 22:40:00 +0000 Subject: [PATCH 4/4] Update SNIPacket.cs remove debug assert because it fails in Mars scenarios --- .../netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs index 431140cf61..3d8d6a90c3 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs @@ -284,12 +284,6 @@ private void ReadFromStreamAsyncContinuation(Task t, object state) } callback(this, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); -#if DEBUG - if (!IsInvalid) - { - Debug.Fail("unreleased packet will be dropped to GC"); - } -#endif } ///