Eshoponcontainers: Question about Entity.GetHashCode()

[dgaspar]

I have a doubt about the customization of the Entity hash code:
https://github.com/dotnet-architecture/eShopOnContainers/blob/0b4f44659dc7e8fc6e2a942b16caf02c87532b9a/src/Services/Ordering/Ordering.Domain/SeedWork/Entity.cs#L66-L78

Isn't it risky to have mutating hash codes?
The moment the transient Entity gets saved, won't it receive an Id value and won't its hash code change?
If that Entity is in a hash table, wouldn't it cause lookups to suddenly fail?
Is such lookup failure intentional?

If an object’s hash code can mutate while it is in the hash table then clearly the Contains method stops working. You put the object in bucket #5, you mutate it, and when you ask the set whether it contains the mutated object, it looks in bucket #74 and doesn’t find it.

source: https://ericlippert.com/2011/02/28/guidelines-and-rules-for-gethashcode/

[sughosneo]

Hi @dgaspar, thank you for reaching out.

Yes, it might not be a good idea to calculate a hash code on a mutable field. That's the reason the Id field (of primitive type) and protected in nature has been used to evaluate the hash for the object.
The value of the Id field is not expected to get changed for the entire lifetime of an object. If it becomes null a default hash value gets returned.

Hope this helps !

[dgaspar]

Thanks for the clarification!

The value of the Id field is not expected to get changed for the entire lifetime of an object.

But by using the above implementation, saving a transient Entity causes its hash code to mutate during its lifetime, isn't it?

Btw, what's the practical benefit of overriding the hash code generation?

[dgaspar]

Remarks of https://docs.microsoft.com/en-us/dotnet/api/system.object.gethashcode?redirectedfrom=MSDN&view=netcore-3.1#remarks mention:

If you override the GetHashCode method, you should also override Equals, and vice versa. If your overridden Equals method returns true when two objects are tested for equality, your overridden GetHashCode method must return the same value for the two objects.

Understood, it's not only for hash tables :)

[AnorZaken]

So the Entity-class _allows_ its children to mutate a non-transient Id, but it _assumes_ they won't, despite not conveying this assumption anywhere?

If you override the GetHashCode method, you should also override Equals, and vice versa. If your overridden Equals method returns true when two objects are tested for equality, your overridden GetHashCode method must return the same value for the two objects.

In other words, if an object can mutate such that its equality is affected, then its hashcode SHOULD mutate as well.
The Entity class does not uphold this:

Let Entity A with Id = 1, and Entity B with Id = 2.
Call GetHashCode() on both to generate and cache a hashcode in _requestedHashCode.
Mutate Id of Entity B to 1.
Entity A and B are now equal, however hashcodes are NOT equal, due to Entity B retaining its outdated hashcode.

The Entity-class should clear / invalidate its cached hashcode in the Id-property setter.

If the only intended purpose of caching the hash was to not mutate it, the caching should probably just be removed altogether. It is after all questionable whether caching the hash actually has any performance benefit given such a simple hash; the cost of a single xor is very low on modern hardware, such that the cost of branching instructions and cpu-cache utilization are likely higher.(*) At a glance the complexity cost seems unjustified.

The real issue in the original question by @dgaspar is that storing equality-mutating objects in a hash-based collection is a code-smell in the first place (likely to introduce bugs).
In such a scenario either refactor to a solution that stores [equality-]immutable objects in the collection, or supply an IEqualityComparer to the collection with an Equals and GetHashCode implementation that doesn't change during the lifetime of the object (and of course make sure you have tests in place that covers your intended usage scenario).

Use of a hash-based collection to find equal objects where the object's implementation allows equality-mutation would require some mechanism in place to guarantee removal and re-insertion on equality-mutation events. One should probably seek a less complex solution first.

_(*This requires real-world testing: stressing the cache, and stressing the cpu's branch-predictor. A simple test snippet would fit inside the cpu-cache thus resulting in zero cache-misses, and would similarly likely be trivial for the cpu's branch-predictor to predict thus resulting in near zero branch mispredictions / zero pipeline flushes.)_