StackOverflowException for lazy proxied entities with explicit interface properties

In a domain model where PKs are not always of the same type, a fairly complex architecture of interfaces and classes is defined.
Said architecture provide for:

• a IEntity defining a object Id { get; } property

• a IEntity<T> : IEntity defining a new T Id { get; } property

Hence, every domain object has to implement both:

• a object IEntity.Id { get { return Id; } }

• and a public virtual T Id { get; }

To make things even more complicated, some entities has lazy fields on them and a IPostLoadEventListener is used.

Problem is: all this machinery functioned pretty well... up until the latest release of NHibernate.
In 5.1, for all entities with lazies, when trying to access the aforementioned Id property inside of the IPostLoadEventListener, a StackOverflowException is thrown.

Please note that all three elements above are needed for the bug to trigger.

For the test case

[fredericDelaporte]

In fact, all this can be abstracted to : Lazy property proxy does not support explicitly implemented interface members.

I reproduce it with this simplified model:

public interface IEntity
{
	object Thing { get; }
}

public class Entity : IEntity
{
	public virtual int Id { get; set; }

	object IEntity.Thing { get { return null; } }
	public virtual string Name { get; set; }
	public virtual string ALongText { get; set; }
}

No changes done on mappings, which must include a lazy non-entity property for the issue to occur.

Test case simplified to:

[Test]
public void ShouldNotThrowStackOverflowException()
{
	using (var session = OpenSession())
	using (session.BeginTransaction())
	{
		IEntity result = session.Get<Entity>(2);

		Assert.That(result.Thing, Is.EqualTo(null));
	}
}

The stack trace for this case:

   at NHibernate.Intercept.DefaultDynamicLazyFieldInterceptor.Intercept(InvocationInfo info)
   at EntityProxy.get_Thing()
   at EntityProxy.get_Thing_callback()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at NHibernate.Proxy.DynamicProxy.InvocationInfo.InvokeMethodOnTarget() (line 111)
   at NHibernate.Intercept.DefaultDynamicLazyFieldInterceptor.Intercept(InvocationInfo info) (line 24)
   at EntityProxy.get_Thing()
   at EntityProxy.get_Thing_callback()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at NHibernate.Proxy.DynamicProxy.InvocationInfo.InvokeMethodOnTarget()
   at NHibernate.Intercept.DefaultDynamicLazyFieldInterceptor.Intercept(InvocationInfo info)
   at EntityProxy.get_Thing()
   at NHibernate.Test.NHSpecificTest.GH1628.Fixture.ShouldNotThrowStackOverflowException() in nhibernate-core\src\NHibernate.Test\NHSpecificTest\GH1628\Fixture.cs:line 57

I am removing my previous comments here for cleaning-up.

[fredericDelaporte]

Lazy property proxy does not support explicitly implemented interface members.

That was already the case in 5.0, but it was ignoring them, while now an attempt at proxifying them is done.

More precisely, the proxy builder was ignoring interfaces from the entity type when this type was a class, not itself an interface. It is then proxifying the entity method filtered for excluding private ones (which also excludes explicit interface implementations), then it proxifies the methods from a bunch of interfaces (without filtering them further).

In 5.1, the interfaces implemented by the entity type are always added to list of interfaces to be proxified, causing explicit implementation to be proxified. But their proxification does not work.

The implied change is here. It seems to be a simplification done during a change about the proxy cache.

For reverting to the previous behavior, I think interfaces.UnionWith(baseType.GetInterfaces()); should be moved in the if (baseType.IsInterface) right above.

Or was this additional change also supposed to fix something without telling it?
In such case, some work is needed to fix the proxifying logic which currently results in recursive calls for explicitly implemented interface method.

[fredericDelaporte]

Checking the new implementation of the "regular proxy" case (proxifying the whole entity, not just a property access), it does proxify explicit implementation too, but does not seem to have the issue.

Maybe the change mentioned in my previous comment was done for consistency. But is it needed for the lazy property proxy case? (In the "regular proxy" case, it needs it for ensuring the state of the object is loaded when accessing such a property, but that requirement does not apply to the lazy property proxy case.) Can we map an explicitly implemented interface property of an entity mapped on a concrete class? It would be quite a cumbersome setup. I think we should revert to previous behavior by doing the change I suggest in my previous comment. At least it fixes the case here. (I am currently running the whole test suite to check it does not cause any regression. Update: no regression.)

[hazzik]

The implied change is here. It seems to be a simplification done during a change about the proxy cache.

I'm not sure why it can cause the issue? What interfaces are missing when doing the "optimized" code?

[fredericDelaporte]

The #1454 change does not cause an interface to be missing, but instead causes additional interfaces to be added to the list of interface for which methods are to be proxified. These additional interfaces are those implemented by the entity class. Previously, they were not added: only methods found on the entity class and its ancestors classes were taken into account, provided they are not private (thus excluding explicitly implemented interface methods).

With the #1454 change, the entity class interfaces are now added to the list of interfaces for which methods are to be proxified. Maybe that looks more correct, but that causes this bug, due to the proxification of explicitly implemented interface methods currently resulting in an infinite recursive call, at least in the case of the DynamicProxy.

I think that for a patch fix, we should just revert to the previous behavior. Maybe should we also add a PR for next minor/major release in order to add back the class interfaces and fix the proxification of explicitly implemented methods, but I doubt it.

I argue that this change was in fact undue, and not actually more correct for dynamic proxy case: dynamic proxies do not need to take into account interfaces of the entity class. The sole purpose of doing so is proxifying explicitly implemented methods/properties. Other methods and properties are proxified by inspection of the entity class.

I do not think the dynamic proxy needs to proxify explicitly implemented methods/properties, because I do not think it makes sens to map an explicitly implemented property (it is maybe not even possible), so such a property should not be furthermore lazily loaded. The dynamic proxy is a no-op proxy on properties and methods which are not lazy, so it does not need to proxify things that are not lazy. Only the regular full entity proxy cares about all publicly accessible methods/properties, because its contract is to guarantee that the state of the entity is loaded if an access to such methods/properties is done (excepted for the id and standard object methods if not overridden.)