Fix for CodePlex 2579: Thread using Migrations can cause other thread…

…s 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.

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
     /// </summary>
     public class DbContextInfo
     {
-        private static readonly ConcurrentDictionary<Type, DbContextInfo> _infoMapping
-            = new ConcurrentDictionary<Type, DbContextInfo>();
+        [ThreadStatic]
+        private static DbContextInfo _currentInfo;
 
         private readonly Type _contextType;
         private readonly DbProviderInfo _modelProviderInfo;
@@ -298,7 +297,7 @@ public virtual Action<DbModelBuilder> 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<DbContext>)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; }
         }
     }
 }

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);

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<Context2579>(null);
+            }
+
+            public Context2579()
+            {
+            }
+
+            public Context2579(string nameOrConnectionString)
+                : base(nameOrConnectionString)
+            {
+            }
+
+            public DbSet<Entity2579> Entities { get; set; }
+        }
+
+        public class Context2579I : DbContext
+        {
+            static Context2579I()
+            {
+                Database.SetInitializer<Context2579I>(null);
+            }
+
+            public Context2579I(string nameOrConnectionString)
+                : base(nameOrConnectionString)
+            {
+            }
+
+            public DbSet<Entity2579> Entities { get; set; }
+        }
+
+        public class Entity2579
+        {
+            public int Id { get; set; }
+        }
+
+        internal sealed class Configuration2579 : DbMigrationsConfiguration<Context2579>
+        {
+            public Configuration2579()
+            {
+                AutomaticMigrationsEnabled = true;
+            }
+
+            public Configuration2579(string connectionString)
+            {
+                AutomaticMigrationsEnabled = true;
+                TargetDatabase = new DbConnectionInfo(connectionString, "System.Data.SqlClient");
+            }
+        }
     }
 }

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<InvalidOperationException>(
                     () => 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

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<DbMigrator>(config, context, DatabaseExistenceState.Unknown, true).Object,
                     null);
 
-                Assert.Null(DbContextInfo.TryGetInfoForContext(internalContext.Owner.GetType()));
+                Assert.Null(DbContextInfo.CurrentInfo);
             }
 
             [Fact]