From 4187b1c308316a22b38ef533b1409024bc0f7406 Mon Sep 17 00:00:00 2001 From: ajcvickers Date: Sat, 15 Nov 2014 18:55:33 -0800 Subject: [PATCH] Fix for CodePlex 2579: Thread using Migrations can cause other threads to use wrong connection The root of the problem is that using DbMigrator causes change to app domain state through use of a static dictionary in DbContextInfo. This change in state can cause a context instance of a given type to get the wrong connection when a migrator is being used with that context type. In other words, the migrator changes static state so that any context instance of a given type used in the app domain will get the connection intended to be used for migrations. In the repro that state was not being cleaned up; the previous fix for this issue caused it to be cleaned up and so the other context in the repro code started getting the correct connection again. However, the code still transitions through this change in app domain state. So if some thread is using a migrator and some other thread starts a query while the migration is going on, then that second thread will use the wrong connection and return wrong data. I suspect that this is what is happening in the production system with the "long running queries", etc. The fix is to remove the static dictionary and set the context info for the current thread instead. This is similar to the code we have in EF7 for getting the service provider into the constructor without passing it--the issue is essentially the same since the context info is needed in the context constructor and yet is not passed into the constructor. I believe that thread static is fine here because there are no opportunities for async calls between setting the thread static and reading of the value. See https://entityframework.codeplex.com/workitem/1552 for the original issue. --- .../Infrastructure/DbContextInfo.cs | 35 ++---- .../Internal/LazyInternalContext.cs | 2 +- .../Migrations/NoTestInfraScenarios.cs | 102 ++++++++++++++++++ .../Infrastructure/DbContextInfoTests.cs | 21 ++-- .../Internal/DatabaseCreatorTests.cs | 4 +- 5 files changed, 120 insertions(+), 44 deletions(-) diff --git a/src/EntityFramework/Infrastructure/DbContextInfo.cs b/src/EntityFramework/Infrastructure/DbContextInfo.cs index c05cbed5c3..8a49aac890 100644 --- a/src/EntityFramework/Infrastructure/DbContextInfo.cs +++ b/src/EntityFramework/Infrastructure/DbContextInfo.cs @@ -2,7 +2,6 @@ namespace System.Data.Entity.Infrastructure { - using System.Collections.Concurrent; using System.Configuration; using System.Data.Common; using System.Data.Entity.Infrastructure.DependencyResolution; @@ -23,8 +22,8 @@ namespace System.Data.Entity.Infrastructure /// public class DbContextInfo { - private static readonly ConcurrentDictionary _infoMapping - = new ConcurrentDictionary(); + [ThreadStatic] + private static DbContextInfo _currentInfo; private readonly Type _contextType; private readonly DbProviderInfo _modelProviderInfo; @@ -298,7 +297,7 @@ public virtual Action OnModelCreating public virtual DbContext CreateInstance() { var configPushed = DbConfigurationManager.Instance.PushConfiguration(_appConfig, _contextType); - MapContextToInfo(_contextType, this); + CurrentInfo = this; DbContext context = null; try @@ -321,7 +320,7 @@ public virtual DbContext CreateInstance() return null; } - context.InternalContext.OnDisposing += (_, __) => ClearInfoForContext(_contextType); + context.InternalContext.OnDisposing += (_, __) => CurrentInfo = null; if (configPushed) { @@ -346,7 +345,7 @@ public virtual DbContext CreateInstance() { if (context == null) { - ClearInfoForContext(_contextType); + CurrentInfo = null; if (configPushed) { @@ -422,28 +421,10 @@ where t.IsClass() && typeof(IDbContextFactory<>).MakeGenericType(_contextType).I return ((IDbContextFactory)Activator.CreateInstance(factoryType)).Create; } - internal static void MapContextToInfo(Type contextType, DbContextInfo info) + internal static DbContextInfo CurrentInfo { - DebugCheck.NotNull(contextType); - DebugCheck.NotNull(info); - - _infoMapping.AddOrUpdate(contextType, info, (t, i) => info); - } - - internal static void ClearInfoForContext(Type contextType) - { - DebugCheck.NotNull(contextType); - - DbContextInfo _; - _infoMapping.TryRemove(contextType, out _); - } - - internal static DbContextInfo TryGetInfoForContext(Type contextType) - { - DebugCheck.NotNull(contextType); - - DbContextInfo info; - return _infoMapping.TryGetValue(contextType, out info) ? info : null; + get { return _currentInfo; } + set { _currentInfo = value; } } } } diff --git a/src/EntityFramework/Internal/LazyInternalContext.cs b/src/EntityFramework/Internal/LazyInternalContext.cs index 0a741b8ae7..87ddff69b3 100644 --- a/src/EntityFramework/Internal/LazyInternalContext.cs +++ b/src/EntityFramework/Internal/LazyInternalContext.cs @@ -398,7 +398,7 @@ protected override void InitializeContext() } try { - var contextInfo = DbContextInfo.TryGetInfoForContext(Owner.GetType()); + var contextInfo = DbContextInfo.CurrentInfo; if (contextInfo != null) { ApplyContextInfo(contextInfo); diff --git a/test/EntityFramework/FunctionalTests/Migrations/NoTestInfraScenarios.cs b/test/EntityFramework/FunctionalTests/Migrations/NoTestInfraScenarios.cs index 369b6d6205..5e0a9d3d20 100644 --- a/test/EntityFramework/FunctionalTests/Migrations/NoTestInfraScenarios.cs +++ b/test/EntityFramework/FunctionalTests/Migrations/NoTestInfraScenarios.cs @@ -2,7 +2,11 @@ namespace System.Data.Entity.Migrations { + using System.Data.Entity.Infrastructure; using System.Data.Entity.Migrations.Design; + using System.Data.SqlClient; + using System.Linq; + using System.Threading.Tasks; using Xunit; public class NoTestInfraScenarios : TestBase @@ -28,5 +32,103 @@ var migrator Assert.False(string.IsNullOrWhiteSpace(migration.UserCode)); Assert.False(string.IsNullOrWhiteSpace(migration.Directory)); } + + [Fact] // CodePlex #2579 + public void Repro() + { + var connection1 = SimpleConnectionString("Db2579Aa"); + var connection2 = SimpleConnectionString("Db2579Bb"); + + using (var context = new Context2579I(connection1)) + { + context.Database.Delete(); + context.Database.Create(); + context.Entities.Add(new Entity2579()); + context.SaveChanges(); + } + + using (var context = new Context2579I(connection2)) + { + context.Database.Delete(); + } + + var tests = new Action[100]; + for (var i = 0; i < 100; i++) + { + tests[i] = () => + { + using (var context = new Context2579(connection1)) + { + Assert.Equal(1, context.Entities.Count()); + } + }; + } + + tests[25] = () => + { + using (new Context2579(connection2)) + { + var migrationConfiguration = new Configuration2579(connection2); + var migrator = new DbMigrator(migrationConfiguration); + + migrator.Update(); + } + }; + + Parallel.Invoke(tests); + } + + public class Context2579 : DbContext + { + static Context2579() + { + Database.SetInitializer(null); + } + + public Context2579() + { + } + + public Context2579(string nameOrConnectionString) + : base(nameOrConnectionString) + { + } + + public DbSet Entities { get; set; } + } + + public class Context2579I : DbContext + { + static Context2579I() + { + Database.SetInitializer(null); + } + + public Context2579I(string nameOrConnectionString) + : base(nameOrConnectionString) + { + } + + public DbSet Entities { get; set; } + } + + public class Entity2579 + { + public int Id { get; set; } + } + + internal sealed class Configuration2579 : DbMigrationsConfiguration + { + public Configuration2579() + { + AutomaticMigrationsEnabled = true; + } + + public Configuration2579(string connectionString) + { + AutomaticMigrationsEnabled = true; + TargetDatabase = new DbConnectionInfo(connectionString, "System.Data.SqlClient"); + } + } } } diff --git a/test/EntityFramework/UnitTests/Infrastructure/DbContextInfoTests.cs b/test/EntityFramework/UnitTests/Infrastructure/DbContextInfoTests.cs index 376b627b9e..c4fdfd9302 100644 --- a/test/EntityFramework/UnitTests/Infrastructure/DbContextInfoTests.cs +++ b/test/EntityFramework/UnitTests/Infrastructure/DbContextInfoTests.cs @@ -128,7 +128,7 @@ public void CreateInstance_should_return_null_when_context_not_constructible_and Assert.False(contextInfo.IsConstructible); Assert.Null(contextInfo.CreateInstance()); - Assert.Null(DbContextInfo.TryGetInfoForContext(typeof(DbContext))); + Assert.Null(DbContextInfo.CurrentInfo); } [Fact] @@ -1197,7 +1197,7 @@ private void Exceptions_applying_new_connection_surfaced_and_context_type_is_unm Assert.Throws( () => new DbContextInfo(contextType, CreateEmptyConfig(), connection)).Message); - Assert.Null(DbContextInfo.TryGetInfoForContext(contextType)); + Assert.Null(DbContextInfo.CurrentInfo); } [DbConfigurationType(typeof(FunctionalTestsConfiguration))] @@ -1330,28 +1330,21 @@ public void InitializeDatabase(InitTestContext context) [Fact] public void Context_type_can_be_assoictaed_with_DbContextInfo_and_then_later_unsuppressed() { - Assert.Null(DbContextInfo.TryGetInfoForContext(typeof(ContextToAssociate))); - Assert.Null(DbContextInfo.TryGetInfoForContext(typeof(ContextToNotAssociate))); + Assert.Null(DbContextInfo.CurrentInfo); var contextInfo = new DbContextInfo(typeof(ContextToAssociate)); - DbContextInfo.MapContextToInfo(typeof(ContextToAssociate), contextInfo); + DbContextInfo.CurrentInfo = contextInfo; - Assert.Same(contextInfo, DbContextInfo.TryGetInfoForContext(typeof(ContextToAssociate))); - Assert.Null(DbContextInfo.TryGetInfoForContext(typeof(ContextToNotAssociate))); + Assert.Same(contextInfo, DbContextInfo.CurrentInfo); - DbContextInfo.ClearInfoForContext(typeof(ContextToAssociate)); + DbContextInfo.CurrentInfo = null; - Assert.Null(DbContextInfo.TryGetInfoForContext(typeof(ContextToAssociate))); - Assert.Null(DbContextInfo.TryGetInfoForContext(typeof(ContextToNotAssociate))); + Assert.Null(DbContextInfo.CurrentInfo); } public class ContextToAssociate : DbContext { } - - public class ContextToNotAssociate : DbContext - { - } } public class FakeDbContextInfoConnectionFactory : IDbConnectionFactory diff --git a/test/EntityFramework/UnitTests/Internal/DatabaseCreatorTests.cs b/test/EntityFramework/UnitTests/Internal/DatabaseCreatorTests.cs index 94d24c21fd..e55b94c38e 100644 --- a/test/EntityFramework/UnitTests/Internal/DatabaseCreatorTests.cs +++ b/test/EntityFramework/UnitTests/Internal/DatabaseCreatorTests.cs @@ -155,14 +155,14 @@ public void CreateDatabase_using_Migrations_does_not_suppress_initializers() { var internalContext = CreateMockContextForMigrator().Object; - DbContextInfo.ClearInfoForContext(internalContext.Owner.GetType()); + DbContextInfo.CurrentInfo = null; new DatabaseCreator().CreateDatabase( internalContext, (config, context) => new Mock(config, context, DatabaseExistenceState.Unknown, true).Object, null); - Assert.Null(DbContextInfo.TryGetInfoForContext(internalContext.Owner.GetType())); + Assert.Null(DbContextInfo.CurrentInfo); } [Fact]