Recompute hashcodes on deserialization #1891

fredericDelaporte · 2018-10-27T12:28:40Z

Computed hashcode values may vary from one process to another. For being consistent inside a process, they must be re-computed after deserialization.

Fixes #1888, and also fixes #1089.

Possible breaking change: many classes, which were not serializing the session factory, do now serialize it. In case of cross-process serialization/deserialization, these session factories will need to be properly named, by setting session_factory_name setting in the configuration used to build them.

src/NHibernate.Test/CacheTest/QueryKeyFixture.cs

fredericDelaporte · 2018-10-27T15:08:44Z

We have two troubles here:

Hashcodes must not be serialized, as they are not guaranteed to be generated identical by different processes.
Hashcodes depending on iterating over dictionaries or sets cannot be reliably computed during deserialization, because dictionaries or sets may not be populated till the very end of the deserialization.

So a thing like QueryKey which holds a set of FilterKey triggers the FilterKey hashcode computation on deserializing. But we have not guarantees that the filter key inner dictionaries, on which its own hashcode computation depends, are ready.

We need to eliminate dictionaries and sets from the serialized state when they are used for hashcode computation, in order to ensure we can compute hashcodes of object without risking iterating over an not yet populated set or dictionary.

fredericDelaporte

Now the issue should be reliably fixed.

fredericDelaporte · 2018-10-28T13:35:44Z

src/NHibernate/Cache/CacheKey.cs

+			// 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();


I could have written it _hashCode ?? (_hashCode = GenerateHashCode()).Value, for lazily initializing it. But that is slightly more complicated code for a corner case, which should rarely happen: deserialization in which the object is put in a set or dictionary as part of the deserialization process. The constructor and the deserialization callback are the normal paths of initialization of this member.
They are mandatory, because solely relying on a lazy initialization could expose the code of GenerateHashCode to multi-threaded usage. It should work in most cases, but better reduce as much as possible the code having to support multi-threading.

code looks much more readable than before

I have some comments at QueryKey and FilterKey, but i test it and it works.
The cache problem will be fixed with this pr.

src/NHibernate/Engine/AssociationKey.cs

fredericDelaporte · 2018-10-28T13:44:58Z

src/NHibernate/Engine/Query/QueryPlanCache.cs

-				{
-					var hash = query.GetHashCode();
-					hash = 29 * hash + (shallow ? 1 : 0);
-					hash = 29 * hash + CollectionHelper.GetHashCode(filterNames, filterNames.Comparer);


filterNames.Comparer is the default one here, the hashset is always constructed by this class, without a custom comparer. The new code does no more use it explicitly, as the hashset may not yet be available, if the hashcode computation has to be done during deserialization.

fredericDelaporte · 2018-10-28T13:45:21Z

src/NHibernate/Engine/Query/QueryPlanCache.cs

-				int hash = query.GetHashCode();
-				hash = 29 * hash + collectionRole.GetHashCode();
-				hash = 29 * hash + (shallow ? 1 : 0);
-				hash = 29 * hash + CollectionHelper.GetHashCode(filterNames, filterNames.Comparer);


Same as previous comment.

src/NHibernate/LockMode.cs

src/NHibernate/Cache/FilterKey.cs

src/NHibernate/Cache/QueryKey.cs

gokhanabatay · 2018-10-28T17:03:11Z

src/NHibernate/Cache/CacheKey.cs

+			// 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();


I have some comments at QueryKey and FilterKey, but i test it and it works.
The cache problem will be fixed with this pr.

fredericDelaporte · 2018-10-28T18:36:11Z

So at least in the use case of @gokhanabatay, this PR solves the trouble of hashcode mismatch seen with his distributed cache usage.

That is still a change a bit heavier than what I would have expected. But it seems to me needed.

gokhanabatay · 2018-10-28T18:43:13Z

So at least in the use case of @gokhanabatay, this PR solves the trouble of hashcode mismatch seen with his distributed cache usage.

That is still a change a bit heavier than what I would have expected. But it seems to me needed.

I agree thank you @fredericDelaporte I will wait to merge at master branch.

src/NHibernate/Cache/FilterKey.cs

gokhanabatay · 2018-10-29T21:00:54Z

@hazzik
@fredericDelaporte
I post the issue to dotnet team please look and watch further action;

Deserialize binary[] to ISet wrong Internal HashCode has been stored #2027
dotnet/core#2027

And finally I got suspicious about https://github.com/nhibernate/NHibernate-Caches is it really works as distributed, because hash code will not return same result across process. If it is than best option is do not check hash codes just check object equality.

Do I need to ignore hash code with distributed cache usage like below? KeySanitizer

CacheConstraints constraints = new CacheConstraints()
{
                MaxKeySize = 250,
                KeySanitizer = x => x.IndexOf('@') >= 0 ? x.Substring(0, x.IndexOf('@')) : x
};

fredericDelaporte · 2018-10-30T02:56:15Z

The PR here precisely dodges the issue you have reported in dotnet core repository. That is the reason for having serialized dictionaries and sets content with arrays in this PR.

The fact that hashcodes are not computed with same values across processes is also the reason for recomputing them on deserialization instead of using the previously stored value in serialized data.

About the use of hashcode in keys, the issue is already known and handled, see nhibernate/NHibernate-Caches#53. This is not yet released (v5.5).

But a static configuration property is already available since v5.4: CoreDistributedCacheProvider.AppendHashcodeToKey. Set it to false. (It can also be set by specifying the attribute append-hashcode="false" on the <coredistributedcache> configuration node.)

hazzik

Please consider following:

src/NHibernate/Cache/FilterKey.cs

src/NHibernate/Cache/QueryKey.cs

src/NHibernate/Engine/AssociationKey.cs

src/NHibernate/Engine/Query/QueryPlanCache.cs

src/NHibernate/LockMode.cs

src/NHibernate/Engine/EntityKey.cs

src/NHibernate/Impl/Printer.cs

hazzik · 2018-11-04T23:21:07Z

Rebased with autosquash

Computed hashcode values may vary from one process to another. For being consistent inside a process, they must be re-computed after deserialization. Fixes nhibernate#1888

Re-implement Printer.ToString

hazzik · 2018-11-05T20:47:43Z

Also fixes #1089.

fredericDelaporte added the t: Fix label Oct 27, 2018

fredericDelaporte mentioned this pull request Oct 27, 2018

Contextual Filter Definition Cache Key Mismatch Error #1889

Closed

This comment has been minimized.

Sign in to view

fredericDelaporte commented Oct 27, 2018

View reviewed changes

src/NHibernate.Test/CacheTest/QueryKeyFixture.cs Show resolved Hide resolved

fredericDelaporte added the possible breaking change label Oct 27, 2018

hazzik previously approved these changes Oct 27, 2018

View reviewed changes

fredericDelaporte dismissed hazzik’s stale review via 3b7605d October 27, 2018 14:11

fredericDelaporte changed the title ~~Recompute hashcodes on deserialization~~ WIP - Recompute hashcodes on deserialization Oct 27, 2018

This comment has been minimized.

Sign in to view

fredericDelaporte changed the title ~~WIP - Recompute hashcodes on deserialization~~ Recompute hashcodes on deserialization Oct 28, 2018

fredericDelaporte force-pushed the SerializedHashcode branch from 3fa5acd to 9b6495b Compare October 28, 2018 13:48

fredericDelaporte commented Oct 28, 2018

View reviewed changes

gokhanabatay reviewed Oct 28, 2018

View reviewed changes

gokhanabatay previously approved these changes Oct 28, 2018

View reviewed changes

hazzik reviewed Oct 29, 2018

View reviewed changes

src/NHibernate/Cache/FilterKey.cs Outdated Show resolved Hide resolved

fredericDelaporte mentioned this pull request Oct 30, 2018

Deserialize binary[] to ISet<T> wrong Internal HashCode has been stored dotnet/core#2027

Closed

hazzik reviewed Oct 30, 2018

View reviewed changes

fredericDelaporte dismissed gokhanabatay’s stale review via f162ede October 31, 2018 17:59

fredericDelaporte commented Oct 31, 2018

View reviewed changes

src/NHibernate/Engine/EntityKey.cs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

hazzik reviewed Nov 4, 2018

View reviewed changes

src/NHibernate/Impl/Printer.cs Outdated Show resolved Hide resolved

hazzik force-pushed the SerializedHashcode branch from 0a80c68 to 5dc7087 Compare November 4, 2018 23:20

hazzik force-pushed the SerializedHashcode branch from 5dc7087 to da5c1b3 Compare November 4, 2018 23:21

fredericDelaporte force-pushed the SerializedHashcode branch from da5c1b3 to bfa9311 Compare November 5, 2018 12:45

Recompute hashcodes on deserialization

c18499e

Computed hashcode values may vary from one process to another. For being consistent inside a process, they must be re-computed after deserialization. Fixes nhibernate#1888

fredericDelaporte force-pushed the SerializedHashcode branch 2 times, most recently from e203384 to 1d9b81d Compare November 5, 2018 12:59

fixup! Recompute hashcodes on deserialization

025c5f9

Re-implement Printer.ToString

fredericDelaporte force-pushed the SerializedHashcode branch from 1d9b81d to 025c5f9 Compare November 5, 2018 13:00

This comment has been minimized.

Sign in to view

hazzik approved these changes Nov 19, 2018

View reviewed changes

Merge branch 'master' into SerializedHashcode

16649c3

fredericDelaporte merged commit 051cc87 into nhibernate:master Nov 20, 2018

fredericDelaporte deleted the SerializedHashcode branch November 20, 2018 10:46

fredericDelaporte added this to the 5.2 milestone Nov 20, 2018

fredericDelaporte added the r: Fixed label Nov 20, 2018

deAtog mentioned this pull request Jul 18, 2019

Cannot instantiate a SessionFactory using Prevalence cache #2190

Closed

karelz mentioned this pull request Jan 31, 2020

Deserialize binary[] to ISet<T> wrong Internal HashCode has been stored dotnet/runtime#27903

Closed

bahusoid mentioned this pull request May 24, 2020

Optimize PersistentGenericSet snapshot #2394

Merged

Recompute hashcodes on deserialization #1891

Recompute hashcodes on deserialization #1891

Uh oh!

Conversation

fredericDelaporte commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

fredericDelaporte commented Oct 27, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

gokhanabatay Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

gokhanabatay Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredericDelaporte Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gokhanabatay Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Oct 28, 2018

Uh oh!

gokhanabatay commented Oct 28, 2018

Uh oh!

Uh oh!

gokhanabatay commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

hazzik commented Nov 4, 2018

Uh oh!

hazzik commented Nov 5, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

fredericDelaporte commented Oct 27, 2018 •

edited

Loading

gokhanabatay commented Oct 29, 2018 •

edited

Loading

fredericDelaporte commented Oct 30, 2018 •

edited

Loading

hazzik left a comment •

edited

Loading