Skip to content

Commit

Permalink
[Release 3.0] Fix deadlock in transaction (.NET Framework) (#1243)
Browse files Browse the repository at this point in the history
  • Loading branch information
@cheenamalhotra
cheenamalhotra authored Sep 17, 2021
1 parent 4f744a8 commit b689674
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 39 deletions.
80 changes: 41 additions & 39 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDelegatedTransaction.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,21 +391,23 @@ public void SinglePhaseCommit(SysTx.SinglePhaseEnlistment enlistment)
#else
{
#endif //DEBUG
lock (connection)
// If the connection is doomed, we can be certain that the
// transaction will eventually be rolled back, and we shouldn't
// attempt to commit it.
if (connection.IsConnectionDoomed)
{
// If the connection is doomed, we can be certain that the
// transaction will eventually be rolled back or has already been aborted externally, 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
enlistment.Aborted(SQL.ConnectionDoomed());
}
else
{
Exception commitException;
lock (connection)
{
Exception commitException;
try
{
// Now that we've acquired the lock, make sure we still have valid state for this operation.
Expand Down Expand Up @@ -434,40 +436,40 @@ public void SinglePhaseCommit(SysTx.SinglePhaseEnlistment enlistment)
ADP.TraceExceptionWithoutRethrow(e);
connection.DoomThisConnection();
}
if (commitException != null)
}
if (commitException != null)
{
// connection.ExecuteTransaction failed with exception
if (_internalTransaction.IsCommitted)
{
// 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.
// Even though we got an exception, the transaction
// was committed by the server.
enlistment.Committed();
}

connection.CleanupConnectionOnTransactionCompletion(_atomicTransaction);
if (commitException == null)
else if (_internalTransaction.IsAborted)
{
// connection.ExecuteTransaction succeeded
enlistment.Committed();
// 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();
}
}
}
Expand Down
35 changes: 35 additions & 0 deletions ...crosoft.Data.SqlClient/tests/ManualTests/SQL/TransactionTest/TransactionEnlistmentTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Data;
using System.Threading.Tasks;
using System.Transactions;
using Xunit;

Expand Down Expand Up @@ -46,6 +47,30 @@ public static void TestManualEnlistment_Enlist_TxScopeComplete()
RunTestSet(TestCase_ManualEnlistment_Enlist_TxScopeComplete);
}

[SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp)]
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer))]
public static void TestEnlistmentPrepare_TxScopeComplete()
{
try
{
using TransactionScope txScope = new(TransactionScopeOption.RequiresNew, new TransactionOptions()
{
IsolationLevel = System.Transactions.IsolationLevel.ReadCommitted,
Timeout = TransactionManager.DefaultTimeout
}, TransactionScopeAsyncFlowOption.Enabled);

using SqlConnection connection = new(DataTestUtility.TCPConnectionString);
connection.Open();
System.Transactions.Transaction.Current.EnlistDurable(EnlistmentForPrepare.s_id, new EnlistmentForPrepare(), EnlistmentOptions.None);
txScope.Complete();
Assert.False(true, "Expected exception not thrown.");
}
catch (Exception e)
{
Assert.True(e is TransactionAbortedException);
}
}

private static void TestCase_AutoEnlistment_TxScopeComplete()
{
SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(ConnectionString);
Expand Down Expand Up @@ -168,6 +193,16 @@ private static void TestCase_ManualEnlistment_Enlist_TxScopeComplete()
Assert.True(string.Equals(result.Rows[0][0], InputCol2));
}

class EnlistmentForPrepare : IEnlistmentNotification
{
public static readonly Guid s_id = Guid.NewGuid();
// fail during prepare, this will cause scope.Complete to throw
public void Prepare(PreparingEnlistment preparingEnlistment) => preparingEnlistment.ForceRollback();
public void Commit(Enlistment enlistment) => enlistment.Done();
public void Rollback(Enlistment enlistment) => enlistment.Done();
public void InDoubt(Enlistment enlistment) => enlistment.Done();
}

private static string TestTableName;
private static string ConnectionString;
private const int InputCol1 = 1;
Expand Down

0 comments on commit b689674

Please sign in to comment.