[Release 2.1] Fix | Fix wrong data blended with transactions in .NET …

…core (dotnet#1023)
DavoudEshtehari · Apr 26, 2021 · 4fd3f72 · 4fd3f72
1 parent 4957501
commit 4fd3f72
Show file tree

Hide file tree

Showing 6 changed files with 152 additions and 6 deletions.
diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs
@@ -3404,7 +3404,9 @@ override public bool Read()
         private bool TryReadInternal(bool setTimeout, out bool more)
         {
             SqlStatistics statistics = null;
-            long scopeID = SqlClientEventSource.Log.TryScopeEnterEvent("<sc.SqlDataReader.Read|API> {0}", ObjectID);
+            long scopeID = SqlClientEventSource.Log.TryScopeEnterEvent("SqlDataReader.TryReadInternal | API | Object Id {0}", ObjectID);
+            RuntimeHelpers.PrepareConstrainedRegions();
+
             try
             {
                 statistics = SqlStatistics.StartTimer(Statistics);
@@ -3554,6 +3556,39 @@ private bool TryReadInternal(bool setTimeout, out bool more)
 
                 return true;
             }
+            catch (OutOfMemoryException e)
+            {
+                _isClosed = true;
+                SqlConnection con = _connection;
+                if (con != null)
+                {
+                    con.Abort(e);
+                }
+                throw;
+            }
+            catch (StackOverflowException e)
+            {
+                _isClosed = true;
+                SqlConnection con = _connection;
+                if (con != null)
+                {
+                    con.Abort(e);
+                }
+                throw;
+            }
+            /* Even though ThreadAbortException exists in .NET Core, 
+             * since Abort is not supported, the common language runtime won't ever throw ThreadAbortException.
+             * just to keep a common codebase!*/
+            catch (System.Threading.ThreadAbortException e)
+            {
+                _isClosed = true;
+                SqlConnection con = _connection;
+                if (con != null)
+                {
+                    con.Abort(e);
+                }
+                throw;
+            }
             finally
             {
                 SqlStatistics.StopTimer(statistics);

diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs
@@ -254,8 +254,10 @@ public void Rollback(SinglePhaseEnlistment enlistment)
                                 connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Rollback, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true);
                             }
                         }
-                        catch (SqlException)
+                        catch (SqlException e)
                         {
+                            ADP.TraceExceptionWithoutRethrow(e);
+
                             // Doom the connection, to make sure that the transaction is
                             // eventually rolled back.
                             // VSTS 144562: doom the connection while having the lock on it to prevent race condition with "Transaction Ended" Event
@@ -273,8 +275,9 @@ public void Rollback(SinglePhaseEnlistment enlistment)
                             // we have the tracing that we're doing to fallback on for the
                             // investigation.
                         }
-                        catch (InvalidOperationException)
+                        catch (InvalidOperationException e)
                         {
+                            ADP.TraceExceptionWithoutRethrow(e);
                             connection.DoomThisConnection();
                         }
                     }

diff --git a/...Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/...Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
@@ -1116,9 +1116,13 @@ bool isDelegateControlRequest
                         stateObj = _parser.GetSession(this);
                         mustPutSession = true;
                     }
-                    else if (internalTransaction.OpenResultsCount != 0)
+                    if (internalTransaction.OpenResultsCount != 0)
                     {
-                        //throw SQL.CannotCompleteDelegatedTransactionWithOpenResults(this);
+                        SqlClientEventSource.Log.TryTraceEvent("<sc.SqlInternalConnectionTds.ExecuteTransactionYukon|DATA|CATCH> {0}, Connection is marked to be doomed when closed. Transaction ended with OpenResultsCount {1} > 0, MARSOn {2}",
+                                                               ObjectID,
+                                                               internalTransaction.OpenResultsCount,
+                                                               _parser.MARSOn);
+                        throw SQL.CannotCompleteDelegatedTransactionWithOpenResults(this, _parser.MARSOn);
                     }
                 }
 

diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs
@@ -786,6 +786,13 @@ internal static Exception SqlDependencyNoMatchingServerDatabaseStart()
         //
         // SQL.SqlDelegatedTransaction
         //
+        static internal Exception CannotCompleteDelegatedTransactionWithOpenResults(SqlInternalConnectionTds internalConnection, bool marsOn)
+        {
+            SqlErrorCollection errors = new SqlErrorCollection();
+            errors.Add(new SqlError(TdsEnums.TIMEOUT_EXPIRED, (byte)0x00, TdsEnums.MIN_ERROR_CLASS, null, (StringsHelper.GetString(Strings.ADP_OpenReaderExists, marsOn ? ADP.Command : ADP.Connection)), "", 0, TdsEnums.SNI_WAIT_TIMEOUT));
+            return SqlException.CreateException(errors, null, internalConnection);
+        }
+
         internal static TransactionPromotionException PromotionFailed(Exception inner)
         {
             TransactionPromotionException e = new TransactionPromotionException(System.StringsHelper.GetString(Strings.SqlDelegatedTransaction_PromotionFailed), inner);

diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs
@@ -1380,8 +1380,12 @@ internal void ExecuteTransactionYukon(
                         stateObj = _parser.GetSession(this);
                         mustPutSession = true;
                     }
-                    else if (internalTransaction.OpenResultsCount != 0)
+                    if (internalTransaction.OpenResultsCount != 0)
                     {
+                        SqlClientEventSource.Log.TryTraceEvent("<sc.SqlInternalConnectionTds.ExecuteTransactionYukon|DATA|CATCH> {0}, Connection is marked to be doomed when closed. Transaction ended with OpenResultsCount {1} > 0, MARSOn {2}",
+                                                               ObjectID,
+                                                               internalTransaction.OpenResultsCount,
+                                                               _parser.MARSOn);
                         throw SQL.CannotCompleteDelegatedTransactionWithOpenResults(this, _parser.MARSOn);
                     }
                 }

diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionTest.cs
@@ -10,6 +10,99 @@ namespace Microsoft.Data.SqlClient.ManualTesting.Tests
 {
     public static class TransactionTest
     {
+        public static TheoryData<string> PoolEnabledConnectionStrings =>
+            new TheoryData<string>
+            {
+                new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString)
+                {
+                    MultipleActiveResultSets = false,
+                    Pooling = true,
+                    MaxPoolSize = 1
+                }.ConnectionString
+                , new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString)
+                {
+                    Pooling = true,
+                    MultipleActiveResultSets = true
+                }.ConnectionString
+            };
+
+        public static TheoryData<string> PoolDisabledConnectionStrings =>
+            new TheoryData<string>
+            {
+                new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString)
+                {
+                    Pooling = false,
+                    MultipleActiveResultSets = false
+                }.ConnectionString
+                , new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString)
+                {
+                    Pooling = false,
+                    MultipleActiveResultSets = true
+                }.ConnectionString
+            };
+
+        [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
+        [MemberData(nameof(PoolEnabledConnectionStrings))]
+        public static void ReadNextQueryAfterTxAbortedPoolEnabled(string connString)
+            => ReadNextQueryAfterTxAbortedTest(connString);
+
+        // Azure SQL has no DTC support
+        [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer))]        
+        [MemberData(nameof(PoolDisabledConnectionStrings))]
+        public static void ReadNextQueryAfterTxAbortedPoolDisabled(string connString)
+            => ReadNextQueryAfterTxAbortedTest(connString);
+
+        private static void ReadNextQueryAfterTxAbortedTest(string connString)
+        {
+            using (System.Transactions.TransactionScope scope = new System.Transactions.TransactionScope())
+            {
+                using (SqlConnection sqlConnection = new SqlConnection(connString))
+                {
+                    SqlCommand cmd = new SqlCommand("SELECT 1", sqlConnection);
+                    sqlConnection.Open();
+                    var reader = cmd.ExecuteReader();
+                    // Disposing Transaction Scope before completing read triggers GitHub issue #980 use-case that leads to wrong data in future rounds.
+                    scope.Dispose();
+                }
+
+                using (SqlConnection sqlConnection = new SqlConnection(connString))
+                using (SqlCommand cmd = new SqlCommand("SELECT 2", sqlConnection))
+                {
+                    sqlConnection.Open();
+                    using (SqlDataReader reader = cmd.ExecuteReader())
+                    {
+                        bool result = reader.Read();
+                        Assert.True(result);
+                        Assert.Equal(2, reader.GetValue(0));
+                    }
+                }
+
+                using (SqlConnection sqlConnection = new SqlConnection(connString))
+                using (SqlCommand cmd = new SqlCommand("SELECT 3", sqlConnection))
+                {
+                    sqlConnection.Open();
+                    using (SqlDataReader reader = cmd.ExecuteReaderAsync().Result)
+                    {
+                        bool result = reader.ReadAsync().Result;
+                        Assert.True(result);
+                        Assert.Equal(3, reader.GetValue(0));
+                    }
+                }
+
+                using (SqlConnection sqlConnection = new SqlConnection(connString))
+                using (SqlCommand cmd = new SqlCommand("SELECT TOP(1) 4 Clm0 FROM sysobjects FOR XML AUTO", sqlConnection))
+                {
+                    sqlConnection.Open();
+                    using (System.Xml.XmlReader reader = cmd.ExecuteXmlReader())
+                    {
+                        bool result = reader.Read();
+                        Assert.True(result);
+                        Assert.Equal("4", reader[0]);
+                    }
+                }
+            }
+        }
+
         // Synapse: Enforced unique constraints are not supported. To create an unenforced unique constraint you must include the NOT ENFORCED syntax as part of your statement.
         [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
         public static void TestMain()