fixup! Recompute hashcodes on deserialization

Author: fredericDelaporte

src/NHibernate/Cache/CacheKey.cs

@@ -18,7 +18,7 @@ public class CacheKey : IDeserializationCallback
 		private readonly string entityOrRoleName;
 		// hashcode may vary among processes, they cannot be stored and have to be re-computed after deserialization
 		[NonSerialized]
-		private Lazy<int> _hashCode;
+		private int? _hashCode;
 		private readonly ISessionFactoryImplementor _factory;
 
 		/// <summary> 
@@ -37,8 +37,7 @@ public CacheKey(object id, IType type, string entityOrRoleName, ISessionFactoryI
 			this.entityOrRoleName = entityOrRoleName;
 			_factory = factory;
 
-			// No need to delay computation here, but we need the lazy for the deserialization case.
-			_hashCode = new Lazy<int>(GenerateHashCode);
+			_hashCode = GenerateHashCode();
 		}
 
 		//Mainly for SysCache and Memcache
@@ -57,22 +56,22 @@ public override bool Equals(object obj)
 
 		public override int GetHashCode()
 		{
-			// If the object is put in a set or dictionary during deserialization, the lazy will be null, and
-			// we have to compute the hashcode on the fly.
-			return _hashCode?.Value ?? GenerateHashCode();
+			// If the object is put in a set or dictionary during deserialization, the hashcode will not yet be
+			// computed. Compute the hashcode on the fly. So long as this happens only during deserialization, there
+			// will be no thread safety issues. For the hashcode to be always defined after deserialization, the
+			// deserialization callback is used.
+			return _hashCode ?? GenerateHashCode();
 		}
 
-		private int GenerateHashCode()
+		/// <inheritdoc />
+		public void OnDeserialization(object sender)
 		{
-			return type.GetHashCode(key, _factory);
+			_hashCode = GenerateHashCode();
 		}
 
-		/// <inheritdoc />
-		public void OnDeserialization(object sender)
+		private int GenerateHashCode()
 		{
-			// No attempt to compute directly here: the computation depends on another object which may not be
-			// ready at this stage, if it itself or one of its dependency has an OnDeserialization.
-			_hashCode = new Lazy<int>(GenerateHashCode);
+			return type.GetHashCode(key, _factory);
 		}
 
 		public object Key

src/NHibernate/Cache/FilterKey.cs

@@ -1,5 +1,7 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
+using System.Runtime.Serialization;
 using NHibernate.Engine;
 using NHibernate.Impl;
 using NHibernate.Type;
@@ -8,10 +10,16 @@
 namespace NHibernate.Cache
 {
 	[Serializable]
-	public class FilterKey
+	public class FilterKey : IDeserializationCallback
 	{
 		private readonly string _filterName;
-		private readonly Dictionary<string, TypedValue> _filterParameters = new Dictionary<string, TypedValue>();
+
+		// Sets and dictionaries are populated last during deserialization, causing them to be potentially empty
+		// during the deserialization callback. This causes them to be unreliable when used in hashcode computations.
+		// So better serialize them as other structures and use those other structures in hashcode computation.
+		[NonSerialized]
+		private Dictionary<string, TypedValue> _filterParameters = new Dictionary<string, TypedValue>();
+		private IEnumerable<KeyValuePair<string, TypedValue>> _serializedFilterParameters;
 
 		// Since v5.2
 		[Obsolete("Use overload taking a FilterImpl")]
@@ -40,7 +48,11 @@ public override int GetHashCode()
 		{
 			var result = 13;
 			result = 37 * result + _filterName.GetHashCode();
-			result = 37 * result + CollectionHelper.GetHashCode(_filterParameters, NamedParameterComparer.Instance);
+
+			// On deserialization, _filterParameters may be not yet populated. Use _serializedFilterParameters
+			// if it is defined.
+			var filterParameters = _serializedFilterParameters ?? _filterParameters;
+			result = 37 * result + CollectionHelper.GetHashCode(filterParameters, NamedParameterComparer.Instance);
 			return result;
 		}
 
@@ -53,7 +65,24 @@ public override bool Equals(object other)
 			}
 			if (!that._filterName.Equals(_filterName))
 				return false;
-			if (!CollectionHelper.DictionaryEquals<string, TypedValue>(that._filterParameters, _filterParameters))
+
+			// On deserialization, _filterParameters may be not yet populated. Use _serializedFilterParameters
+			// if it is defined.
+			var filterParameters = _filterParameters;
+			if (_serializedFilterParameters != null)
+			{
+				filterParameters = new Dictionary<string, TypedValue>(_serializedFilterParameters.Count());
+				foreach (var param in _serializedFilterParameters)
+					filterParameters.Add(param.Key, param.Value);
+			}
+			var thatFilterParameters = that._filterParameters;
+			if (that._serializedFilterParameters != null)
+			{
+				thatFilterParameters = new Dictionary<string, TypedValue>(that._serializedFilterParameters.Count());
+				foreach (var param in that._serializedFilterParameters)
+					thatFilterParameters.Add(param.Key, param.Value);
+			}
+			if (!CollectionHelper.DictionaryEquals<string, TypedValue>(thatFilterParameters, filterParameters))
 				return false;
 			return true;
 		}
@@ -76,5 +105,21 @@ public static ISet<FilterKey> CreateFilterKeys(IDictionary<string, IFilter> enab
 			}
 			return result;
 		}
+
+		[OnSerializing]
+		private void OnSerializing(StreamingContext context)
+		{
+			_serializedFilterParameters = _filterParameters.ToArray();
+		}
+
+		/// <inheritdoc />
+		public void OnDeserialization(object sender)
+		{
+			_filterParameters = new Dictionary<string, TypedValue>(_serializedFilterParameters.Count());
+			foreach (var param in _serializedFilterParameters)
+				_filterParameters.Add(param.Key, param.Value);
+
+			_serializedFilterParameters = null;
+		}
 	}
 }

src/NHibernate/Cache/QueryKey.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Runtime.Serialization;
 using System.Text;
 using NHibernate.Engine;
@@ -20,12 +21,21 @@ public class QueryKey : IDeserializationCallback
 		private readonly object[] _values;
 		private readonly int _firstRow = RowSelection.NoValue;
 		private readonly int _maxRows = RowSelection.NoValue;
+
+		// Sets and dictionaries are populated last during deserialization, causing them to be potentially empty
+		// during the deserialization callback. This causes them to be unreliable when used in hashcode computations.
+		// So better also serialize them as other structures and use those other structures in hashcode computation.
+		// We nonetheless need to also serialize them, because they are externally supplied and may use a custom
+		// comparer, implementation, ...
 		private readonly IDictionary<string, TypedValue> _namedParameters;
 		private readonly ISet<FilterKey> _filters;
+		private IEnumerable<KeyValuePair<string, TypedValue>> _serializedNamedParameters;
+		private IEnumerable<FilterKey> _serializedFilters;
+
 		private readonly CacheableResultTransformer _customTransformer;
 		// hashcode may vary among processes, they cannot be stored and have to be re-computed after deserialization
 		[NonSerialized]
-		private Lazy<int> _hashCode;
+		private int? _hashCode;
 
 		private int[] _multiQueriesFirstRows;
 		private int[] _multiQueriesMaxRows;
@@ -61,8 +71,7 @@ public QueryKey(ISessionFactoryImplementor factory, SqlString queryString, Query
 			_filters = filters;
 			_customTransformer = customTransformer;
 
-			// No need to delay computation here, but we need the lazy for the deserialization case.
-			_hashCode = new Lazy<int>(ComputeHashCode);
+			_hashCode = ComputeHashCode();
 		}
 
 		public CacheableResultTransformer ResultTransformer
@@ -131,12 +140,32 @@ public override bool Equals(object other)
 				}
 			}
 
-			if (!CollectionHelper.SetEquals(_filters, that._filters))
+			// On deserialization, _namedParameters and _filters may be not yet populated. Use their _serialization
+			// counterparts if they are defined.
+			if (_serializedFilters != null || that._serializedFilters != null)
+			{
+				var filters = _serializedFilters ?? _filters;
+				var thatFilters = that._serializedFilters ?? that._filters;
+				// BagEquals is less efficient than a SetEquals, but if the sets are using a custom comparer, it
+				// will cause less issues than rebuilding sets without the custom comparer.
+				if (!CollectionHelper.BagEquals(filters, thatFilters))
+					return false;
+			}
+			else if (!CollectionHelper.SetEquals(_filters, that._filters))
 			{
 				return false;
 			}
 
-			if (!CollectionHelper.DictionaryEquals(_namedParameters, that._namedParameters))
+			if (_serializedNamedParameters != null || that._serializedNamedParameters != null)
+			{
+				var namedParameters = _serializedNamedParameters ?? _namedParameters;
+				var thatNamedParameters = that._serializedNamedParameters ?? that._namedParameters;
+				// BagEquals is less efficient than a DictionaryEquals, but if the dictionaries are using a custom
+				// comparer, it will cause less issues than rebuilding the dictionaries without the custom comparer.
+				if (!CollectionHelper.BagEquals(namedParameters, thatNamedParameters, NamedParameterComparer.Instance))
+					return false;
+			}
+			else if (!CollectionHelper.DictionaryEquals(_namedParameters, that._namedParameters))
 			{
 				return false;
 			}
@@ -154,16 +183,27 @@ public override bool Equals(object other)
 
 		public override int GetHashCode()
 		{
-			// If the object is put in a set or dictionary during deserialization, the lazy will be null, and
-			// we have to compute the hashcode on the fly.
-			return _hashCode?.Value ?? ComputeHashCodeOnDeserialization();
+			// If the object is put in a set or dictionary during deserialization, the hashcode will not yet be
+			// computed. Compute the hashcode on the fly. So long as this happens only during deserialization, there
+			// will be no thread safety issues. For the hashcode to be always defined after deserialization, the
+			// deserialization callback is used.
+			return _hashCode ?? ComputeHashCode();
 		}
 
-		private int ComputeHashCodeOnDeserialization()
+		/// <inheritdoc />
+		public void OnDeserialization(object sender)
 		{
-			(_filters as IDeserializationCallback).OnDeserialization(this);
-			(_namedParameters as IDeserializationCallback).OnDeserialization(this);
-			return ComputeHashCode();
+			_hashCode = ComputeHashCode();
+
+			_serializedNamedParameters = null;
+			_serializedFilters = null;
+		}
+
+		[OnSerializing]
+		private void OnSerializing(StreamingContext context)
+		{
+			_serializedNamedParameters = _namedParameters?.ToArray();
+			_serializedFilters = _filters?.ToArray();
 		}
 
 		public int ComputeHashCode()
@@ -174,7 +214,10 @@ public int ComputeHashCode()
 				result = 37 * result + _firstRow.GetHashCode();
 				result = 37 * result + _maxRows.GetHashCode();
 
-				result = 37 * result + (_namedParameters == null ? 0 : CollectionHelper.GetHashCode(_namedParameters, NamedParameterComparer.Instance));
+				// On deserialization, _namedParameters may be not yet populated. Use _serializedNamedParameters
+				// if it is defined.
+				var namedParameters = _serializedNamedParameters ?? _namedParameters;
+				result = 37 * result + (namedParameters == null ? 0 : CollectionHelper.GetHashCode(namedParameters, NamedParameterComparer.Instance));
 
 				for (int i = 0; i < _types.Length; i++)
 				{
@@ -201,12 +244,12 @@ public int ComputeHashCode()
 					}
 				}
 
-				if (_filters != null)
+				// On deserialization, _filters may be not yet populated. Use _serializedFilters
+				// if it is defined.
+				var filters = _serializedFilters ?? _filters;
+				if (filters != null)
 				{
-					foreach (var filter in _filters)
-					{
-						result = 37 * result + filter.GetHashCode();
-					}
+					result = 37 * result + CollectionHelper.GetHashCode(filters);
 				}
 
 				result = 37 * result + (_customTransformer == null ? 0 : _customTransformer.GetHashCode());
@@ -215,16 +258,6 @@ public int ComputeHashCode()
 			}
 		}
 
-		/// <inheritdoc />
-		public void OnDeserialization(object sender)
-		{
-			// No attempt to compute directly here: the computation depends on a complex graph of objects,
-			// including hashsets and dictionaries, which are not yet populated. We could force them to get
-			// populated by explicitly calling OnDeserialization on them, but then, we would also have to iterate
-			// them and do the same on any dictionary or set contained by their elements (like FilterKey).
-			_hashCode = new Lazy<int>(ComputeHashCode);
-		}
-
 		public override string ToString()
 		{
 			StringBuilder buf = new StringBuilder()

src/NHibernate/Engine/AssociationKey.cs

@@ -15,15 +15,14 @@ internal sealed class AssociationKey : IDeserializationCallback
 		private readonly string propertyName;
 		// hashcode may vary among processes, they cannot be stored and have to be re-computed after deserialization
 		[NonSerialized]
-		private Lazy<int> _hashCode;
+		private int? _hashCode;
 
 		public AssociationKey(EntityKey ownerKey, string propertyName)
 		{
 			this.ownerKey = ownerKey;
 			this.propertyName = propertyName;
 
-			// No need to delay computation here, but we need the lazy for the deserialization case.
-			_hashCode = new Lazy<int>(GenerateHashCode);
+			_hashCode = GenerateHashCode();
 		}
 
 		public override bool Equals(object that)
@@ -45,17 +44,17 @@ public override bool Equals(object that)
 
 		public override int GetHashCode()
 		{
-			// If the object is put in a set or dictionary during deserialization, the lazy will be null, and
-			// we have to compute the hashcode on the fly.
-			return _hashCode?.Value ?? GenerateHashCode();
+			// If the object is put in a set or dictionary during deserialization, the hashcode will not yet be
+			// computed. Compute the hashcode on the fly. So long as this happens only during deserialization, there
+			// will be no thread safety issues. For the hashcode to be always defined after deserialization, the
+			// deserialization callback is used.
+			return _hashCode ?? GenerateHashCode();
 		}
 
 		/// <inheritdoc />
 		public void OnDeserialization(object sender)
 		{
-			// No attempt to compute directly here: the computation depends on another object which may not be
-			// ready at this stage, if it itself or one of its dependency has an OnDeserialization.
-			_hashCode = new Lazy<int>(GenerateHashCode);
+			_hashCode = GenerateHashCode();
 		}
 
 		private int GenerateHashCode()

src/NHibernate/Engine/CollectionKey.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Runtime.Serialization;
-using NHibernate.Engine;
 using NHibernate.Impl;
 using NHibernate.Persister.Collection;
 using NHibernate.Type;
@@ -19,7 +18,7 @@ public sealed class CollectionKey : IDeserializationCallback
 		private readonly ISessionFactoryImplementor factory;
 		// hashcode may vary among processes, they cannot be stored and have to be re-computed after deserialization
 		[NonSerialized]
-		private Lazy<int> _hashCode;
+		private int? _hashCode;
 
 		public CollectionKey(ICollectionPersister persister, object key)
 			: this(persister.Role, key, persister.KeyType, persister.Factory)
@@ -33,8 +32,7 @@ private CollectionKey(string role, object key, IType keyType, ISessionFactoryImp
 			this.keyType = keyType;
 			this.factory = factory;
 
-			// No need to delay computation here, but we need the lazy for the deserialization case.
-			_hashCode = new Lazy<int>(GenerateHashCode);
+			_hashCode = GenerateHashCode();
 		}
 
 		public override bool Equals(object obj)
@@ -45,17 +43,17 @@ public override bool Equals(object obj)
 
 		public override int GetHashCode()
 		{
-			// If the object is put in a set or dictionary during deserialization, the lazy will be null, and
-			// we have to compute the hashcode on the fly.
-			return _hashCode?.Value ?? GenerateHashCode();
+			// If the object is put in a set or dictionary during deserialization, the hashcode will not yet be
+			// computed. Compute the hashcode on the fly. So long as this happens only during deserialization, there
+			// will be no thread safety issues. For the hashcode to be always defined after deserialization, the
+			// deserialization callback is used.
+			return _hashCode ?? GenerateHashCode();
 		}
 
 		/// <inheritdoc />
 		public void OnDeserialization(object sender)
 		{
-			// No attempt to compute directly here: the computation depends on another object which may not be
-			// ready at this stage, if it itself or one of its dependency has an OnDeserialization.
-			_hashCode = new Lazy<int>(GenerateHashCode);
+			_hashCode = GenerateHashCode();
 		}
 
 		private int GenerateHashCode()