Fix StackOverflowException in DefaultDynamicLazyFieldInterceptor #1437

offan · 2017-11-16T13:16:21Z

offan · 2017-11-16T13:33:08Z

fixed the test

fredericDelaporte · 2017-11-16T13:34:39Z

src/NHibernate.Test/LazyProperty/LazyPropertyFixture.cs

@@ -50,7 +50,7 @@ protected override void OnTearDown()
 			using (var s = OpenSession())
 			using (var tx = s.BeginTransaction())
 			{
-				Assert.That(s.CreateSQLQuery("delete from Book").ExecuteUpdate(), Is.EqualTo(1));
+				Assert.That(s.CreateSQLQuery("delete from Book").ExecuteUpdate(), Is.EqualTo(2));


Since I have commented on the commit (no PR at that time), the PR was not showing the line I was commenting. This was this one.

I consider it should be changed to:

s.CreateQuery("delete from Book").ExecuteUpdate();

There are no reason to assert the number of inserted things in the teardown. So better remove this undue old assert rather than fixing it. (By the way, it is better to use a HQL delete rather than a SQL one, so I have changed that too in the above code.)

fredericDelaporte · 2017-11-16T13:35:10Z

src/NHibernate.Test/LazyProperty/LazyPropertyFixture.cs

+
+		[Test]
+       		public void CanMergeWithLazyProperty()
+		{


Formatting glitch.

offan · 2017-11-16T18:18:33Z

fixed

fredericDelaporte

I have adjusted the tests, and I have added a potential fix. I think there is still some points to check, see comments details.

fredericDelaporte · 2017-11-17T18:23:20Z

src/NHibernate.Test/LazyProperty/LazyPropertyFixture.cs

@@ -50,7 +51,7 @@ protected override void OnTearDown()
 			using (var s = OpenSession())
 			using (var tx = s.BeginTransaction())
 			{
-				Assert.That(s.CreateSQLQuery("delete from Book").ExecuteUpdate(), Is.EqualTo(1));
+				s.CreateQuery("delete from Book").ExecuteUpdate();


This assert in teardown is indeed "completing" the CanLoadAndSaveObjectInDifferentSessions test. Removed and CanLoadAndSaveObjectInDifferentSessions test changed for correctly asserting itself all that it needs to assert.

fredericDelaporte · 2017-11-17T18:25:14Z

src/NHibernate.Test/LazyProperty/LazyPropertyFixture.cs

+			using (ISession s = OpenSession())
+			{
+				var book = s.Get<Book>(2);
+				Assert.That(book, Is.Null);


The test fails when merging a not yet persisted entity. Since we use assigned ids, we must ensure that any future changes will not cause this test to start merging an already persisted entity.

fredericDelaporte · 2017-11-17T18:31:03Z

src/NHibernate/Event/Default/DefaultMergeEventListener.cs

@@ -230,6 +230,9 @@ private object MergeTransientEntity(object entity, string entityName, object req
 			else
 			{
 				copy = source.Instantiate(persister, id);
+				// #1436: we use the persister to instanciate a new, transient entity. In case it has lazy properties,
+				// we need to to tell its proxy that the properties are all fetched, since it is a transient entity.
+				persister.AfterInitialize(copy, false, source);


Minimal fix. I wonder whether it should be put here or inside SessionImpl.Instantiate called just above. The Instantiate method semantic does not seem to imply it may only be used on transient entities, that is why I have decided to put the fix here.

The stack overflow was occurring because the proxy FieldInterceptor was not initialized. In such case, trying to get it causes a sort of "endless self initialization" loop. Maybe I should try to fix that instead. I will give it a try later.

fredericDelaporte · 2017-11-18T10:36:32Z

src/NHibernate/Intercept/DefaultDynamicLazyFieldInterceptor.cs

+
+				if ("get_FieldInterceptor".Equals(methodName))
+				{
+					return null;


When the field interceptor is not set (we are here in the else part of an if (FieldInterceptor != null)) and is asked for, we should just yield null instead of trying to call it on the underlying Target (which was causing the stack overflow).

The "get_FieldInterceptor".Equals(methodName) test is debatable and causes issues if the entity itself has a FieldInterceptor property. But this case is already bugged due to other similar tests done in DefaultDynamicLazyFieldInterceptor, so adding it does not cause any regression. It will need a separated fix.

I think checks for set_FieldInterceptor/get_FieldInterceptor shall be at the beginning of the method, as in anyway the code inside is a duplicate.

fredericDelaporte · 2017-11-18T10:42:36Z

src/NHibernate/Event/Default/DefaultMergeEventListener.cs

@@ -230,9 +230,6 @@ private object MergeTransientEntity(object entity, string entityName, object req
 			else
 			{
 				copy = source.Instantiate(persister, id);
-				// #1436: we use the persister to instanciate a new, transient entity. In case it has lazy properties,
-				// we need to to tell its proxy that the properties are all fetched, since it is a transient entity.
-				persister.AfterInitialize(copy, false, source);


Fix no more needed with the alternate fix. Not having the proxy initialized is then equivalent to having it considering all properties are already fetched.

Ascertained by adding more asserts in the transient merge test.

fredericDelaporte · 2017-11-18T12:40:48Z

@hazzik, if the refactoring is ok for you, I am going to rebase and squash all that to two commits, by author.

hazzik · 2017-11-18T21:45:38Z

src/NHibernate/Intercept/DefaultDynamicLazyFieldInterceptor.cs

@@ -13,15 +13,25 @@ public class DefaultDynamicLazyFieldInterceptor : IFieldInterceptorAccessor, Pro
 		public object Intercept(InvocationInfo info)
 		{
 			var methodName = info.TargetMethod.Name;
-			if (FieldInterceptor != null)
+			var isPropertyGet = ReflectHelper.IsPropertyGet(info.TargetMethod);
+			var isPropertySet = !isPropertyGet && ReflectHelper.IsPropertySet(info.TargetMethod);


!isPropertyGet && part isn't needed

hazzik · 2017-11-18T22:33:07Z

@fredericDelaporte I did a further refactoring and merged master into here. If you want to squash commits, I don't mind if my name would be lost in the process.

fredericDelaporte · 2017-11-18T22:49:14Z

The point on names is mainly to keep commit associated with who has directly worked on the change. For the fix part, being you or me, no trouble, but I do not think it should be @offan since he may not have analyzed and followed the rational behind the fix. So if someone was later asking him explanations on the fix if he was set as the author, it would not be practical.

…ties * Fixes #1436 * Fixes a test which was relying on an assert in teardown. * Adjusts the new Merge test to ensure it tests its expected case.

fredericDelaporte · 2017-11-18T23:38:58Z

Rebased and squashed.

fredericDelaporte reviewed Nov 16, 2017

View reviewed changes

fredericDelaporte changed the title ~~Add test for GH1436~~ Add test for #1436 Nov 17, 2017

fredericDelaporte reviewed Nov 17, 2017

View reviewed changes

fredericDelaporte self-assigned this Nov 17, 2017

fredericDelaporte reviewed Nov 18, 2017

View reviewed changes

fredericDelaporte mentioned this pull request Nov 18, 2017

Allow naming an entity property FieldInterceptor when it has lazy properties #1444

Merged

fredericDelaporte added the t: Fix label Nov 18, 2017

hazzik reviewed Nov 18, 2017

View reviewed changes

hazzik changed the title ~~Add test for #1436~~ Fix StackOverflowException in DefaultDynamicLazyFieldInterceptor Nov 18, 2017

hazzik mentioned this pull request Nov 18, 2017

Upgrade AsyncGenerator to 0.6.2 and regenerate. #1445

Merged

doroshenko and others added 2 commits November 19, 2017 00:30

Add test for GH1436

ee89456

Fix a stack overflow when merging a transient entity with lazy proper…

9130217

…ties * Fixes #1436 * Fixes a test which was relying on an assert in teardown. * Adjusts the new Merge test to ensure it tests its expected case.

fredericDelaporte approved these changes Nov 19, 2017

View reviewed changes

fredericDelaporte merged commit a218b4f into nhibernate:master Nov 19, 2017

fredericDelaporte added this to the 5.1 milestone Nov 19, 2017

fredericDelaporte added the r: Fixed label Nov 19, 2017

Fix StackOverflowException in DefaultDynamicLazyFieldInterceptor #1437

Fix StackOverflowException in DefaultDynamicLazyFieldInterceptor #1437

Uh oh!

Conversation

offan commented Nov 16, 2017 • edited by hazzik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

offan commented Nov 16, 2017

Uh oh!

fredericDelaporte Nov 16, 2017

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Nov 16, 2017

Choose a reason for hiding this comment

Uh oh!

offan commented Nov 16, 2017

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Nov 17, 2017 • 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!

fredericDelaporte Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Nov 18, 2017

Choose a reason for hiding this comment

Uh oh!

hazzik Nov 18, 2017

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Nov 18, 2017

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Nov 18, 2017

Uh oh!

hazzik Nov 18, 2017 • 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!

hazzik commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Nov 18, 2017

Uh oh!

fredericDelaporte commented Nov 18, 2017

Uh oh!

Uh oh!

offan commented Nov 16, 2017 •

edited by hazzik

Loading

fredericDelaporte Nov 17, 2017 •

edited

Loading

hazzik Nov 18, 2017 •

edited

Loading

hazzik commented Nov 18, 2017 •

edited

Loading