NH-3954 - Dynamic proxy cache may yield a wrong proxy

src/NHibernate.Test/NHSpecificTest/NH3954/Entity.cs

@@ -0,0 +1,55 @@
+using System;
+using System.Collections.Generic;
+
+namespace NHibernate.Test.NHSpecificTest.NH3954
+{
+	class Entity1
+	{
+	}
+	class Entity1FakeProxy
+	{
+	}
+	class Entity1FakeProxy2
+	{
+	}
+
+	class Entity2
+	{
+	}
+	class Entity2FakeProxy
+	{
+	}
+	class Entity2FakeProxy2
+	{
+	}
+
+	class Entity3
+	{
+	}
+	class Entity3FakeProxy
+	{
+	}
+	class Entity3FakeProxy2
+	{
+	}
+
+	class Entity4
+	{
+	}
+	class Entity4FakeProxy
+	{
+	}
+	class Entity4FakeProxy2
+	{
+	}
+
+	class Entity5
+	{
+	}
+	class Entity5FakeProxy
+	{
+	}
+	class Entity5FakeProxy2
+	{
+	}
+}

src/NHibernate.Test/NHSpecificTest/NH3954/EqualsFixture.cs

@@ -0,0 +1,79 @@
+using System;
+using System.Reflection;
+using NHibernate.Proxy;
+using NHibernate.Proxy.DynamicProxy;
+using NHibernate.Type;
+using NUnit.Framework;
+
+namespace NHibernate.Test.NHSpecificTest.NH3954
+{
+	[TestFixture]
+	public class EqualsFixture
+	{
+		private static readonly FieldInfo HashCodeField =
+			typeof(ProxyCacheEntry).GetField("_hashCode", BindingFlags.Instance | BindingFlags.NonPublic);
+
+		/// <summary>
+		/// Allows to simulate a hashcode collision. Issue would be unpractical to test otherwise.
+		/// Hashcode collision must be supported for avoiding unexpected and hard to reproduce failures.
+		/// </summary>
+		private void TweakEntry(ProxyCacheEntry entryToTweak, int hashcode)
+		{
+			// Though hashCode is a readonly field, this works at the time of this writing. If it starts breaking and cannot be fixed,
+			// ignore those tests or throw them away.
+			HashCodeField.SetValue(entryToTweak, hashcode);
+		}
+
+		// Non reg test case
+		[Test]
+		public void TypeEquality()
+		{
+			var entry1 = new ProxyCacheEntry(typeof(Entity1), null);
+			var entry2 = new ProxyCacheEntry(typeof(Entity1), new System.Type[0]);
+			Assert.IsTrue(entry1.Equals(entry2));
+			Assert.IsTrue(entry2.Equals(entry1));
+		}
+
+		[Test]
+		public void TypeInequality()
+		{
+			var entry1 = new ProxyCacheEntry(typeof(Entity1), null);
+			var entry2 = new ProxyCacheEntry(typeof(Entity2), null);
+			TweakEntry(entry2, entry1.GetHashCode());
+			Assert.IsFalse(entry1.Equals(entry2));
+			Assert.IsFalse(entry2.Equals(entry1));
+		}
+
+		// Non reg test case
+		[Test]
+		public void InterfaceEquality()
+		{
+			var entry1 = new ProxyCacheEntry(typeof(Entity1), new[] { typeof(INHibernateProxy), typeof(IProxy) });
+			// Interfaces order inverted on purpose: must be supported.
+			var entry2 = new ProxyCacheEntry(typeof(Entity1), new[] { typeof(IProxy), typeof(INHibernateProxy) });
+			Assert.IsTrue(entry1.Equals(entry2));
+			Assert.IsTrue(entry2.Equals(entry1));
+		}
+
+		// Non reg test case
+		[Test]
+		public void InterfaceEqualityWithLotOfUnordererdAndDupInterfaces()
+		{
+			var entry1 = new ProxyCacheEntry(typeof(Entity1), new[] { typeof(INHibernateProxy), typeof(IProxy), typeof(IType), typeof(IDisposable), typeof(IFilter) });
+			// Interfaces order inverted and duplicated on purpose: must be supported.
+			var entry2 = new ProxyCacheEntry(typeof(Entity1), new[] { typeof(IType), typeof(IProxy), typeof(IFilter), typeof(IDisposable), typeof(IType), typeof(IFilter), typeof(INHibernateProxy) });
+			Assert.IsTrue(entry1.Equals(entry2));
+			Assert.IsTrue(entry2.Equals(entry1));
+		}
+
+		[Test]
+		public void InterfaceInequality()
+		{
+			var entry1 = new ProxyCacheEntry(typeof(Entity1), new[] { typeof(INHibernateProxy), typeof(IProxy) });
+			var entry2 = new ProxyCacheEntry(typeof(Entity1), new[] { typeof(IProxy) });
+			TweakEntry(entry2, entry1.GetHashCode());
+			Assert.IsFalse(entry1.Equals(entry2));
+			Assert.IsFalse(entry2.Equals(entry1));
+		}
+	}
+}

src/NHibernate.Test/NHSpecificTest/NH3954/ProxyCacheFixture.cs

@@ -0,0 +1,155 @@
+using System.Collections.Concurrent;
+using System.Reflection;
+using NHibernate.Proxy;
+using NHibernate.Proxy.DynamicProxy;
+using NUnit.Framework;
+
+namespace NHibernate.Test.NHSpecificTest.NH3954
+{
+	[TestFixture, Explicit("Demonstrates bug impact on cache, but which tests will fail is a bit unpredictable")]
+	public class ProxyCacheFixture
+	{
+		private ProxyCache _cache;
+		private ConcurrentDictionary<ProxyCacheEntry, System.Type> _internalCache;
+		private int _hashCode1;
+		private int _hashCode2;
+
+		private static readonly FieldInfo InternalCacheField =
+			typeof(ProxyCache).GetField("cache", BindingFlags.Static | BindingFlags.NonPublic);
+
+		[SetUp]
+		public void SetUp()
+		{
+			_cache = new ProxyCache();
+
+			_internalCache = (ConcurrentDictionary<ProxyCacheEntry, System.Type>)InternalCacheField.GetValue(null);
+
+			_cache.StoreProxyType(typeof(Entity1FakeProxy), typeof(Entity1));
+			_cache.StoreProxyType(typeof(Entity2FakeProxy), typeof(Entity2), typeof(INHibernateProxy));
+			_cache.StoreProxyType(typeof(Entity3FakeProxy), typeof(Entity3));
+			_cache.StoreProxyType(typeof(Entity4FakeProxy), typeof(Entity4), typeof(IProxy));
+			_cache.StoreProxyType(typeof(Entity5FakeProxy), typeof(Entity5), typeof(INHibernateProxy), typeof(IProxy));
+
+			// Artificially inject other entries with same hashcodes
+			_hashCode1 = new ProxyCacheEntry(typeof(Entity1), null).GetHashCode();
+			Inject(new ProxyCacheEntry(typeof(Entity1), new[] { typeof(INHibernateProxy) }), _hashCode1, typeof(Entity1FakeProxy2));
+			Inject(new ProxyCacheEntry(typeof(Entity3), null), _hashCode1, typeof(Entity3FakeProxy2));
+
+			_hashCode2 = new ProxyCacheEntry(typeof(Entity2), new[] { typeof(INHibernateProxy) }).GetHashCode();
+			Inject(new ProxyCacheEntry(typeof(Entity2), null), _hashCode2, typeof(Entity2FakeProxy2));
+			Inject(new ProxyCacheEntry(typeof(Entity4), new[] { typeof(IProxy) }), _hashCode2, typeof(Entity4FakeProxy2));
+			Inject(new ProxyCacheEntry(typeof(Entity5), new[] { typeof(INHibernateProxy), typeof(IProxy) }), _hashCode2, typeof(Entity5FakeProxy2));
+		}
+
+		private void Inject(ProxyCacheEntry entryToTweak, int hashcode, System.Type result)
+		{
+			TweakEntry(entryToTweak, hashcode);
+			_internalCache[entryToTweak] = result;
+		}
+
+		private static readonly FieldInfo HashCodeField =
+			typeof(ProxyCacheEntry).GetField("_hashCode", BindingFlags.Instance | BindingFlags.NonPublic);
+
+		/// <summary>
+		/// Allows to simulate a hashcode collision. Issue would be unpractical to test otherwise.
+		/// Hashcode collision must be supported for avoiding unexpected and hard to reproduce failures.
+		/// </summary>
+		private void TweakEntry(ProxyCacheEntry entryToTweak, int hashcode)
+		{
+			// Though hashCode is a readonly field, this works at the time of this writing. If it starts breaking and cannot be fixed,
+			// ignore those tests or throw them away.
+			HashCodeField.SetValue(entryToTweak, hashcode);
+		}
+
+		[Test]
+		public void ProxyCacheEntity1FakeProxy()
+		{
+			var result = _cache.GetProxyType(typeof(Entity1));
+			Assert.AreEqual(typeof(Entity1FakeProxy), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity1FakeProxy2()
+		{
+			var entry = new ProxyCacheEntry(typeof(Entity1), new[] { typeof(INHibernateProxy) });
+			TweakEntry(entry, _hashCode1);
+			var result = _internalCache[entry];
+			Assert.AreEqual(typeof(Entity1FakeProxy2), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity2FakeProxy()
+		{
+			var result = _cache.GetProxyType(typeof(Entity2), typeof(INHibernateProxy));
+			Assert.AreEqual(typeof(Entity2FakeProxy), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity2FakeProxy2()
+		{
+			var entry = new ProxyCacheEntry(typeof(Entity2), null);
+			TweakEntry(entry, _hashCode2);
+			var result = _internalCache[entry];
+			Assert.AreEqual(typeof(Entity2FakeProxy2), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity3FakeProxy()
+		{
+			var result = _cache.GetProxyType(typeof(Entity3));
+			Assert.AreEqual(typeof(Entity3FakeProxy), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity3FakeProxy2()
+		{
+			var entry = new ProxyCacheEntry(typeof(Entity3), null);
+			TweakEntry(entry, _hashCode1);
+			var result = _internalCache[entry];
+			Assert.AreEqual(typeof(Entity3FakeProxy2), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity4FakeProxy()
+		{
+			var result = _cache.GetProxyType(typeof(Entity4), typeof(IProxy));
+			Assert.AreEqual(typeof(Entity4FakeProxy), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity4FakeProxy2()
+		{
+			var entry = new ProxyCacheEntry(typeof(Entity4), new[] { typeof(IProxy) });
+			TweakEntry(entry, _hashCode2);
+			var result = _internalCache[entry];
+			Assert.AreEqual(typeof(Entity4FakeProxy2), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity5FakeProxy()
+		{
+			var result = _cache.GetProxyType(typeof(Entity5), typeof(IProxy), typeof(INHibernateProxy));
+			Assert.AreEqual(typeof(Entity5FakeProxy), result);
+		}
+
+		[Test]
+		public void ProxyCacheEntity5FakeProxy2()
+		{
+			// Interfaces order inverted on purpose: must be supported.
+			var entry = new ProxyCacheEntry(typeof(Entity5), new[] { typeof(IProxy), typeof(INHibernateProxy) });
+			TweakEntry(entry, _hashCode2);
+			var result = _internalCache[entry];
+			Assert.AreEqual(typeof(Entity5FakeProxy2), result);
+		}
+
+		[Test]
+		public void ProxyCacheNone()
+		{
+			// Beware not testing the lookup failure of any combination, even tweaked, actually added in cache.
+			// (Otherwise the test may starts failing unexpectedly sometimes, as the original bug ...)
+			// This one was not added in anyway.
+			System.Type result;
+			Assert.IsFalse(_cache.TryGetProxyType(typeof(Entity2), new[] { typeof(IProxy) }, out result));
+		}
+	}
+}

src/NHibernate.Test/NHibernate.Test.csproj

@@ -736,6 +736,9 @@
     <Compile Include="NHSpecificTest\NH3963\Entity.cs" />
     <Compile Include="NHSpecificTest\NH3963\MappedAsFixture.cs" />
     <Compile Include="NHSpecificTest\NH3964\Fixture.cs" />
+    <Compile Include="NHSpecificTest\NH3954\Entity.cs" />
+    <Compile Include="NHSpecificTest\NH3954\EqualsFixture.cs" />
+    <Compile Include="NHSpecificTest\NH3954\ProxyCacheFixture.cs" />
     <Compile Include="NHSpecificTest\NH3950\Entity.cs" />
     <Compile Include="NHSpecificTest\NH3950\Fixture.cs" />
     <Compile Include="NHSpecificTest\NH3952\Entity.cs" />

src/NHibernate/Proxy/DynamicProxy/ProxyCacheEntry.cs

@@ -11,52 +11,57 @@
 
 namespace NHibernate.Proxy.DynamicProxy
 {
-	[Serializable]
-	public class ProxyCacheEntry
+	public class ProxyCacheEntry : IEquatable<ProxyCacheEntry>
 	{
-		private readonly int hashCode;
+		private readonly int _hashCode;
 
 		public ProxyCacheEntry(System.Type baseType, System.Type[] interfaces)
 		{
 			if (baseType == null)
-			{
-				throw new ArgumentNullException("baseType");
-			}
+				throw new ArgumentNullException(nameof(baseType));
 			BaseType = baseType;
-			Interfaces = interfaces ?? new System.Type[0];
+			_uniqueInterfaces = new HashSet<System.Type>(interfaces ?? new System.Type[0]);
 
-			if (Interfaces.Length == 0)
+			if (_uniqueInterfaces.Count == 0)
 			{
-				hashCode = baseType.GetHashCode();
+				_hashCode = baseType.GetHashCode();
 				return;
 			}
-
-			// duplicated type exclusion
-			var set = new HashSet<System.Type>(Interfaces) { baseType };
-			hashCode = 59;
-			foreach (System.Type type in set)
+
+			var allTypes = new List<System.Type>(_uniqueInterfaces) { baseType };
+			_hashCode = 59;
+			foreach (System.Type type in allTypes)
 			{
-				hashCode ^= type.GetHashCode();
+				// This simple implementation is nonsensitive to list order. If changing it for a sensitive one,
+				// take care of ordering the list.
+				_hashCode ^= type.GetHashCode();
 			}
 		}
 
-		public System.Type BaseType { get; private set; }
-		public System.Type[] Interfaces { get; private set; }
+		public System.Type BaseType { get; }
+		public IReadOnlyCollection<System.Type> Interfaces { get { return _uniqueInterfaces; } }
+
+		private HashSet<System.Type> _uniqueInterfaces;
 
 		public override bool Equals(object obj)
 		{
 			var that = obj as ProxyCacheEntry;
-			if (ReferenceEquals(null, that))
+			return Equals(that);
+		}
+
+		public bool Equals(ProxyCacheEntry other)
+		{
+			if (ReferenceEquals(null, other) ||
+				// hashcode inequality allows an early exit, but their equality is not enough for guaranteeing instances equality.
+				_hashCode != other._hashCode ||
+				BaseType != other.BaseType)
 			{
 				return false;
 			}
 
-			return hashCode == that.GetHashCode();
+			return _uniqueInterfaces.SetEquals(other.Interfaces);
 		}
 
-		public override int GetHashCode()
-		{
-			return hashCode;
-		}
+		public override int GetHashCode() => _hashCode;
 	}
 }