Opened 5 years ago

Closed 4 years ago

#1898 closed defect (done)

Object graph traversal fails when encountering objects of type System.Reflection.Emit.SignatureHelper

Reported by: abeham Owned by: mkommend
Priority: medium Milestone: HeuristicLab 3.3.8
Component: Common Version: 3.3.8
Keywords: Cc:

Description

Calling GetHashCode() to see if it's contained in the HashSet of previously seen objects results in a NullReferenceException.

This type was introduced into the object graph, because I tried to add protocol buffer's ExtensionRegistry as a field to the ExternalEvaluator. I can find another, slightly slower solution for my problem, but the problem remains that object graph traversal is not completely safe.

Line 50 in ObjectExtensions.cs fails:

if (o != null && !objects.Contains(o) && !ExcludeType(o.GetType()))

Excluding this type would require swapping the second and the third check. I don't know if this has performance implications, but I suspect that o.GetType().GetHashCode() is slower than o.GetHashCode().

Change History (9)

comment:1 Changed 5 years ago by mkommend

  • Owner changed from swagner to mkommend
  • Status changed from new to accepted

comment:2 Changed 5 years ago by mkommend

r8309: Minor modifications in CollectGASample unit test.

comment:3 Changed 5 years ago by mkommend

r8310:

  • Corrected GetAllFields extension methods to return fields across the hierarchy only once.
  • Changed filtering of object and types to first check for excluded types.
  • Removed unnecassary comparer for excluded members HashSet.
  • Corrected outdated comments
  • Added SignatureHelper to the excluded types.

Overall the performance increased due to the changes above and the signature helper should not cause problems anymore. Btw, thanks for spotting this defect.

comment:4 Changed 5 years ago by mkommend

  • Owner changed from mkommend to abeham
  • Status changed from accepted to reviewing

comment:5 follow-up: Changed 5 years ago by abeham

  • Owner changed from abeham to mkommend
  • Status changed from reviewing to assigned

Thx for fixing this!

I think it's not correct to remove the ReferenceEqualityComparer. We always want to check objects by reference and we need to ignore any object's overriden Equals and GetHashCode methods. MSDN confirms that the default EqualityComparer<T> (if that's not somehow different from the default equality comparer for sets) makes use of these overrides: "The Default property checks whether type T implements the System.IEquatable<T> interface and, if so, returns an EqualityComparer<T> that uses that implementation. Otherwise, it returns an EqualityComparer<T> that uses the overrides of Object.Equals and Object.GetHashCode provided by T."

Also I don't think DeclaredOnly is correct, according to MSDN: "Specifies that only members declared at the level of the supplied type's hierarchy should be considered. Inherited members are not considered.". Why do we want to ignore inherited members?

comment:6 in reply to: ↑ 5 Changed 5 years ago by mkommend

Replying to abeham:

Thx for fixing this!

I think it's not correct to remove the ReferenceEqualityComparer. We always want to check objects by reference and we need to ignore any object's overriden Equals and GetHashCode methods. MSDN confirms that the default EqualityComparer<T> (if that's not somehow different from the default equality comparer for sets) makes use of these overrides: "The Default property checks whether type T implements the System.IEquatable<T> interface and, if so, returns an EqualityComparer<T> that uses that implementation. Otherwise, it returns an EqualityComparer<T> that uses the overrides of Object.Equals and Object.GetHashCode provided by T."

The comparer was only used to determine the if members should be ignored in the object graph traversal. As the HashSet containing the objects to ignore is provided as parameter, the user can control how the objects should be compared by providing another comparer.

Also I don't think DeclaredOnly is correct, according to MSDN: "Specifies that only members declared at the level of the supplied type's hierarchy should be considered. Inherited members are not considered.". Why do we want to ignore inherited members?

The MSDN also states only public & protected fields are returned and to get all private fields we have to follow the inheritance chain upwards. As we do so, we do not need to consider fields declared in base classes, as these are discovered by the recursive application of the method on its base type.

comment:7 Changed 5 years ago by mkommend

  • Status changed from assigned to accepted

comment:8 Changed 5 years ago by mkommend

  • Status changed from accepted to readytorelease

comment:9 Changed 4 years ago by swagner

  • Resolution set to done
  • Status changed from readytorelease to closed
  • Version changed from 3.3.7 to 3.3.8
Note: See TracTickets for help on using tickets.