From 2e04aa8b112272f857d3856f56f86e4d23eab73a Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 28 Apr 2021 11:18:59 -0700 Subject: [PATCH] [Release 2.1] Fix wrong data blended with transactions in .NET core (#1051) --- .../Microsoft/Data/SqlClient/SqlDataReader.cs | 37 +++++++- .../Data/SqlClient/SqlDelegatedTransaction.cs | 7 +- .../SqlClient/SqlInternalConnectionTds.cs | 8 +- .../src/Microsoft/Data/SqlClient/SqlUtil.cs | 7 ++ .../SqlClient/SqlInternalConnectionTds.cs | 6 +- .../SQL/TransactionTest/TransactionTest.cs | 93 +++++++++++++++++++ 6 files changed, 152 insertions(+), 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 index 674f7b1a2a..f339ff9a5c 100644 --- 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(" {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 index fe63845be9..8dac52d82b 100644 --- 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/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs index bc8c0655ff..1c0f50d8f8 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs +++ b/src/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(" {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 index bf21c8db3a..93de3ded09 100644 --- 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 index 381a87036e..609195eee2 100644 --- 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(" {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 index b223a709e9..3d5347f3a6 100644 --- 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 PoolEnabledConnectionStrings => + new TheoryData + { + new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + MultipleActiveResultSets = false, + Pooling = true, + MaxPoolSize = 1 + }.ConnectionString + , new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) + { + Pooling = true, + MultipleActiveResultSets = true + }.ConnectionString + }; + + public static TheoryData PoolDisabledConnectionStrings => + new TheoryData + { + 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()