Solution with IPromotableSinglePhaseNotification and TransactionCompletion against a promoted transaction to MS-DTC

[neilzzy]

Description

When performing multiple queries in parallel, and each pair of 2 queries are inside a transaction scope, an unexpected transaction error is thrown (see below).
dotnet/SqlClient#1675
The product team of sqlclient need suggestions to work around this issue.
#96067
Based on the transaction documents, subscribing to the TransactionCompleted event has negative affection on perf. I'd appreciate any thoughts with the following questions:

Is there any opportunity to improve it or this is the best version of the event driven design?
What would be the replacement to receive the transaction completion signal with a promoted transaction to DTC through a IPromotableSinglePhaseNotification implementation?
Here is the links to the design on MDS:

SqlDelegatedTransaction
TransactionCompletedEvent

Reproduction Steps

Issue can be reproduced intermittently with below code

async Task Main()
{
	var tasks = Enumerable.Range(0, 100_000).Select(PerformWork);
	await Task.WhenAll(tasks);
	"DONE!".Dump();
}
const string connString = "Server=.\\SQL2019;Database=TempDB;Application Name=TestApp;Trusted_Connection=True;TrustServerCertificate=True;Pooling=True;Encrypt=True;Max Pool Size = 5000;";

static TransactionScope CreateTransactionScope() =>
	new TransactionScope(TransactionScopeOption.Required,
		new TransactionOptions { IsolationLevel = System.Transactions.IsolationLevel.ReadCommitted }, TransactionScopeAsyncFlowOption.Enabled);

static async Task PerformWork(int num)
{
	try
	{
		using (var ts = CreateTransactionScope())
		{
			for (int i = 0; i < 2; ++i)
			{
				await using (var dbConn = new SqlConnection(connString))
				{
					await dbConn.OpenAsync();
					await Task.Delay(1);
				} // Connection is disposed (and thus closed)
			}//for
			//ts.Complete();
		}//ts
	}//try
	catch (Exception e)
	{
		Console.WriteLine($"Failed {num}");
		e.Dump(); Environment.Exit(-1);
		throw;
	}
}

Expected behavior

no exception at all

Actual behavior

intermittently failed with Unhandled exception. System.Transactions.TransactionAbortedException: The transaction has aborted.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[ghost]

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

When performing multiple queries in parallel, and each pair of 2 queries are inside a transaction scope, an unexpected transaction error is thrown (see below).
dotnet/SqlClient#1675
The product team of sqlclient need suggestions to work around this issue.
#96067
Based on the transaction documents, subscribing to the TransactionCompleted event has negative affection on perf. I'd appreciate any thoughts with the following questions:

Is there any opportunity to improve it or this is the best version of the event driven design?
What would be the replacement to receive the transaction completion signal with a promoted transaction to DTC through a IPromotableSinglePhaseNotification implementation?
Here is the links to the design on MDS:

SqlDelegatedTransaction
TransactionCompletedEvent

Reproduction Steps

Issue can be reproduced intermittently with below code

async Task Main()
{
var tasks = Enumerable.Range(0, 100_000).Select(PerformWork);
await Task.WhenAll(tasks);
"DONE!".Dump();
}
const string connString = "Server=.\SQL2019;Database=TempDB;Application Name=TestApp;Trusted_Connection=True;TrustServerCertificate=True;Pooling=True;Encrypt=True;Max Pool Size = 5000;";

static TransactionScope CreateTransactionScope() =>
new TransactionScope(TransactionScopeOption.Required,
new TransactionOptions { IsolationLevel = System.Transactions.IsolationLevel.ReadCommitted }, TransactionScopeAsyncFlowOption.Enabled);

static async Task PerformWork(int num)
{
try
{
using (var ts = CreateTransactionScope())
{
for (int i = 0; i < 2; ++i)
{
await using (var dbConn = new SqlConnection(connString))
{
await dbConn.OpenAsync();
await Task.Delay(1);
} // Connection is disposed (and thus closed)
}//for
//ts.Complete();
}//ts
}//try
catch (Exception e)
{
Console.WriteLine($"Failed {num}");
e.Dump(); Environment.Exit(-1);
throw;
}
}

Expected behavior

no exception at all

Actual behavior

intermitent failed with Unhandled exception. System.Transactions.TransactionAbortedException: The transaction has aborted.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	neilzzy
Assignees:	-
Labels:	area-System.Transactions, untriaged
Milestone:	-

[roji]

@neilzzy thanks for opening this, I didn't know that #96067 was opened on this.

I remember looking into this a while back as part of investigating deadlocks and bugs with SqlClient's distributed transaction support (dotnet/SqlClient#1675, still unresolved). I'm no expert in SqlClient internals, but I found myself wondering whether SqlClient could simply stop subscribing to the TransactionCompleted event altogether - please see this comment and the discussion around it. In other words, regardless of the perf impact of subscribing to the event, SqlClient's usage of it seems problematic and may be the cause of actual bugs.

More generally, from what I can tell, the TransactionCompleted event was meant more for application consumption - to know when the transaction was completed - and not really for resource managers (database drivers) to use it in order to implement their distributed transaction support. I'd highly recommend a deep analysis on the SqlClient side for how that event is used and why, and whether it's possible to simplify things and stop doing that.

Beyond that, I can see the comment about TransactionCompleted having negative perf implications in the docs; I don't know what that refers to specifically, but I suspect that beyond SqlClient's internal use of the event, that wouldn't be an important issue affecting many people. We can certainly keep this open in the backlog to track looking more closely at it, though I doubt that would happen any time soon.

/cc @DavoudEshtehari

[SamMonoRT]

will need to pick up as part of planning for next release.