NH-3985 - Bugfix for subsequent child sessions being returned in disposed state #600

alobakov · 2017-04-16T06:12:43Z

NH-3985 - Test case and fix for subsequent child sessions being returned in disposed state

fredericDelaporte · 2017-04-16T09:37:40Z

src/NHibernate.Test/NHSpecificTest/NH3985/Mappings.hbm.xml

+			<generator class="identity" />
+		</id>
+		<property name="Name" type="String">
+			<column name="Name" not-null="true" length="50" sql-type="NVARCHAR(50)" />


Avoid specifying db specific types. Better not specify any type at all, NHibernate infers them.

fredericDelaporte · 2017-04-16T09:39:03Z

src/NHibernate.Test/NHSpecificTest/NH3985/Entity.cs

+
+		#endregion
+
+		public virtual int ProcessID { get; set; }


Oracle will use decimal instead for identities/sequences, so Oracle build may failed due to this.

fredericDelaporte · 2017-04-16T09:43:22Z

src/NHibernate.Test/NHSpecificTest/NH3985/Mappings.hbm.xml

+	<class name="Process" table="Processes">
+		<id name="ProcessID" type="Int32">
+			<column name="ProcessID" not-null="true" sql-type="INT" />
+			<generator class="identity" />


Identity with Oracle use decimal. Using native as generator would be safer by the way, current NHibernate Oracle dialect does not implement identity though Oracle supports it. I believe it does fallback to sequence nether-less, so will work like native, but at least for native that is its documented behavior.

You may also consider using an app side generator like guid.comb, that helps avoid db specific issues.

alobakov · 2017-04-16T15:13:27Z

Doh... Man, thanks for all the reminders. That was a total brain fart on my part. :))

Will fix.

alobakov · 2017-04-16T18:08:01Z

Ok, fixed that. - Let me know if anything else doesn't look right.
BTW, is there a way to re-run the build and tests on the existing pull request to verify that Oracle-related tests would now succeed? Or should I just close this PR and submit a new one?

hazzik · 2017-04-17T05:22:24Z

src/NHibernate/Impl/SessionImpl.cs

+				var childImpl = _childSession as SessionImpl;
+
+				// if child session never existed or has already been disposed, create new
+				if (childImpl == null || childImpl.IsAlreadyDisposed)


What if instead of this check we remove the reference on disposal?

That's what I'm doing in the implementation of NH-3986 with a collection of child sessions, but that will come in as a separate pull request. If both end up included in the same release, this becomes moot point.

There are 2 points to implement the correct logic here:

It can be back-ported (why? because this is actually a bug) to previous versions without a lot of work required

I'm not yet sure that the improvement will make it to 5.0

hazzik · 2017-04-17T05:22:45Z

src/NHibernate.Test/NHibernate.Test.csproj

@@ -3248,6 +3250,9 @@
    <EmbeddedResource Include="NHSpecificTest\NH1291AnonExample\Mappings.hbm.xml" />
  </ItemGroup>
  <ItemGroup>
+    <EmbeddedResource Include="NHSpecificTest\NH3985\Mappings.hbm.xml">
+      <SubType>Designer</SubType>


No designer please

Yes, I didn't like that either, but Visual Studio seems to keep adding it. Not sure exactly what it means and why it gets added for some mapping files while not others.

Looks like there's plenty of other hbm.xml files with this sub-element, but only select items have it. - Very odd.

alobakov · 2017-04-17T08:02:38Z

src/NHibernate.Test/NHibernate.Test.csproj

@@ -3248,6 +3250,9 @@
    <EmbeddedResource Include="NHSpecificTest\NH1291AnonExample\Mappings.hbm.xml" />
  </ItemGroup>
  <ItemGroup>
+    <EmbeddedResource Include="NHSpecificTest\NH3985\Mappings.hbm.xml">
+      <SubType>Designer</SubType>


Looks like there's plenty of other hbm.xml files with this sub-element, but only select items have it. - Very odd.

alobakov · 2017-04-18T02:58:35Z

src/NHibernate.Test/NHibernate.Test.csproj

@@ -3248,6 +3244,7 @@
    <EmbeddedResource Include="NHSpecificTest\NH1291AnonExample\Mappings.hbm.xml" />
  </ItemGroup>
  <ItemGroup>
+    <EmbeddedResource Include="NHSpecificTest\NH3985\Mappings.hbm.xml" />


Removed this and other SubType elements, however it seems to have been an ongoing issue with Visual Studio since VS2005, so there's no clear way to eradicate them once and for all. They appear to be completely harmless though.

Thanks.

These tags become harmful when you try to merge branches as they provide false anchors for the merge tools.

hazzik · 2017-04-23T22:30:28Z

I modified your PR to remove child session from parent as soon as it gets disposed.

hazzik · 2017-04-23T22:41:07Z

@alobakov next time please make sure that PR is made from a branch, not from master.

alobakov · 2017-04-24T01:33:53Z

Ok will do.
It did have its own branch in my fork, though there were no specific instructions in CONTRIBUTING.md

fredericDelaporte · 2017-04-24T16:27:01Z

The Commit your Test Case section tells it. Maybe a bit late in this guide. Not sure it mandates a change.

fredericDelaporte · 2017-04-24T16:42:47Z

src/NHibernate/Impl/SessionImpl.cs

@@ -1681,6 +1682,9 @@ private void Dispose(bool isDisposing)
 				// free unmanaged resources here

 				IsAlreadyDisposed = true;
+
+				rootSession?.RemoveChildSession(this);


Should be moved to Close method, otherwise a closed child-session may be returned by GetChildSession although they are unusable.

fredericDelaporte · 2017-04-24T17:33:03Z

src/NHibernate/Impl/SessionImpl.cs

+
+		void RemoveChildSession(ISession session)
+		{
+			Interlocked.CompareExchange(ref _childSession, null, session);


Session is not thread safe, so we should not care about thread safety here.

I know there is a case where Dispose can be called from another thread: when a transaction in which the session was enlisted ends. But this case is buggy and doomed anyway: the child-session is already awaiting disposal (TransactionContext.ShouldCloseSessionOnDistributedTransactionCompleted being true) and should already be considered unusable for any other purpose than ending that transaction. So having it still returnable by its parent session is a trouble.

Being thread safe here will not help avoid troubles. Either we should ensure the child session is immediately removed from its parent on child dispose/close call, even when its disposal is postponed due to an ongoing transaction (TransactionContext not null), or we should cease postponing disposal when there is an ongoing transaction, as Hibernate do with JdbcContext. In both cases, we would no more be exposed to the transaction completion occurring on a distinct thread trouble.

Just seeing now these commit comments on the same subject.

In do not really get how another child-session could be there with current implementation. But well, if it is not about thread-safety but only for not removing another one, I would rather implement it without any threading helper, with a if or iif.

It would be more explicit what is currently done here. Using the threading helper without a comment to tell why will let any later reader to wonder why is it coded that way, are there any need to be thread safe here, ...

hazzik · 2017-04-25T09:57:52Z

Changed as per review comments

…d state

hazzik · 2017-04-25T21:16:10Z

Thanks everyone. Squashed and applied to master now.

fredericDelaporte reviewed Apr 16, 2017

View reviewed changes

alobakov force-pushed the master branch 2 times, most recently from c2b5b0a to 07fedc2 Compare April 16, 2017 17:58

hazzik reviewed Apr 17, 2017

View reviewed changes

alobakov commented Apr 18, 2017

View reviewed changes

hazzik force-pushed the master branch 2 times, most recently from 68e06d4 to b24a75a Compare April 23, 2017 22:38

hazzik approved these changes Apr 23, 2017

View reviewed changes

hazzik requested a review from fredericDelaporte April 23, 2017 22:40

fredericDelaporte reviewed Apr 24, 2017

View reviewed changes

fredericDelaporte approved these changes Apr 25, 2017

View reviewed changes

NH-3985 - Fix for subsequent child sessions being returned in dispose…

4558d53

…d state

hazzik force-pushed the master branch from a31c36e to 4558d53 Compare April 25, 2017 21:06

hazzik merged commit 4558d53 into nhibernate:master Apr 25, 2017

fredericDelaporte added this to the 5.0.0 milestone Apr 26, 2017

fredericDelaporte mentioned this pull request May 4, 2017

NH-4003 - porting ISessionCreationOptions #624

Merged

hazzik added r: Fixed and removed r: Fixed labels Aug 3, 2017

fredericDelaporte added the t: Bug label Sep 7, 2017

NH-3985 - Bugfix for subsequent child sessions being returned in disposed state #600

NH-3985 - Bugfix for subsequent child sessions being returned in disposed state #600

Uh oh!

Conversation

alobakov commented Apr 16, 2017 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alobakov commented Apr 16, 2017

Uh oh!

alobakov commented Apr 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Apr 23, 2017

Uh oh!

hazzik commented Apr 23, 2017

Uh oh!

alobakov commented Apr 24, 2017

Uh oh!

fredericDelaporte commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Apr 24, 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 Apr 25, 2017

Uh oh!

hazzik commented Apr 25, 2017

Uh oh!

Uh oh!

alobakov commented Apr 16, 2017 •

edited by fredericDelaporte

Loading

alobakov commented Apr 16, 2017 •

edited

Loading

fredericDelaporte Apr 24, 2017 •

edited

Loading