Regression: Session.Refresh on entity with lazy collection set throws LazyInitializationException

[deAtog]

After upgrading to 5.3.1 from 4.0.4 we now get a LazyInitializationException when trying to refresh an entity with a lazy collection. The reason for the exception given is: no session or session was closed Minimal pseudo code to reproduce the issue is as follows:

public function CanRefreshEntity(int id) {
    using (ISession s = session_factory.OpenSession()) {
        EntityWithLazyCollection entity;

        using (ITransaction t = s.BeginTransaction()) {
            entity = s.Get<EntityWithLazyCollection>(id);
        }

        using (ITransaction t = s.BeginTransaction()) {
            // LazyInitializationException thrown here..
           s.Refresh(entity);
        }
    }
}

Pseudo xml mapping we are using is as follows:

<class name="EntityWithLazyCollection">
   <id name="Id" column="ParentId" generator="identity"/>
   <set name="Children" lazy="true" inverse="true" cascade="save-update">
      <key unique="true">
         <column name="ParentId" not-null="true"/>
      </key>
      <one-to-many class="Child"/>
   </set>
</class>

[bahusoid]

I can't reproduce it. Exception stack trace?
And full reproducible example (using for instance https://github.com/nhibernate/nhibernate-core-testcase) would also be good

[deAtog]

Stack trace is as follows:

at NHibernate.Collection.AbstractPersistentCollection.ThrowLazyInitializationException(String message)
at NHibernate.Collection.AbstractPersistentCollection.ThrowLazyInitializationExceptionIfNotConnected()
at NHibernate.Collection.AbstractPersistentCollection.Initialize(Boolean writing)
at NHibernate.Collection.AbstractPersistentCollection.Read()
at NHibernate.Collection.Generic.PersistentGenericSet`1.Equals(Object other)
at System.Object.Equals(Object objA, Object objB)
at Domain.EntityWithLazyCollection.set_Children(ISet`1 value)
at (Object , Object[] , SetterCallback )
at NHibernate.Bytecode.Lightweight.AccessOptimizer.SetPropertyValues(Object target, Object[] values)
at NHibernate.Tuple.Entity.PocoEntityTuplizer.SetPropertyValuesWithOptimizer(Object entity, Object[] values)
at NHibernate.Tuple.Entity.PocoEntityTuplizer.SetPropertyValues(Object entity, Object[] values)
at NHibernate.Persister.Entity.AbstractEntityPersister.SetPropertyValues(Object obj, Object[] values)
at NHibernate.Engine.TwoPhaseLoad.InitializeEntity(Object entity, Boolean readOnly, ISessionImplementor session, PreLoadEvent preLoadEvent, PostLoadEvent postLoadEvent, Action`2 cacheBatchingHandler)
at NHibernate.Loader.Loader.InitializeEntitiesAndCollections(IList hydratedObjects, DbDataReader reader, ISessionImplementor session, Boolean readOnly, CacheBatcher cacheBatcher)
at NHibernate.Loader.Loader.DoQuery(ISessionImplementor session, QueryParameters queryParameters, Boolean returnProxies, IResultTransformer forcedResultTransformer, QueryCacheResultBuilder queryCacheResultBuilder)
at NHibernate.Loader.Loader.DoQueryAndInitializeNonLazyCollections(ISessionImplementor session, QueryParameters queryParameters, Boolean returnProxies, IResultTransformer forcedResultTransformer, QueryCacheResultBuilder queryCacheResultBuilder)
at NHibernate.Loader.Loader.DoQueryAndInitializeNonLazyCollections(ISessionImplementor session, QueryParameters queryParameters, Boolean returnProxies)
at NHibernate.Loader.Loader.LoadEntity(ISessionImplementor session, Object id, IType identifierType, Object optionalObject, String optionalEntityName, Object optionalIdentifier, IEntityPersister persister)
at NHibernate.Loader.Entity.AbstractEntityLoader.Load(ISessionImplementor session, Object id, Object optionalObject, Object optionalId)
at NHibernate.Loader.Entity.AbstractEntityLoader.Load(Object id, Object optionalObject, ISessionImplementor session)
at NHibernate.Persister.Entity.AbstractEntityPersister.Load(Object id, Object optionalObject, LockMode lockMode, ISessionImplementor session)
at NHibernate.Event.Default.DefaultRefreshEventListener.OnRefresh(RefreshEvent event, IDictionary refreshedAlready)
at NHibernate.Event.Default.DefaultRefreshEventListener.OnRefresh(RefreshEvent event)
at NHibernate.Impl.SessionImpl.FireRefresh(RefreshEvent refreshEvent)
at NHibernate.Impl.SessionImpl.Refresh(Object obj)
at Tests.CanRefreshEntity(int id)

It seems my simple example left out the fact that EntityWithLazyCollection uses Fody to add PropertyChanging and PropertyChanged event handlers. The Children property of EntityWithLazyCollection thus looks like the following:

public virtual ISet<Child> Children {
   get {
       return this._children;
   }
   protected internal set {
       this.OnPropertyChanging(nameof(Children));

       if (object.Equals((object) this.Children, (object) value)) {
          return;
       }

       this._children = value;
       this.OnPropertyChanged(<>PropertyChangedEventArgs.Children);
   }
}

After spending a day on this yesterday, here is my understanding of what I think is happening here:

1. During the Refresh() operation, EntityWithLazyCollection and all of it's collections/proxies are evicted from the session.
2. The database is queried for the most recent information.
3. An attempt is made to update the existing object with the new data.
4. The Equals check causes an attempt to load the proxy which is no longer associated with the session.

This was not an issue with NHibernate 4.0.4 GA, but I'm not sure if Refresh() was causing it to unproxy all child collections prior to the refresh or not.

[deAtog]

Looking at running the same test with NHibernate 4.0.4 GA reveals that all proxies were being instantiated while updating the entity. This is an obviously overlooked performance issue, and I am left wondering if we should disable generation of PropertyChanged events for collections or manually implement PropertyChanged on all our classes to avoid this issue.

[bahusoid]

Hmm... PersistentGenericBag.Equals force initialize collection and call SequenceEqual. It doesn't look right to me. Collections Equals should make reference equality. Though this behavior since 4.0. It's not the reason for your trouble

[deAtog]

Adding the DoNotCheckEquality attribute for PropertyChanged.Fody to the Children collection changes the above code to:

[DoNotCheckEquality]
public virtual ISet<Child> Children {
   get {
       return this._children;
   }
   protected internal set {
       if (this.Children == value) {
          return;
       }

       this.OnPropertyChanging(nameof(Children));
       this._children = value;
       this.OnPropertyChanged(<>PropertyChangedEventArgs.Children);
   }
}

This change does not cause the proxy to be initialized and avoids the exception. Maybe NHibernate proxies for collections should implement Equals?

[deAtog]

Yeah, I'm going to chalk this one up to an issue with PropertyChanged.Fody. Their documentation states that it uses the == operator to perform comparisons by default when in fact this is not the case. The default behavior can be changed by either adding the DoNotCheckForEquality attribute or by setting the CheckForEquality option to false in the FodyWeavers.xml file. Seeing as how I can't see any reason why we would ever want to check reference types for equality before updating the value, the latter seems like a better solution to resolve the issue. In my personal opinion, the default for that option should have been false and users should have been required to opt-in to the equality comparison instead of having to opt-out.