From c7026fb71ee94b2b4c4a7eba298bfc70a956edf7 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 27 Mar 2020 23:57:12 -0700 Subject: [PATCH 1/5] Fix Promote Transaction issue --- .../src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs | 2 ++ .../src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs | 2 ++ 2 files changed, 4 insertions(+) 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 acfe827b48..9a26596b8c 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 @@ -413,6 +413,8 @@ internal void TransactionEnded(Transaction transaction) _active = false; _connection = null; } + // Safest approach is to doom this connection, whose transaction has been aborted externally. + connection.DoomThisConnection(); } } } 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 index 21a5966f3e..ecfa696055 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs @@ -504,6 +504,8 @@ internal void TransactionEnded(SysTx.Transaction transaction) _active = false; _connection = null; } + // Safest approach is to doom this connection, whose transaction has been aborted externally. + connection.DoomThisConnection(); } } } From 2f59a6f4b00fd53b2d2322271e6b1f3f9681bfd7 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Thu, 2 Apr 2020 16:53:09 -0700 Subject: [PATCH 2/5] Changes to not throw exception is Abort cannot be done. --- .../Data/SqlClient/SqlDelegatedTransaction.cs | 50 +++++++++++-------- 1 file changed, 28 insertions(+), 22 deletions(-) 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 index ecfa696055..64f6d998e7 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs @@ -265,11 +265,7 @@ public Byte[] Promote() public void Rollback(SysTx.SinglePhaseEnlistment enlistment) { Debug.Assert(null != enlistment, "null enlistment?"); - - SqlInternalConnection connection = GetValidConnection(); - SqlConnection usersConnection = connection.Connection; - SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborting transaction.", ObjectID, connection.ObjectID); - RuntimeHelpers.PrepareConstrainedRegions(); + SqlInternalConnection connection = null; try { @@ -277,12 +273,13 @@ public void Rollback(SysTx.SinglePhaseEnlistment enlistment) TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection(); RuntimeHelpers.PrepareConstrainedRegions(); + tdsReliabilitySection.Start(); +#endif //DEBUG + connection = GetValidConnection(); + SqlConnection usersConnection = connection.Connection; + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborting transaction.", ObjectID, connection.ObjectID); try { - tdsReliabilitySection.Start(); -#else - { -#endif //DEBUG lock (connection) { try @@ -331,6 +328,21 @@ public void Rollback(SysTx.SinglePhaseEnlistment enlistment) connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); enlistment.Aborted(); } + catch (System.OutOfMemoryException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.StackOverflowException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.Threading.ThreadAbortException e) + { + usersConnection.Abort(e); + throw; + } #if DEBUG finally { @@ -338,20 +350,14 @@ public void Rollback(SysTx.SinglePhaseEnlistment enlistment) } #endif //DEBUG } - catch (System.OutOfMemoryException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.StackOverflowException e) + catch (ObjectDisposedException) { - usersConnection.Abort(e); - throw; - } - catch (System.Threading.ThreadAbortException e) - { - usersConnection.Abort(e); - throw; + if (_atomicTransaction.TransactionInformation.Status == SysTx.TransactionStatus.Active) + { + throw; + } + // Do not throw exception if connection is already aborted/ended from another thread, + // replicate as done in TransactionEnded event handler. } } From f34cdf871fc406460bc07c5fcca45924b95cd427 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Thu, 2 Apr 2020 18:08:55 -0700 Subject: [PATCH 3/5] Port changes to NetCore --- .../Data/SqlClient/SqlDelegatedTransaction.cs | 119 ++++++++++-------- 1 file changed, 66 insertions(+), 53 deletions(-) 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 9a26596b8c..dfa52afc33 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 @@ -217,72 +217,85 @@ public byte[] Promote() public void Rollback(SinglePhaseEnlistment enlistment) { Debug.Assert(null != enlistment, "null enlistment?"); + SqlInternalConnection connection = null; - SqlInternalConnection connection = GetValidConnection(); - SqlConnection usersConnection = connection.Connection; - SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborting transaction.", ObjectID, connection.ObjectID); - RuntimeHelpers.PrepareConstrainedRegions(); try { - lock (connection) + connection = GetValidConnection(); + SqlConnection usersConnection = connection.Connection; + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborting transaction.", ObjectID, connection.ObjectID); + RuntimeHelpers.PrepareConstrainedRegions(); + try { - try + lock (connection) { - // Now that we've acquired the lock, make sure we still have valid state for this operation. - ValidateActiveOnConnection(connection); - _active = false; // set to inactive first, doesn't matter how the execute completes, this transaction is done. - _connection = null; // Set prior to ExecuteTransaction call in case this initiates a TransactionEnd event + try + { + // Now that we've acquired the lock, make sure we still have valid state for this operation. + ValidateActiveOnConnection(connection); + _active = false; // set to inactive first, doesn't matter how the execute completes, this transaction is done. + _connection = null; // Set prior to ExecuteTransaction call in case this initiates a TransactionEnd event + + // If we haven't already rolled back (or aborted) then tell the SQL Server to roll back + if (!_internalTransaction.IsAborted) + { + connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Rollback, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true); + } + } + catch (SqlException) + { + // 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 + connection.DoomThisConnection(); - // If we haven't already rolled back (or aborted) then tell the SQL Server to roll back - if (!_internalTransaction.IsAborted) + // Unlike SinglePhaseCommit, a rollback is a rollback, regardless + // of how it happens, so SysTx won't throw an exception, and we + // don't want to throw an exception either, because SysTx isn't + // handling it and it may create a fail fast scenario. In the end, + // there is no way for us to communicate to the consumer that this + // failed for more serious reasons than usual. + // + // This is a bit like "should you throw if Close fails", however, + // it only matters when you really need to know. In that case, + // we have the tracing that we're doing to fallback on for the + // investigation. + } + catch (InvalidOperationException) { - connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Rollback, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true); + connection.DoomThisConnection(); } } - catch (SqlException) - { - // 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 - connection.DoomThisConnection(); - // Unlike SinglePhaseCommit, a rollback is a rollback, regardless - // of how it happens, so SysTx won't throw an exception, and we - // don't want to throw an exception either, because SysTx isn't - // handling it and it may create a fail fast scenario. In the end, - // there is no way for us to communicate to the consumer that this - // failed for more serious reasons than usual. - // - // This is a bit like "should you throw if Close fails", however, - // it only matters when you really need to know. In that case, - // we have the tracing that we're doing to fallback on for the - // investigation. - } - catch (InvalidOperationException) - { - connection.DoomThisConnection(); - } + // it doesn't matter whether the rollback succeeded or not, we presume + // that the transaction is aborted, because it will be eventually. + connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); + enlistment.Aborted(); + } + catch (System.OutOfMemoryException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.StackOverflowException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.Threading.ThreadAbortException e) + { + usersConnection.Abort(e); + throw; } - - // it doesn't matter whether the rollback succeeded or not, we presume - // that the transaction is aborted, because it will be eventually. - connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); - enlistment.Aborted(); - } - catch (System.OutOfMemoryException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.StackOverflowException e) - { - usersConnection.Abort(e); - throw; } - catch (System.Threading.ThreadAbortException e) + catch (ObjectDisposedException) { - usersConnection.Abort(e); - throw; + if (_atomicTransaction.TransactionInformation.Status == TransactionStatus.Active) + { + throw; + } + // Do not throw exception if connection is already aborted/ended from another thread, + // replicate as done in TransactionEnded event handler. } } From 2fd5e75e0dd3d2a6bf33b72f0da26226053b1da1 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 3 Apr 2020 11:08:01 -0700 Subject: [PATCH 4/5] Additional checks to handle external connection abort. --- .../Data/SqlClient/SqlDelegatedTransaction.cs | 312 ++++++++------- .../Data/SqlClient/SqlDelegatedTransaction.cs | 360 +++++++++--------- 2 files changed, 353 insertions(+), 319 deletions(-) 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 dfa52afc33..6a4ede69fd 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 @@ -137,79 +137,92 @@ public byte[] Promote() // from an operational standpoint (i.e. logging connection's ObjectID should be fine, // but the PromotedDTCToken can change over calls. so that must be protected). SqlInternalConnection connection = GetValidConnection(); - Exception promoteException; byte[] returnValue = null; - SqlConnection usersConnection = connection.Connection; - SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, promoting transaction.", ObjectID, connection.ObjectID); - RuntimeHelpers.PrepareConstrainedRegions(); - try + + if (null != connection) { - lock (connection) + SqlConnection usersConnection = connection.Connection; + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, promoting transaction.", ObjectID, connection.ObjectID); + RuntimeHelpers.PrepareConstrainedRegions(); + try { - try + lock (connection) { - // Now that we've acquired the lock, make sure we still have valid state for this operation. - ValidateActiveOnConnection(connection); + try + { + // Now that we've acquired the lock, make sure we still have valid state for this operation. + ValidateActiveOnConnection(connection); - connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Promote, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true); - returnValue = _connection.PromotedDTCToken; + connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Promote, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true); + 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 (SysTxForGlobalTransactions.SetDistributedTransactionIdentifier == null) + // For Global Transactions, we need to set the Transaction Id since we use a Non-MSDTC Promoter type. + if (_connection.IsGlobalTransaction) { - throw SQL.UnsupportedSysTxForGlobalTransactions(); - } + if (SysTxForGlobalTransactions.SetDistributedTransactionIdentifier == null) + { + throw SQL.UnsupportedSysTxForGlobalTransactions(); + } - if (!_connection.IsGlobalTransactionsEnabledForServer) - { - throw SQL.GlobalTransactionsNotEnabled(); + if (!_connection.IsGlobalTransactionsEnabledForServer) + { + throw SQL.GlobalTransactionsNotEnabled(); + } + + SysTxForGlobalTransactions.SetDistributedTransactionIdentifier.Invoke(_atomicTransaction, new object[] { this, GetGlobalTxnIdentifierFromToken() }); } - SysTxForGlobalTransactions.SetDistributedTransactionIdentifier.Invoke(_atomicTransaction, new object[] { this, GetGlobalTxnIdentifierFromToken() }); + promoteException = null; } + catch (SqlException e) + { + promoteException = e; - promoteException = null; + // 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 + connection.DoomThisConnection(); + } + catch (InvalidOperationException e) + { + promoteException = e; + connection.DoomThisConnection(); + } } - catch (SqlException e) - { - promoteException = e; + } + catch (System.OutOfMemoryException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.StackOverflowException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.Threading.ThreadAbortException e) + { + usersConnection.Abort(e); + throw; + } - // 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 - connection.DoomThisConnection(); - } - catch (InvalidOperationException e) - { - promoteException = e; - connection.DoomThisConnection(); - } + //Throw exception only if Transaction is till active and not yet aborted. + if (promoteException != null && Transaction.TransactionInformation.Status != TransactionStatus.Aborted) + { + throw SQL.PromotionFailed(promoteException); + } + else + { + // The transaction was aborted externally, since it's already doomed above, we only log the same. + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborted during promotion.", ObjectID, connection.ObjectID); } } - catch (System.OutOfMemoryException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.StackOverflowException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.Threading.ThreadAbortException e) - { - usersConnection.Abort(e); - throw; - } - - if (promoteException != null) + else { - throw SQL.PromotionFailed(promoteException); + // The transaction was aborted externally, doom the connection to make sure it's eventually rolled back and log the same. + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborted before promoting.", ObjectID, connection.ObjectID); } - return returnValue; } @@ -217,13 +230,12 @@ public byte[] Promote() public void Rollback(SinglePhaseEnlistment enlistment) { Debug.Assert(null != enlistment, "null enlistment?"); - SqlInternalConnection connection = null; + SqlInternalConnection connection = GetValidConnection(); - try + if (null != connection) { - connection = GetValidConnection(); SqlConnection usersConnection = connection.Connection; - SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborting transaction.", ObjectID, connection.ObjectID); + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, rolling back transaction.", ObjectID, connection.ObjectID); RuntimeHelpers.PrepareConstrainedRegions(); try { @@ -288,14 +300,11 @@ public void Rollback(SinglePhaseEnlistment enlistment) throw; } } - catch (ObjectDisposedException) + else { - if (_atomicTransaction.TransactionInformation.Status == TransactionStatus.Active) - { - throw; - } - // Do not throw exception if connection is already aborted/ended from another thread, - // replicate as done in TransactionEnded event handler. + // The transaction was aborted, report that to SysTx and log the same. + enlistment.Aborted(); + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborted before rollback.", ObjectID, connection.ObjectID); } } @@ -303,107 +312,116 @@ public void Rollback(SinglePhaseEnlistment enlistment) public void SinglePhaseCommit(SinglePhaseEnlistment enlistment) { Debug.Assert(null != enlistment, "null enlistment?"); - SqlInternalConnection connection = GetValidConnection(); - SqlConnection usersConnection = connection.Connection; - SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, committing transaction.", ObjectID, connection.ObjectID); - RuntimeHelpers.PrepareConstrainedRegions(); - try - { - // If the connection is dooomed, we can be certain that the - // transaction will eventually be rolled back, and we shouldn't - // attempt to commit it. - if (connection.IsConnectionDoomed) - { - lock (connection) - { - _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. - _connection = null; - } - enlistment.Aborted(SQL.ConnectionDoomed()); - } - else + if (null != connection) + { + SqlConnection usersConnection = connection.Connection; + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, committing transaction.", ObjectID, connection.ObjectID); + RuntimeHelpers.PrepareConstrainedRegions(); + try { - Exception commitException; - lock (connection) + // If the connection is dooomed, we can be certain that the + // transaction will eventually be rolled back, and we shouldn't + // attempt to commit it. + if (connection.IsConnectionDoomed) { - try + lock (connection) { - // Now that we've acquired the lock, make sure we still have valid state for this operation. - ValidateActiveOnConnection(connection); - _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. - _connection = null; // Set prior to ExecuteTransaction call in case this initiates a TransactionEnd event - - connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Commit, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true); - commitException = null; + _connection = null; } - catch (SqlException e) - { - commitException = 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 - connection.DoomThisConnection(); - } - catch (InvalidOperationException e) - { - commitException = e; - connection.DoomThisConnection(); - } + enlistment.Aborted(SQL.ConnectionDoomed()); } - if (commitException != null) + else { - // connection.ExecuteTransaction failed with exception - if (_internalTransaction.IsCommitted) + Exception commitException; + lock (connection) { - // Even though we got an exception, the transaction - // was committed by the server. - enlistment.Committed(); + try + { + // Now that we've acquired the lock, make sure we still have valid state for this operation. + ValidateActiveOnConnection(connection); + + _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. + _connection = null; // Set prior to ExecuteTransaction call in case this initiates a TransactionEnd event + + connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Commit, null, System.Data.IsolationLevel.Unspecified, _internalTransaction, true); + commitException = null; + } + catch (SqlException e) + { + commitException = 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 + connection.DoomThisConnection(); + } + catch (InvalidOperationException e) + { + commitException = e; + connection.DoomThisConnection(); + } } - else if (_internalTransaction.IsAborted) + if (commitException != null) { - // The transaction was aborted, report that to - // SysTx. - enlistment.Aborted(commitException); + // connection.ExecuteTransaction failed with exception + if (_internalTransaction.IsCommitted) + { + // Even though we got an exception, the transaction + // was committed by the server. + enlistment.Committed(); + } + else if (_internalTransaction.IsAborted) + { + // The transaction was aborted, report that to + // SysTx. + enlistment.Aborted(commitException); + } + else + { + // The transaction is still active, we cannot + // know the state of the transaction. + enlistment.InDoubt(commitException); + } + + // We eat the exception. This is called on the SysTx + // thread, not the applications thread. If we don't + // eat the exception an UnhandledException will occur, + // causing the process to FailFast. } - else + + connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); + if (commitException == null) { - // The transaction is still active, we cannot - // know the state of the transaction. - enlistment.InDoubt(commitException); + // connection.ExecuteTransaction succeeded + enlistment.Committed(); } - - // We eat the exception. This is called on the SysTx - // thread, not the applications thread. If we don't - // eat the exception an UnhandledException will occur, - // causing the process to FailFast. - } - - connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); - if (commitException == null) - { - // connection.ExecuteTransaction succeeded - enlistment.Committed(); } } + catch (System.OutOfMemoryException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.StackOverflowException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.Threading.ThreadAbortException e) + { + usersConnection.Abort(e); + throw; + } } - catch (System.OutOfMemoryException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.StackOverflowException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.Threading.ThreadAbortException e) + else { - usersConnection.Abort(e); - throw; + // The transaction was aborted before we could commit, report that to SysTx and log the same. + enlistment.Aborted(); + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborted before commit.", ObjectID, connection.ObjectID); } } @@ -436,7 +454,7 @@ internal void TransactionEnded(Transaction transaction) private SqlInternalConnection GetValidConnection() { SqlInternalConnection connection = _connection; - if (null == connection) + if (null == connection && Transaction.TransactionInformation.Status != TransactionStatus.Aborted) { throw ADP.ObjectDisposed(this); } 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 index 64f6d998e7..05cb15960a 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs @@ -167,97 +167,110 @@ public Byte[] Promote() Exception promoteException; byte[] returnValue = null; - SqlConnection usersConnection = connection.Connection; - SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, promoting transaction.", ObjectID, connection.ObjectID); - RuntimeHelpers.PrepareConstrainedRegions(); - - try + if (null != connection) { -#if DEBUG - TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection(); - + SqlConnection usersConnection = connection.Connection; + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, promoting transaction.", ObjectID, connection.ObjectID); RuntimeHelpers.PrepareConstrainedRegions(); + try { - tdsReliabilitySection.Start(); +#if DEBUG + TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection(); + + RuntimeHelpers.PrepareConstrainedRegions(); + try + { + tdsReliabilitySection.Start(); #else { #endif //DEBUG - lock (connection) - { - try + lock (connection) { - // Now that we've acquired the lock, make sure we still have valid state for this operation. - ValidateActiveOnConnection(connection); + try + { + // Now that we've acquired the lock, make sure we still have valid state for this operation. + ValidateActiveOnConnection(connection); - connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Promote, null, IsolationLevel.Unspecified, _internalTransaction, true); - returnValue = _connection.PromotedDTCToken; + connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Promote, null, IsolationLevel.Unspecified, _internalTransaction, true); + 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 (SysTxForGlobalTransactions.SetDistributedTransactionIdentifier == null) + // For Global Transactions, we need to set the Transaction Id since we use a Non-MSDTC Promoter type. + if (_connection.IsGlobalTransaction) { - throw SQL.UnsupportedSysTxForGlobalTransactions(); - } + if (SysTxForGlobalTransactions.SetDistributedTransactionIdentifier == null) + { + throw SQL.UnsupportedSysTxForGlobalTransactions(); + } - if (!_connection.IsGlobalTransactionsEnabledForServer) - { - throw SQL.GlobalTransactionsNotEnabled(); + if (!_connection.IsGlobalTransactionsEnabledForServer) + { + throw SQL.GlobalTransactionsNotEnabled(); + } + + SysTxForGlobalTransactions.SetDistributedTransactionIdentifier.Invoke(_atomicTransaction, new object[] { this, GetGlobalTxnIdentifierFromToken() }); } - SysTxForGlobalTransactions.SetDistributedTransactionIdentifier.Invoke(_atomicTransaction, new object[] { this, GetGlobalTxnIdentifierFromToken() }); + promoteException = null; } + catch (SqlException e) + { + promoteException = e; - promoteException = null; - } - catch (SqlException e) - { - promoteException = e; - - ADP.TraceExceptionWithoutRethrow(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 - connection.DoomThisConnection(); - } - catch (InvalidOperationException e) - { - promoteException = e; - ADP.TraceExceptionWithoutRethrow(e); - connection.DoomThisConnection(); + // 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 + connection.DoomThisConnection(); + } + catch (InvalidOperationException e) + { + promoteException = e; + ADP.TraceExceptionWithoutRethrow(e); + connection.DoomThisConnection(); + } } } - } #if DEBUG - finally + finally + { + tdsReliabilitySection.Stop(); + } +#endif //DEBUG + } + catch (System.OutOfMemoryException e) { - tdsReliabilitySection.Stop(); + usersConnection.Abort(e); + throw; + } + catch (System.StackOverflowException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.Threading.ThreadAbortException e) + { + usersConnection.Abort(e); + throw; } -#endif //DEBUG - } - catch (System.OutOfMemoryException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.StackOverflowException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.Threading.ThreadAbortException e) - { - usersConnection.Abort(e); - throw; - } - if (promoteException != null) + //Throw exception only if Transaction is till active and not yet aborted. + if (promoteException != null && Transaction.TransactionInformation.Status != SysTx.TransactionStatus.Aborted) + { + throw SQL.PromotionFailed(promoteException); + } + else + { + // The transaction was aborted externally, since it's already doomed above, we only log the same. + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborted during promote.", ObjectID, connection.ObjectID); + } + } + else { - throw SQL.PromotionFailed(promoteException); + // The transaction was aborted externally, doom the connection to make sure it's eventually rolled back and log the same. + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborted before promote.", ObjectID, connection.ObjectID); } - return returnValue; } @@ -265,19 +278,17 @@ public Byte[] Promote() public void Rollback(SysTx.SinglePhaseEnlistment enlistment) { Debug.Assert(null != enlistment, "null enlistment?"); - SqlInternalConnection connection = null; + SqlInternalConnection connection = GetValidConnection(); - try + if (null != connection) { #if DEBUG TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection(); - - RuntimeHelpers.PrepareConstrainedRegions(); tdsReliabilitySection.Start(); #endif //DEBUG - connection = GetValidConnection(); SqlConnection usersConnection = connection.Connection; - SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborting transaction.", ObjectID, connection.ObjectID); + RuntimeHelpers.PrepareConstrainedRegions(); + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, rolling back transaction.", ObjectID, connection.ObjectID); try { lock (connection) @@ -350,14 +361,11 @@ public void Rollback(SysTx.SinglePhaseEnlistment enlistment) } #endif //DEBUG } - catch (ObjectDisposedException) + else { - if (_atomicTransaction.TransactionInformation.Status == SysTx.TransactionStatus.Active) - { - throw; - } - // Do not throw exception if connection is already aborted/ended from another thread, - // replicate as done in TransactionEnded event handler. + // The transaction was aborted, report that to SysTx and log the same. + enlistment.Aborted(); + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborted before rollback.", ObjectID, connection.ObjectID); } } @@ -365,128 +373,136 @@ public void Rollback(SysTx.SinglePhaseEnlistment enlistment) public void SinglePhaseCommit(SysTx.SinglePhaseEnlistment enlistment) { Debug.Assert(null != enlistment, "null enlistment?"); - SqlInternalConnection connection = GetValidConnection(); - SqlConnection usersConnection = connection.Connection; - SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, committing transaction.", ObjectID, connection.ObjectID); - RuntimeHelpers.PrepareConstrainedRegions(); - try - { -#if DEBUG - TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection(); + if (null != connection) + { + SqlConnection usersConnection = connection.Connection; + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, committing transaction.", ObjectID, connection.ObjectID); RuntimeHelpers.PrepareConstrainedRegions(); + try { - tdsReliabilitySection.Start(); +#if DEBUG + TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection(); + try + { + tdsReliabilitySection.Start(); #else { #endif //DEBUG - // If the connection is dooomed, we can be certain that the - // transaction will eventually be rolled back, and we shouldn't - // attempt to commit it. - if (connection.IsConnectionDoomed) - { - lock (connection) + // If the connection is dooomed, we can be certain that the + // transaction will eventually be rolled back, and we shouldn't + // attempt to commit it. + if (connection.IsConnectionDoomed) { - _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. - _connection = null; - } + lock (connection) + { + _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. + _connection = null; + } - enlistment.Aborted(SQL.ConnectionDoomed()); - } - else - { - Exception commitException; - lock (connection) + enlistment.Aborted(SQL.ConnectionDoomed()); + } + else { - try + Exception commitException; + lock (connection) { - // Now that we've acquired the lock, make sure we still have valid state for this operation. - ValidateActiveOnConnection(connection); + try + { + // Now that we've acquired the lock, make sure we still have valid state for this operation. + ValidateActiveOnConnection(connection); - _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. - _connection = null; // Set prior to ExecuteTransaction call in case this initiates a TransactionEnd event + _active = false; // set to inactive first, doesn't matter how the rest completes, this transaction is done. + _connection = null; // Set prior to ExecuteTransaction call in case this initiates a TransactionEnd event - connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Commit, null, IsolationLevel.Unspecified, _internalTransaction, true); - commitException = null; - } - catch (SqlException e) - { - commitException = e; + connection.ExecuteTransaction(SqlInternalConnection.TransactionRequest.Commit, null, IsolationLevel.Unspecified, _internalTransaction, true); + commitException = null; + } + catch (SqlException e) + { + commitException = e; - ADP.TraceExceptionWithoutRethrow(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 - connection.DoomThisConnection(); + // 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 + connection.DoomThisConnection(); + } + catch (InvalidOperationException e) + { + commitException = e; + ADP.TraceExceptionWithoutRethrow(e); + connection.DoomThisConnection(); + } } - catch (InvalidOperationException e) + if (commitException != null) { - commitException = e; - ADP.TraceExceptionWithoutRethrow(e); - connection.DoomThisConnection(); + // connection.ExecuteTransaction failed with exception + if (_internalTransaction.IsCommitted) + { + // Even though we got an exception, the transaction + // was committed by the server. + enlistment.Committed(); + } + else if (_internalTransaction.IsAborted) + { + // The transaction was aborted, report that to + // SysTx. + enlistment.Aborted(commitException); + } + else + { + // The transaction is still active, we cannot + // know the state of the transaction. + enlistment.InDoubt(commitException); + } + + // We eat the exception. This is called on the SysTx + // thread, not the applications thread. If we don't + // eat the exception an UnhandledException will occur, + // causing the process to FailFast. } - } - if (commitException != null) - { - // connection.ExecuteTransaction failed with exception - if (_internalTransaction.IsCommitted) + + connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); + if (commitException == null) { - // Even though we got an exception, the transaction - // was committed by the server. + // connection.ExecuteTransaction succeeded enlistment.Committed(); } - else if (_internalTransaction.IsAborted) - { - // The transaction was aborted, report that to - // SysTx. - enlistment.Aborted(commitException); - } - else - { - // The transaction is still active, we cannot - // know the state of the transaction. - enlistment.InDoubt(commitException); - } - - // We eat the exception. This is called on the SysTx - // thread, not the applications thread. If we don't - // eat the exception an UnhandledException will occur, - // causing the process to FailFast. - } - - connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction); - if (commitException == null) - { - // connection.ExecuteTransaction succeeded - enlistment.Committed(); } } - } #if DEBUG - finally + finally + { + tdsReliabilitySection.Stop(); + } +#endif //DEBUG + } + catch (System.OutOfMemoryException e) { - tdsReliabilitySection.Stop(); + usersConnection.Abort(e); + throw; + } + catch (System.StackOverflowException e) + { + usersConnection.Abort(e); + throw; + } + catch (System.Threading.ThreadAbortException e) + { + usersConnection.Abort(e); + throw; } -#endif //DEBUG - } - catch (System.OutOfMemoryException e) - { - usersConnection.Abort(e); - throw; - } - catch (System.StackOverflowException e) - { - usersConnection.Abort(e); - throw; } - catch (System.Threading.ThreadAbortException e) + else { - usersConnection.Abort(e); - throw; + // The transaction was aborted before we could commit, report that to SysTx and log the same. + enlistment.Aborted(); + SqlClientEventSource.Log.TraceEvent(" {0}, Connection {1}, aborted before commit.", ObjectID, connection.ObjectID); } } @@ -520,7 +536,7 @@ internal void TransactionEnded(SysTx.Transaction transaction) private SqlInternalConnection GetValidConnection() { SqlInternalConnection connection = _connection; - if (null == connection) + if (null == connection && _atomicTransaction.TransactionInformation.Status != SysTx.TransactionStatus.Aborted) { throw ADP.ObjectDisposed(this); } From 7259dc2628460e720264381c45fe1db972fe2b87 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 8 Apr 2020 15:30:10 -0700 Subject: [PATCH 5/5] Edit --- .../src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs | 1 - 1 file changed, 1 deletion(-) 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 index 05cb15960a..d80a7a685a 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs @@ -36,7 +36,6 @@ internal int ObjectID private SqlInternalConnection _connection; // the internal connection that is the root of the transaction private IsolationLevel _isolationLevel; // the IsolationLevel of the transaction we delegated to the server private SqlInternalTransaction _internalTransaction; // the SQL Server transaction we're delegating to - private SysTx.Transaction _atomicTransaction; private bool _active; // Is the transaction active?