Fix | Invalid transaction exception against the connections and distributed transactions (#2301) (#2321)

Javad · web-flow · commit f8520c7b9b65 · 2024-01-26T12:34:28.000-08:00
diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs
@@ -16,6 +16,7 @@ internal abstract partial class DbConnectionInternal
     {
         private static int _objectTypeCount;
         internal readonly int _objectID = Interlocked.Increment(ref _objectTypeCount);
+        private TransactionCompletedEventHandler _transactionCompletedEventHandler = null;
 
         private bool _isInStasis;
 
@@ -437,15 +438,19 @@ internal void DetachTransaction(Transaction transaction, bool isExplicitlyReleas
             // potentially a multi-threaded event, so lock the connection to make sure we don't enlist in a new
             // transaction between compare and assignment. No need to short circuit outside of lock, since failed comparisons should
             // be the exception, not the rule.
-            lock (this)
+            // locking on anything other than the transaction object would lead to a thread deadlock with sys.Transaction.TransactionCompleted event.
+            lock (transaction)
             {
                 // Detach if detach-on-end behavior, or if outer connection was closed
-                DbConnection owner = (DbConnection)Owner;
-                if (isExplicitlyReleasing || UnbindOnTransactionCompletion || null == owner)
+                DbConnection owner = Owner;
+                if (isExplicitlyReleasing || UnbindOnTransactionCompletion || owner is null)
                 {
                     Transaction currentEnlistedTransaction = _enlistedTransaction;
                     if (currentEnlistedTransaction != null && transaction.Equals(currentEnlistedTransaction))
                     {
+                        // We need to remove the transaction completed event handler to cease listening for the transaction to end.
+                        currentEnlistedTransaction.TransactionCompleted -= _transactionCompletedEventHandler;
+
                         EnlistedTransaction = null;
 
                         if (IsTxRootWaitingForTxEnd)
@@ -479,7 +484,8 @@ void TransactionCompletedEvent(object sender, TransactionEventArgs e)
 
         private void TransactionOutcomeEnlist(Transaction transaction)
         {
-            transaction.TransactionCompleted += new TransactionCompletedEventHandler(TransactionCompletedEvent);
+            _transactionCompletedEventHandler ??= new TransactionCompletedEventHandler(TransactionCompletedEvent);
+            transaction.TransactionCompleted += _transactionCompletedEventHandler;
         }
 
         internal void SetInStasis()
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
@@ -159,17 +159,17 @@ public byte[] Promote()
                             ValidateActiveOnConnection(connection);
 
                             connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Promote, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true);
-                            returnValue = _connection.PromotedDTCToken;
+                            returnValue = connection.PromotedDTCToken;
 
                             // For Global Transactions, we need to set the Transaction Id since we use a Non-MSDTC Promoter type.
-                            if (_connection.IsGlobalTransaction)
+                            if (connection.IsGlobalTransaction)
                             {
                                 if (SysTxForGlobalTransactions.SetDistributedTransactionIdentifier == null)
                                 {
                                     throw SQL.UnsupportedSysTxForGlobalTransactions();
                                 }
 
-                                if (!_connection.IsGlobalTransactionsEnabledForServer)
+                                if (!connection.IsGlobalTransactionsEnabledForServer)
                                 {
                                     throw SQL.GlobalTransactionsNotEnabled();
                                 }
diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs
@@ -18,11 +18,10 @@ namespace Microsoft.Data.ProviderBase
     using SysTx = System.Transactions;
 
     internal abstract class DbConnectionInternal
-    { // V1.1.3300
-
-
+    {
         private static int _objectTypeCount;
         internal readonly int _objectID = Interlocked.Increment(ref _objectTypeCount);
+        private SysTx.TransactionCompletedEventHandler _transactionCompletedEventHandler = null;
 
         internal static readonly StateChangeEventArgs StateChangeClosed = new StateChangeEventArgs(ConnectionState.Open, ConnectionState.Closed);
         internal static readonly StateChangeEventArgs StateChangeOpen = new StateChangeEventArgs(ConnectionState.Closed, ConnectionState.Open);
@@ -900,15 +899,18 @@ internal void DetachTransaction(SysTx.Transaction transaction, bool isExplicitly
             // potentially a multi-threaded event, so lock the connection to make sure we don't enlist in a new
             // transaction between compare and assignment. No need to short circuit outside of lock, since failed comparisons should
             // be the exception, not the rule.
-            lock (this)
+            // locking on anything other than the transaction object would lead to a thread deadlock with sys.Transaction.TransactionCompleted event.
+            lock (transaction)
             {
                 // Detach if detach-on-end behavior, or if outer connection was closed
-                DbConnection owner = (DbConnection)Owner;
-                if (isExplicitlyReleasing || UnbindOnTransactionCompletion || null == owner)
+                DbConnection owner = Owner;
+                if (isExplicitlyReleasing || UnbindOnTransactionCompletion || owner is null)
                 {
                     SysTx.Transaction currentEnlistedTransaction = _enlistedTransaction;
                     if (currentEnlistedTransaction != null && transaction.Equals(currentEnlistedTransaction))
                     {
+                        // We need to remove the transaction completed event handler to cease listening for the transaction to end.
+                        currentEnlistedTransaction.TransactionCompleted -= _transactionCompletedEventHandler;
 
                         EnlistedTransaction = null;
 
@@ -947,7 +949,8 @@ void TransactionCompletedEvent(object sender, SysTx.TransactionEventArgs e)
         [SecurityPermission(SecurityAction.Assert, Flags = SecurityPermissionFlag.UnmanagedCode)]
         private void TransactionOutcomeEnlist(SysTx.Transaction transaction)
         {
-            transaction.TransactionCompleted += new SysTx.TransactionCompletedEventHandler(TransactionCompletedEvent);
+            _transactionCompletedEventHandler ??= new SysTx.TransactionCompletedEventHandler(TransactionCompletedEvent);
+            transaction.TransactionCompleted += _transactionCompletedEventHandler;
         }
 
         internal void SetInStasis()
diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs
@@ -192,17 +192,17 @@ public Byte[] Promote()
                                 ValidateActiveOnConnection(connection);
 
                                 connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Promote, null, IsolationLevel.Unspecified, _internalTransaction, true);
-                                returnValue = _connection.PromotedDTCToken;
+                                returnValue = connection.PromotedDTCToken;
 
                                 // For Global Transactions, we need to set the Transaction Id since we use a Non-MSDTC Promoter type.
-                                if (_connection.IsGlobalTransaction)
+                                if (connection.IsGlobalTransaction)
                                 {
                                     if (SysTxForGlobalTransactions.SetDistributedTransactionIdentifier == null)
                                     {
                                         throw SQL.UnsupportedSysTxForGlobalTransactions();
                                     }
 
-                                    if (!_connection.IsGlobalTransactionsEnabledForServer)
+                                    if (!connection.IsGlobalTransactionsEnabledForServer)
                                     {
                                         throw SQL.GlobalTransactionsNotEnabled();
                                     }

Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,7 @@ internal abstract partial class DbConnectionInternal`
`16`	`16`	`{`
`17`	`17`	`private static int _objectTypeCount;`
`18`	`18`	`internal readonly int _objectID = Interlocked.Increment(ref _objectTypeCount);`
	`19`	`+ private TransactionCompletedEventHandler _transactionCompletedEventHandler = null;`
`19`	`20`
`20`	`21`	`private bool _isInStasis;`
`21`	`22`
`@@ -437,15 +438,19 @@ internal void DetachTransaction(Transaction transaction, bool isExplicitlyReleas`
`437`	`438`	`// potentially a multi-threaded event, so lock the connection to make sure we don't enlist in a new`
`438`	`439`	`// transaction between compare and assignment. No need to short circuit outside of lock, since failed comparisons should`
`439`	`440`	`// be the exception, not the rule.`
`440`		`- lock (this)`
	`441`	`+ // locking on anything other than the transaction object would lead to a thread deadlock with sys.Transaction.TransactionCompleted event.`
	`442`	`+ lock (transaction)`
`441`	`443`	`{`
`442`	`444`	`// Detach if detach-on-end behavior, or if outer connection was closed`
`443`		`- DbConnection owner = (DbConnection)Owner;`
`444`		`- if (isExplicitlyReleasing \|\| UnbindOnTransactionCompletion \|\| null == owner)`
	`445`	`+ DbConnection owner = Owner;`
	`446`	`+ if (isExplicitlyReleasing \|\| UnbindOnTransactionCompletion \|\| owner is null)`
`445`	`447`	`{`
`446`	`448`	`Transaction currentEnlistedTransaction = _enlistedTransaction;`
`447`	`449`	`if (currentEnlistedTransaction != null && transaction.Equals(currentEnlistedTransaction))`
`448`	`450`	`{`
	`451`	`+ // We need to remove the transaction completed event handler to cease listening for the transaction to end.`
	`452`	`+ currentEnlistedTransaction.TransactionCompleted -= _transactionCompletedEventHandler;`
	`453`	`+`
`449`	`454`	`EnlistedTransaction = null;`
`450`	`455`
`451`	`456`	`if (IsTxRootWaitingForTxEnd)`
`@@ -479,7 +484,8 @@ void TransactionCompletedEvent(object sender, TransactionEventArgs e)`
`479`	`484`
`480`	`485`	`private void TransactionOutcomeEnlist(Transaction transaction)`
`481`	`486`	`{`
`482`		`- transaction.TransactionCompleted += new TransactionCompletedEventHandler(TransactionCompletedEvent);`
	`487`	`+ _transactionCompletedEventHandler ??= new TransactionCompletedEventHandler(TransactionCompletedEvent);`
	`488`	`+ transaction.TransactionCompleted += _transactionCompletedEventHandler;`
`483`	`489`	`}`
`484`	`490`
`485`	`491`	`internal void SetInStasis()`
Original file line number	Diff line number	Diff line change
`@@ -159,17 +159,17 @@ public byte[] Promote()`
`159`	`159`	`ValidateActiveOnConnection(connection);`
`160`	`160`
`161`	`161`	`connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Promote, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true);`
`162`		`- returnValue = _connection.PromotedDTCToken;`
	`162`	`+ returnValue = connection.PromotedDTCToken;`
`163`	`163`
`164`	`164`	`// For Global Transactions, we need to set the Transaction Id since we use a Non-MSDTC Promoter type.`
`165`		`- if (_connection.IsGlobalTransaction)`
	`165`	`+ if (connection.IsGlobalTransaction)`
`166`	`166`	`{`
`167`	`167`	`if (SysTxForGlobalTransactions.SetDistributedTransactionIdentifier == null)`
`168`	`168`	`{`
`169`	`169`	`throw SQL.UnsupportedSysTxForGlobalTransactions();`
`170`	`170`	`}`
`171`	`171`
`172`		`- if (!_connection.IsGlobalTransactionsEnabledForServer)`
	`172`	`+ if (!connection.IsGlobalTransactionsEnabledForServer)`
`173`	`173`	`{`
`174`	`174`	`throw SQL.GlobalTransactionsNotEnabled();`
`175`	`175`	`}`
Original file line number	Diff line number	Diff line change
`@@ -192,17 +192,17 @@ public Byte[] Promote()`
`192`	`192`	`ValidateActiveOnConnection(connection);`
`193`	`193`
`194`	`194`	`connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Promote, null, IsolationLevel.Unspecified, _internalTransaction, true);`
`195`		`- returnValue = _connection.PromotedDTCToken;`
	`195`	`+ returnValue = connection.PromotedDTCToken;`
`196`	`196`
`197`	`197`	`// For Global Transactions, we need to set the Transaction Id since we use a Non-MSDTC Promoter type.`
`198`		`- if (_connection.IsGlobalTransaction)`
	`198`	`+ if (connection.IsGlobalTransaction)`
`199`	`199`	`{`
`200`	`200`	`if (SysTxForGlobalTransactions.SetDistributedTransactionIdentifier == null)`
`201`	`201`	`{`
`202`	`202`	`throw SQL.UnsupportedSysTxForGlobalTransactions();`
`203`	`203`	`}`
`204`	`204`
`205`		`- if (!_connection.IsGlobalTransactionsEnabledForServer)`
	`205`	`+ if (!connection.IsGlobalTransactionsEnabledForServer)`
`206`	`206`	`{`
`207`	`207`	`throw SQL.GlobalTransactionsNotEnabled();`
`208`	`208`	`}`