NH-3977 - Thread safety weaknesses of MapBasedSessionContext, test case and fix.

Author: fredericDelaporte

src/NHibernate.Test/ConnectionTest/MapBasedSessionContextFixture.cs

@@ -0,0 +1,79 @@
+using System.Collections;
+using System.Threading;
+using NHibernate.Cfg;
+using NHibernate.Context;
+using NHibernate.Engine;
+using NUnit.Framework;
+
+namespace NHibernate.Test.ConnectionTest
+{
+	[TestFixture]
+	public class MapBasedSessionContextFixture : ConnectionManagementTestCase
+	{
+		protected override ISession GetSessionUnderTest()
+		{
+			ISession session = OpenSession();
+			session.BeginTransaction();
+			return session;
+		}
+
+		protected override void Configure(Configuration configuration)
+		{
+			base.Configure(cfg);
+			cfg.SetProperty(Environment.CurrentSessionContextClass, typeof(TestableMapBasedSessionContext).AssemblyQualifiedName);
+		}
+
+		protected override void OnSetUp()
+		{
+			TestableMapBasedSessionContext._map = null;
+		}
+
+		[Test]
+		public void MapContextThreadSafety()
+		{
+			using (var factory1 = cfg.BuildSessionFactory())
+			using (var session1 = factory1.OpenSession())
+			using (var factory2 = cfg.BuildSessionFactory())
+			using (var session2 = factory2.OpenSession())
+			{
+				var thread1 = new Thread(() =>
+				{
+					CurrentSessionContext.Bind(session1);
+				});
+
+				var thread2 = new Thread(() =>
+				{
+					CurrentSessionContext.Bind(session2);
+				});
+
+				thread1.Start();
+				thread2.Start();
+				thread1.Join();
+				thread2.Join();
+
+				Assert.IsTrue(CurrentSessionContext.HasBind(factory1), $"No session bound to \"{nameof(factory1)}\" factory.");
+				Assert.IsTrue(CurrentSessionContext.HasBind(factory2), $"No session bound to \"{nameof(factory2)}\" factory.");
+			}
+		}
+	}
+
+	public class TestableMapBasedSessionContext : MapBasedSessionContext
+	{
+		public TestableMapBasedSessionContext(ISessionFactoryImplementor factory) : base(factory) { }
+
+		// Context is the app with such implementation. Just for the test case.
+		internal static IDictionary _map;
+
+		protected override IDictionary GetMap()
+		{
+			return _map;
+		}
+
+		protected override void SetMap(IDictionary value)
+		{
+			// Give a fair chance to have a concurrency bug if base implementation is not thread safe.
+			Thread.Sleep(100);
+			_map = value;
+		}
+	}
+}

src/NHibernate.Test/NHibernate.Test.csproj

@@ -206,6 +206,7 @@
     <Compile Include="ConnectionTest\ConnectionManagementTestCase.cs" />
     <Compile Include="ConnectionTest\Other.cs" />
     <Compile Include="ConnectionTest\Silly.cs" />
+    <Compile Include="ConnectionTest\MapBasedSessionContextFixture.cs" />
     <Compile Include="ConnectionTest\ThreadLocalCurrentSessionTest.cs" />
     <Compile Include="ConventionsTestCase.cs" />
     <Compile Include="Criteria\AddNumberProjection.cs" />

src/NHibernate/Context/MapBasedSessionContext.cs

@@ -1,5 +1,5 @@
 using System.Collections;
-
+using System.Collections.Concurrent;
 using NHibernate.Engine;
 
 namespace NHibernate.Context
@@ -8,6 +8,9 @@ public abstract class MapBasedSessionContext : CurrentSessionContext
 	{
 		private readonly ISessionFactoryImplementor _factory;
 
+		// Must be static, different instances of MapBasedSessionContext may have to yield the same map.
+		private static readonly object _locker = new object();
+
 		protected MapBasedSessionContext(ISessionFactoryImplementor factory)
 		{
 			_factory = factory;
@@ -20,30 +23,39 @@ protected override ISession Session
 		{
 			get
 			{
-				IDictionary map = GetMap();
-				if (map == null)
-				{
-					return null;
-				}
-				else
-				{
-					return map[_factory] as ISession;
-				}
+				ISession value = null;
+				GetConcreteMap()?.TryGetValue(_factory, out value);
+				// We want null if no value was there, no need to explicitly handle false outcome of TryGetValue.
+				return value;
 			}
 			set
 			{
-				IDictionary map = GetMap();
+				var map = GetConcreteMap();
 				if (map == null)
 				{
-					map = new Hashtable();
-					SetMap(map);
+					// Double check locking. Cannot use a Lazy<T> for such a semantic: the map can be bound
+					// to some execution context through SetMap/GetMap, a Lazy<T> would defeat those methods purpose.
+					lock (_locker)
+					{
+						map = GetConcreteMap();
+						if (map == null)
+						{
+							map = new ConcurrentDictionary<ISessionFactoryImplementor, ISession>();
+							SetMap(map);
+						}
+					}
 				}
 				map[_factory] = value;
 			}
 		}
 
+		private ConcurrentDictionary<ISessionFactoryImplementor, ISession> GetConcreteMap()
+		{
+			return (ConcurrentDictionary<ISessionFactoryImplementor, ISession>)GetMap();
+		}
+
 		/// <summary>
-		/// Get the dictionary mapping session factory to its current session.
+		/// Get the dictionary mapping session factory to its current session. Yield <c>null</c> if none have been set.
 		/// </summary>
 		protected abstract IDictionary GetMap();