From 7ee7529730bfacf611d9f161a135aa80cec9c9a5 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Wed, 16 Dec 2020 09:45:05 +0200 Subject: [PATCH 1/2] Allow opting out of automatic savepoints in SaveChanges Fixes #23526 --- .../Update/Internal/BatchExecutor.cs | 6 +- src/EFCore/Infrastructure/DatabaseFacade.cs | 17 ++++ .../TransactionTestBase.cs | 93 +++++++++++++++++++ .../TransactionSqlServerTest.cs | 18 ++-- 4 files changed, 126 insertions(+), 8 deletions(-) diff --git a/src/EFCore.Relational/Update/Internal/BatchExecutor.cs b/src/EFCore.Relational/Update/Internal/BatchExecutor.cs index c17fffc1450..4379148de70 100644 --- a/src/EFCore.Relational/Update/Internal/BatchExecutor.cs +++ b/src/EFCore.Relational/Update/Internal/BatchExecutor.cs @@ -88,7 +88,8 @@ public virtual int Execute( { connection.Open(); - if (transaction?.SupportsSavepoints == true) + if (transaction?.SupportsSavepoints == true + && CurrentContext.Context.Database.AutoSavepointsEnabled) { transaction.CreateSavepoint(SavepointName); createdSavepoint = true; @@ -181,7 +182,8 @@ public virtual async Task ExecuteAsync( { await connection.OpenAsync(cancellationToken).ConfigureAwait(false); - if (transaction?.SupportsSavepoints == true) + if (transaction?.SupportsSavepoints == true + && CurrentContext.Context.Database.AutoSavepointsEnabled) { await transaction.CreateSavepointAsync(SavepointName, cancellationToken).ConfigureAwait(false); createdSavepoint = true; diff --git a/src/EFCore/Infrastructure/DatabaseFacade.cs b/src/EFCore/Infrastructure/DatabaseFacade.cs index fce69f07e5b..575416ec6c2 100644 --- a/src/EFCore/Infrastructure/DatabaseFacade.cs +++ b/src/EFCore/Infrastructure/DatabaseFacade.cs @@ -303,6 +303,23 @@ public virtual IDbContextTransaction CurrentTransaction /// public virtual bool AutoTransactionsEnabled { get; set; } = true; + /// + /// + /// Whether a transaction savepoint will be created automatically by if it is called + /// after a transaction has been manually started with . + /// + /// + /// The default value is , meaning that will create a + /// transaction savepoint within a manually-started transaction. Regardless of this property, savepoints are only created + /// if the data provider supports them; see . + /// + /// + /// Setting this value to should only be done with caution since the database could be left in a + /// corrupted state if fails. + /// + /// + public virtual bool AutoSavepointsEnabled { get; set; } = true; + /// /// /// Returns the name of the database provider currently in use. diff --git a/test/EFCore.Relational.Specification.Tests/TransactionTestBase.cs b/test/EFCore.Relational.Specification.Tests/TransactionTestBase.cs index 8a1bd8c7d7e..a0ea2380399 100644 --- a/test/EFCore.Relational.Specification.Tests/TransactionTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/TransactionTestBase.cs @@ -16,6 +16,7 @@ using Xunit; using IsolationLevel = System.Data.IsolationLevel; +// ReSharper disable MethodHasAsyncOverload // ReSharper disable InconsistentNaming namespace Microsoft.EntityFrameworkCore { @@ -1209,6 +1210,98 @@ public virtual async Task Externally_closed_connections_are_handled_correctly(bo Assert.Equal(ConnectionState.Closed, connection.State); } + [ConditionalTheory] + [InlineData(true)] + [InlineData(false)] + public virtual async Task SaveChanges_implicitly_creates_savepoint(bool async) + { + using (var context = CreateContext()) + { + Assert.True(context.Database.AutoSavepointsEnabled); + + using var transaction = async + ? await context.Database.BeginTransactionAsync() + : context.Database.BeginTransaction(); + + context.Add(new TransactionCustomer { Id = 77, Name = "Bobble" }); + + if (async) + { + await context.SaveChangesAsync(); + } + else + { + context.SaveChanges(); + } + + context.Add(new TransactionCustomer { Id = 78, Name = "Hobble" }); + context.Add(new TransactionCustomer { Id = 1, Name = "Gobble" }); // Cause SaveChanges failure + + if (async) + { + await Assert.ThrowsAsync(() => context.SaveChangesAsync()); + await transaction.CommitAsync(); + } + else + { + Assert.Throws(() => context.SaveChanges()); + transaction.Commit(); + } + } + + using (var context = CreateContext()) + { + Assert.Equal(77, context.Set().Max(c => c.Id)); + } + } + + [ConditionalTheory] + [InlineData(true)] + [InlineData(false)] + public virtual async Task SaveChanges_can_be_used_with_no_savepoint(bool async) + { + using (var context = CreateContext()) + { + context.Database.AutoSavepointsEnabled = false; + + using var transaction = async + ? await context.Database.BeginTransactionAsync() + : context.Database.BeginTransaction(); + + context.Add(new TransactionCustomer { Id = 77, Name = "Bobble" }); + + if (async) + { + await context.SaveChangesAsync(); + } + else + { + context.SaveChanges(); + } + + context.Add(new TransactionCustomer { Id = 78, Name = "Hobble" }); + context.Add(new TransactionCustomer { Id = 1, Name = "Gobble" }); // Cause SaveChanges failure + + if (async) + { + await Assert.ThrowsAsync(() => context.SaveChangesAsync()); + await transaction.CommitAsync(); + } + else + { + Assert.Throws(() => context.SaveChanges()); + transaction.Commit(); + } + + context.Database.AutoSavepointsEnabled = true; + } + + using (var context = CreateContext()) + { + Assert.Equal(78, context.Set().Max(c => c.Id)); + } + } + [ConditionalTheory] [InlineData(true)] [InlineData(false)] diff --git a/test/EFCore.SqlServer.FunctionalTests/TransactionSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/TransactionSqlServerTest.cs index 932e1f17a95..a240171f878 100644 --- a/test/EFCore.SqlServer.FunctionalTests/TransactionSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/TransactionSqlServerTest.cs @@ -20,10 +20,22 @@ public TransactionSqlServerTest(TransactionSqlServerFixture fixture) { } + // Test relies on savepoints, which are disabled when MARS is enabled + public override Task SaveChanges_implicitly_creates_savepoint(bool async) + => new SqlConnectionStringBuilder(TestStore.ConnectionString).MultipleActiveResultSets + ? Task.CompletedTask + : base.SaveChanges_implicitly_creates_savepoint(async); + // Savepoints cannot be released in SQL Server public override Task Savepoint_can_be_released(bool async) => Task.CompletedTask; + // Test relies on savepoints, which are disabled when MARS is enabled + public override Task SaveChanges_uses_explicit_transaction_with_failure_behavior(bool async, bool autoTransaction) + => new SqlConnectionStringBuilder(TestStore.ConnectionString).MultipleActiveResultSets + ? Task.CompletedTask + : base.SaveChanges_uses_explicit_transaction_with_failure_behavior(async, autoTransaction); + [ConditionalTheory] [InlineData(true)] [InlineData(false)] @@ -53,12 +65,6 @@ public virtual async Task Savepoints_are_disabled_with_MARS(bool async) Assert.Contains(Fixture.ListLoggerFactory.Log, t => t.Id == SqlServerEventId.SavepointsDisabledBecauseOfMARS); } - // Test relies on savepoints, which are disabled when MARS is enabled - public override Task SaveChanges_uses_explicit_transaction_with_failure_behavior(bool async, bool autoTransaction) - => new SqlConnectionStringBuilder(TestStore.ConnectionString).MultipleActiveResultSets - ? Task.CompletedTask - : base.SaveChanges_uses_explicit_transaction_with_failure_behavior(async, autoTransaction); - protected override bool SnapshotSupported => true; From 628c512e6cfea5a12b03bb03744c37ec56400810 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Thu, 17 Dec 2020 19:39:38 +0200 Subject: [PATCH 2/2] Handle AutoSavepointsEnabled for DbContext pooling --- src/EFCore/DbContext.cs | 5 +++++ .../Internal/DbContextPoolConfigurationSnapshot.cs | 10 ++++++++++ .../DbContextPoolingTest.cs | 11 +++++++++++ 3 files changed, 26 insertions(+) diff --git a/src/EFCore/DbContext.cs b/src/EFCore/DbContext.cs index aba4e94c7d7..0bd47b8a40b 100644 --- a/src/EFCore/DbContext.cs +++ b/src/EFCore/DbContext.cs @@ -718,6 +718,10 @@ void IDbContextPoolable.SetLease(DbContextLease lease) _database.AutoTransactionsEnabled = _configurationSnapshot?.AutoTransactionsEnabled == null || _configurationSnapshot.AutoTransactionsEnabled.Value; + + _database.AutoSavepointsEnabled + = _configurationSnapshot?.AutoSavepointsEnabled == null + || _configurationSnapshot.AutoSavepointsEnabled.Value; } } @@ -733,6 +737,7 @@ void IDbContextPoolable.SnapshotConfiguration() _changeTracker?.AutoDetectChangesEnabled, _changeTracker?.QueryTrackingBehavior, _database?.AutoTransactionsEnabled, + _database?.AutoSavepointsEnabled, _changeTracker?.LazyLoadingEnabled, _changeTracker?.CascadeDeleteTiming, _changeTracker?.DeleteOrphansTiming); diff --git a/src/EFCore/Internal/DbContextPoolConfigurationSnapshot.cs b/src/EFCore/Internal/DbContextPoolConfigurationSnapshot.cs index b2d518639bb..32e7a3e22db 100644 --- a/src/EFCore/Internal/DbContextPoolConfigurationSnapshot.cs +++ b/src/EFCore/Internal/DbContextPoolConfigurationSnapshot.cs @@ -23,6 +23,7 @@ public DbContextPoolConfigurationSnapshot( bool? autoDetectChangesEnabled, QueryTrackingBehavior? queryTrackingBehavior, bool? autoTransactionsEnabled, + bool? autoSavepointsEnabled, bool? lazyLoadingEnabled, CascadeTiming? cascadeDeleteTiming, CascadeTiming? deleteOrphansTiming) @@ -30,6 +31,7 @@ public DbContextPoolConfigurationSnapshot( AutoDetectChangesEnabled = autoDetectChangesEnabled; QueryTrackingBehavior = queryTrackingBehavior; AutoTransactionsEnabled = autoTransactionsEnabled; + AutoSavepointsEnabled = autoSavepointsEnabled; LazyLoadingEnabled = lazyLoadingEnabled; CascadeDeleteTiming = cascadeDeleteTiming; DeleteOrphansTiming = deleteOrphansTiming; @@ -82,5 +84,13 @@ public DbContextPoolConfigurationSnapshot( /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public virtual bool? AutoTransactionsEnabled { get; } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public virtual bool? AutoSavepointsEnabled { get; } } } diff --git a/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs b/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs index e58ea0ab9b8..b0911474b27 100644 --- a/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs @@ -98,6 +98,7 @@ public PooledContext(DbContextOptions options) ChangeTracker.AutoDetectChangesEnabled = false; ChangeTracker.LazyLoadingEnabled = false; Database.AutoTransactionsEnabled = false; + Database.AutoSavepointsEnabled = false; ChangeTracker.CascadeDeleteTiming = CascadeTiming.Never; ChangeTracker.DeleteOrphansTiming = CascadeTiming.Never; } @@ -539,6 +540,7 @@ public async Task Context_configuration_is_reset(bool useInterface, bool async) context1.ChangeTracker.CascadeDeleteTiming = CascadeTiming.Immediate; context1.ChangeTracker.DeleteOrphansTiming = CascadeTiming.Immediate; context1.Database.AutoTransactionsEnabled = true; + context1.Database.AutoSavepointsEnabled = true; context1.SavingChanges += (sender, args) => { }; context1.SavedChanges += (sender, args) => { }; context1.SaveChangesFailed += (sender, args) => { }; @@ -568,6 +570,7 @@ public async Task Context_configuration_is_reset(bool useInterface, bool async) Assert.Equal(CascadeTiming.Never, context2.ChangeTracker.CascadeDeleteTiming); Assert.Equal(CascadeTiming.Never, context2.ChangeTracker.DeleteOrphansTiming); Assert.False(context2.Database.AutoTransactionsEnabled); + Assert.False(context2.Database.AutoSavepointsEnabled); } [ConditionalTheory] @@ -585,6 +588,7 @@ public async Task Context_configuration_is_reset_with_factory(bool async) context1.ChangeTracker.CascadeDeleteTiming = CascadeTiming.Immediate; context1.ChangeTracker.DeleteOrphansTiming = CascadeTiming.Immediate; context1.Database.AutoTransactionsEnabled = true; + context1.Database.AutoSavepointsEnabled = true; context1.SavingChanges += (sender, args) => { }; context1.SavedChanges += (sender, args) => { }; context1.SaveChangesFailed += (sender, args) => { }; @@ -609,6 +613,7 @@ public async Task Context_configuration_is_reset_with_factory(bool async) Assert.Equal(CascadeTiming.Never, context2.ChangeTracker.CascadeDeleteTiming); Assert.Equal(CascadeTiming.Never, context2.ChangeTracker.DeleteOrphansTiming); Assert.False(context2.Database.AutoTransactionsEnabled); + Assert.False(context2.Database.AutoSavepointsEnabled); } [ConditionalFact] @@ -628,6 +633,7 @@ public void Change_tracker_can_be_cleared_without_resetting_context_config() context.ChangeTracker.CascadeDeleteTiming = CascadeTiming.Immediate; context.ChangeTracker.DeleteOrphansTiming = CascadeTiming.Immediate; context.Database.AutoTransactionsEnabled = true; + context.Database.AutoSavepointsEnabled = true; context.ChangeTracker.Tracked += ChangeTracker_OnTracked; context.ChangeTracker.StateChanged += ChangeTracker_OnStateChanged; context.SavingChanges += (sender, args) => { }; @@ -642,6 +648,7 @@ public void Change_tracker_can_be_cleared_without_resetting_context_config() Assert.Equal(CascadeTiming.Immediate, context.ChangeTracker.CascadeDeleteTiming); Assert.Equal(CascadeTiming.Immediate, context.ChangeTracker.DeleteOrphansTiming); Assert.True(context.Database.AutoTransactionsEnabled); + Assert.True(context.Database.AutoSavepointsEnabled); Assert.False(_changeTracker_OnTracked); Assert.False(_changeTracker_OnStateChanged); @@ -687,6 +694,7 @@ public async Task Default_Context_configuration_is_reset(bool async) context1.ChangeTracker.LazyLoadingEnabled = false; context1.ChangeTracker.QueryTrackingBehavior = QueryTrackingBehavior.NoTracking; context1.Database.AutoTransactionsEnabled = false; + context1.Database.AutoSavepointsEnabled = false; context1.ChangeTracker.CascadeDeleteTiming = CascadeTiming.Immediate; context1.ChangeTracker.DeleteOrphansTiming = CascadeTiming.Immediate; @@ -705,6 +713,7 @@ public async Task Default_Context_configuration_is_reset(bool async) Assert.Equal(CascadeTiming.Immediate, context2.ChangeTracker.CascadeDeleteTiming); Assert.Equal(CascadeTiming.Immediate, context2.ChangeTracker.DeleteOrphansTiming); Assert.True(context2.Database.AutoTransactionsEnabled); + Assert.True(context2.Database.AutoSavepointsEnabled); } [ConditionalTheory] @@ -721,6 +730,7 @@ public async Task Default_Context_configuration_is_reset_with_factory(bool async context1.ChangeTracker.LazyLoadingEnabled = false; context1.ChangeTracker.QueryTrackingBehavior = QueryTrackingBehavior.NoTracking; context1.Database.AutoTransactionsEnabled = false; + context1.Database.AutoSavepointsEnabled = false; context1.ChangeTracker.CascadeDeleteTiming = CascadeTiming.Immediate; context1.ChangeTracker.DeleteOrphansTiming = CascadeTiming.Immediate; @@ -736,6 +746,7 @@ public async Task Default_Context_configuration_is_reset_with_factory(bool async Assert.Equal(CascadeTiming.Immediate, context2.ChangeTracker.CascadeDeleteTiming); Assert.Equal(CascadeTiming.Immediate, context2.ChangeTracker.DeleteOrphansTiming); Assert.True(context2.Database.AutoTransactionsEnabled); + Assert.True(context2.Database.AutoSavepointsEnabled); } [ConditionalTheory]