diff --git a/src/EFCore.Relational/Storage/RelationalConnection.cs b/src/EFCore.Relational/Storage/RelationalConnection.cs index 1799f51c98a..0810231774a 100644 --- a/src/EFCore.Relational/Storage/RelationalConnection.cs +++ b/src/EFCore.Relational/Storage/RelationalConnection.cs @@ -39,6 +39,7 @@ public abstract class RelationalConnection : IRelationalConnection, ITransaction private bool _openedInternally; private int? _commandTimeout; private readonly int? _defaultCommandTimeout; + private volatile bool _resetting; private readonly ConcurrentStack _ambientTransactions = new(); private DbConnection? _connection; private readonly IRelationalCommandBuilder _relationalCommandBuilder; @@ -684,16 +685,14 @@ private void ClearTransactions(bool clearAmbient) { CurrentTransaction = null; EnlistedTransaction = null; - if (clearAmbient && _ambientTransactions.Count > 0) + if (clearAmbient) { - while (_ambientTransactions.Any(t => t != null)) + _resetting = true; + while (_ambientTransactions.TryPop(out var ambientTransaction)) { - _ambientTransactions.TryPop(out var ambientTransaction); - if (ambientTransaction != null) - { - ambientTransaction.TransactionCompleted -= HandleTransactionCompleted; - } + ambientTransaction.TransactionCompleted -= HandleTransactionCompleted; } + _resetting = false; } if (_openedCount < 0) @@ -852,18 +851,18 @@ private void HandleAmbientTransactions() private void HandleTransactionCompleted(object? sender, TransactionEventArgs e) { // This could be invoked on a different thread at arbitrary time after the transaction completes - _ambientTransactions.TryPeek(out var ambientTransaction); - if (e.Transaction != ambientTransaction) + if (!_ambientTransactions.TryPop(out var ambientTransaction) + || _resetting) { - throw new InvalidOperationException(RelationalStrings.NestedAmbientTransactionError); + return; } - if (ambientTransaction != null) + if (e.Transaction != ambientTransaction) { - ambientTransaction.TransactionCompleted -= HandleTransactionCompleted; + throw new InvalidOperationException(RelationalStrings.NestedAmbientTransactionError); } - _ambientTransactions.TryPop(out _); + ambientTransaction.TransactionCompleted -= HandleTransactionCompleted; } /// diff --git a/test/EFCore.Relational.Tests/RelationalConnectionTest.cs b/test/EFCore.Relational.Tests/RelationalConnectionTest.cs index 25b8ddd0fde..64162f035f3 100644 --- a/test/EFCore.Relational.Tests/RelationalConnectionTest.cs +++ b/test/EFCore.Relational.Tests/RelationalConnectionTest.cs @@ -2,7 +2,9 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Data; +using System.Transactions; using Microsoft.EntityFrameworkCore.TestUtilities.FakeProvider; +using IsolationLevel = System.Data.IsolationLevel; namespace Microsoft.EntityFrameworkCore; @@ -1082,6 +1084,79 @@ public async Task Reports_command_diagnostic_on_cancellation() }); } + [ConditionalFact] + public void HandleTransactionCompleted_with_concurrent_ClearTransactions_is_thread_safe() + { + // This test verifies the fix for the race condition where HandleTransactionCompleted + // could be called on a different thread while ClearTransactions is executing. + var exceptions = new List(); + for (var i = 0; i < Environment.ProcessorCount; i++) + { + var connection = new FakeRelationalConnection( + CreateOptions(new FakeRelationalOptionsExtension().WithConnectionString("Database=ConcurrencyTest"))); + + using var scope = new TransactionScope(); + connection.Open(); + + var random = new Random(); + var resetFirst = random.Next(0, 1) == 0; + var tasks = new Task[2]; + tasks[0] = Task.Run(async () => + { + try + { + // Small delay to increase chance of race condition + await Task.Yield(); + + if (resetFirst) + { + ((IResettableService)connection).ResetState(); + } + else + { + scope.Complete(); + } + } + catch (Exception ex) + { + lock (exceptions) + { + exceptions.Add(ex); + } + } + }); + + tasks[1] = Task.Run(async () => + { + try + { + // Small delay to increase chance of race condition + await Task.Yield(); + + if (resetFirst) + { + scope.Complete(); + } + else + { + ((IResettableService)connection).ResetState(); + } + } + catch (Exception ex) + { + lock (exceptions) + { + exceptions.Add(ex); + } + } + }); + + Task.WaitAll(tasks, TimeSpan.FromSeconds(10)); + } + + Assert.Empty(exceptions); + } + private static IDbContextOptions CreateOptions(params RelationalOptionsExtension[] optionsExtensions) { var optionsBuilder = new DbContextOptionsBuilder(); diff --git a/test/EFCore.Relational.Tests/TestUtilities/FakeProvider/FakeRelationalConnection.cs b/test/EFCore.Relational.Tests/TestUtilities/FakeProvider/FakeRelationalConnection.cs index e2076a3fe63..870a3840261 100644 --- a/test/EFCore.Relational.Tests/TestUtilities/FakeProvider/FakeRelationalConnection.cs +++ b/test/EFCore.Relational.Tests/TestUtilities/FakeProvider/FakeRelationalConnection.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Transactions; using Microsoft.EntityFrameworkCore.Diagnostics.Internal; using Microsoft.EntityFrameworkCore.Infrastructure.Internal; using Microsoft.EntityFrameworkCore.Storage.Internal; @@ -70,6 +71,11 @@ public List> TransactionDiagnosticEvents public List> ConnectionDiagnosticEvents => ((ListDiagnosticSource)Dependencies.ConnectionLogger.DiagnosticSource).DiagnosticList; + protected override bool SupportsAmbientTransactions => true; + + protected override void ConnectionEnlistTransaction(Transaction transaction) + { } + protected override DbConnection CreateDbConnection() { var connection = new FakeDbConnection(ConnectionString);