NH-4077 - do not auto-flush while already flushing #682

fredericDelaporte · 2017-09-04T14:37:31Z

The trouble was due to auto-flushing getting triggered from a flush event, altering the flush action list while it was iterating over it.

fredericDelaporte · 2017-09-04T16:11:05Z

src/NHibernate/Engine/ISessionImplementor.cs

+		/// being called multiple times.
+		/// </summary>
+		/// <returns>A disposable to dispose when auto flush should be restored.</returns>
+		IDisposable SuspendAutoFlush();


Needing it from an other internal class than the session, and factoring it by the way.

fredericDelaporte · 2017-09-04T16:15:42Z

src/NHibernate/Impl/SessionImpl.cs

+					finally
+					{
+						AfterOperation(success);
+					}
 				}


Same as previous AfterOperation case.

fredericDelaporte · 2017-09-04T16:15:56Z

src/NHibernate/Impl/SessionImpl.cs

+					finally
+					{
+						AfterOperation(success);
+					}
 				}


Discrepancy: the AfterOperation(success); was previously put after decrementing back dontFlushFromFind, meaning after operations could trigger auto-flush. Now it is done before leaving the new using, so before re-enabling auto-flushes.
But AfterOperation does in fact nothing if we are inside a transaction. And when we are not inside a transaction, auto-flush is anyway already disabled. So this change has no consequences with current code.

fredericDelaporte · 2017-09-04T16:16:58Z

src/NHibernate/Impl/SessionImpl.cs

-				finally
-				{
-					dontFlushFromFind--;
-					AfterOperation(success);
 				}


Same as previous AfterOperation case.

fredericDelaporte · 2017-09-04T16:17:07Z

src/NHibernate/Impl/SessionImpl.cs

+					finally
+					{
+						AfterOperation(success);
+					}
 				}


Same as previous AfterOperation case.

BhaaLseN

I backported the changes to 4.1.1.GA and it worked just fine with our old bugged code; so I believe the changes are good...at least for what we use it for.

BhaaLseN · 2017-09-05T05:57:59Z

src/NHibernate/Impl/AbstractSessionImpl.cs

+		/// <inheritdoc/>
+		public IDisposable SuspendAutoFlush()
+		{
+			_suspendAutoFlushCount++;


Any thoughts on putting this in the SuspendAutoFlushHelper ctor? IMO it makes it more obvious that the operation is (supposed to be) symmetric, plus you cannot forget one half if you need to reuse that class elsewhere (although unlikely, given it's private)

Also, thoughts on using Interlocked.Increment instead for atomicity?

Make sens moving to constructor. But as sessions are inherently non-thread safe and so are supposed to be used only by one thread at a time, I will not introduce a thread safety guard here.

hazzik · 2017-09-05T22:52:05Z

I think it would make sense to move SuspendAutoFlush to IEventSource interface and the implementation into SessionImpl. As none of these matters for stateless sessions

fredericDelaporte · 2017-09-05T22:55:01Z

So, moving by the way the AutoFlushSuspended property (previously named DontFlushFromFind) too.

hazzik · 2017-09-05T22:55:29Z

So, moving by the way the AutoFlushSuspended

Yes, I meant to write it, but forgot

hazzik · 2017-09-05T23:01:54Z

src/NHibernate/Impl/AbstractSessionImpl.cs

+			#region IDisposable Support
+			private bool _disposedValue; // To detect redundant calls
+
+			protected virtual void Dispose(bool disposing)


This class is private, so I dont see the reasons to have virtual members and/or implement Disposable "correctly"

Default standard snippet from VS, including the anti-redundant call (which is a must for this case).

And well, we could consider than when disposing is false (meaning being called by GC, explicit dispose lacking) we should left the counter incremented for avoiding "fixing" the lack of dispose call through GC and hiding somewhat it.

Now moved to SessionImpl, pointless virtual removed.

But, then we miss finalizer, which should call Dispose(false) then

No more needed since Fx 2 I believe, handled by GC without any finalizer. The fact that SessionImpl still does that is an old remnant of Fx 1.

Current recommendations are explicit about not needing a finalizer in almost all cases nowaday.

But anyway no one is going to call Dispose(false), so I don't think we need to follow this obsolete pattern. Also, I don't think that framework will call Dispose(false) or Dispose() on the finalization stage. We need to prevent calling Dispose() twice, but anyway, without this check we'll get NullReferenceException as session would be unset by the time of second time Dispose() is called. Which, I believe, is clearer than allowing Dispose() to be called twice with no op on the second call.

From where is the protected virtual Dispose(boolean disposing) pattern said to be obsolete? Only implementing explicitly a finalizer is obsolete for most cases. This doc seems quite current.

Now for such a little class it should not ever have consequences, but I just rather stick to recommended patterns when they are strongly recommended as in previously linked documentation.

I don't see where it says that finalize is optional:

What I see is:

You must override Finalize to ensure that unmanaged resources are disposed of if your System.IDisposable.Dispose implementation is not called by a consumer of your type.

But again, we implement the IDisposable interface not for it's purpose, but because of convenience of using.

The fact is my implementation is invalid for the disposing pattern because I should not touch the managed session when disposing is false: if both were dereferenced, the session could have been GC already and should no more be used in any way.

I am probably wrong by telling the GC call disposing(false) itself, it is likely only useful if some later inheritor decide to implement its own finalizer though this should no more be the way to go (use SafeHandle instead for not having to handle directly the releasing of unmanaged resources).

But well, following the recommended practice just avoids having to think about such details while not being a significant overhead.

This is the recommended variation and doesn't require overriding the System.Object.Finalize method.

This is in the first point above. The "must" is in the second way of implementing dispose, while the recommended way is the first.

hazzik · 2017-09-05T23:36:40Z

I would implement the helper as following:

class SuspendAutoFlushHelper : IDisposable
{
	SessionImpl _session;
	public SuspendAutoFlushHelper(SessionImpl session)
	{
		_session = session;
		_session._suspendAutoFlushCount++;
	}

	public void Dispose()
	{
		if (_session == null)
			throw new ObjectDisposedException("The SuspendAutoFlushHelper has been disposed already");

		_session._suspendAutoFlushCount--;
		_session = null;
	}
}

This just fit for purpose. The ObjectDisposedException has been just added for those who do not like NullReferenceException in general.

The reason for implementing it like this, because a) we just abusing the using(IDisposable) feature for our mercantile needs, and b) this object is not truly disposable as we do not have any resources to dispose.

hazzik · 2017-09-05T23:51:18Z

Also, there is no point of protected Dispose(bool) as the class is private and don't have any inheritors, so, make it private, and we're good to go.

fredericDelaporte · 2017-09-05T23:59:44Z

I not really agree with the ObjectDisposedException. An usual dispose should avoid throwing (although some well known classes depart from that, like TransactionScope). This exception is for other methods of the class. The dispose itself should just silently do nothing when called multiple times.

Now for this hijacked dispose, ok, I will code it your way.

(Note: private is not enough, only sealed is. We can derived from a private nested class within the same containing class.)

fredericDelaporte · 2017-09-06T10:02:31Z

Thanks for reviewing,

I have seen in some previous merge to master than updating the branch (merging from master) then merging back to master was causing some noise like here for the test warnings. (Which actual commits are to be found below on 9 august.)

So I am going to rebase instead, discarding the review by the way.

hazzik · 2017-09-06T10:03:30Z

np

…

On Wed, 6 Sep 2017 at 10:02 PM, Frédéric Delaporte ***@***.***> wrote: Thanks for the reviewing, I have seen in some previous merge to master than updating the branch (merging from master) then merging back to master was causing some noise like here <https://github.com/nhibernate/nhibernate-core/commits/master?after=5d8219b39c3f904ee0b3a04e6b692c1bbd478270+30> for the tests warning. So I am going to rebase instead, discarding the review by the way. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#682 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI1l6xJ7vaIFVMxWZPQ_wgiFiz11978ks5sfm24gaJpZM4PL_0g> .

both fixtures use an evil PostXxxListener to cause/force an auto-flush of the session that is currently being committed inside a transaction. for PostInsertFixture it leads to to duplicate inserts. for PostUpdateFixture it causes an ArgumentOutOfRange exception by clearing a list that is currently being iterated.

…h NH-4077.

fredericDelaporte commented Sep 4, 2017

View reviewed changes

hazzik added the t: Bug label Sep 5, 2017

BhaaLseN reviewed Sep 5, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4077-Master branch 2 times, most recently from 820b080 to b4eaab9 Compare September 5, 2017 09:24

hazzik reviewed Sep 5, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4077-Master branch from b4eaab9 to f13afb1 Compare September 5, 2017 23:14

fredericDelaporte force-pushed the NH-4077-Master branch 2 times, most recently from 1ec68ca to aaee9d9 Compare September 6, 2017 00:05

hazzik previously approved these changes Sep 6, 2017

View reviewed changes

BhaaLseN and others added 4 commits September 6, 2017 12:09

NH-4077 - fix for auto-flush triggered from flush events.

a2f3fba

NH-2322 - reverting old fix which was just a crutch, now fixed throug…

8604a4c

…h NH-4077.

NH-4077 - Do not trigger flush during auto-flush either.

0cba4e0

fredericDelaporte dismissed hazzik’s stale review via 0cba4e0 September 6, 2017 10:11

fredericDelaporte force-pushed the NH-4077-Master branch from aaee9d9 to 0cba4e0 Compare September 6, 2017 10:11

fredericDelaporte merged commit 567ffe8 into nhibernate:master Sep 6, 2017

fredericDelaporte deleted the NH-4077-Master branch September 6, 2017 15:12

fredericDelaporte added the r: Fixed label Sep 6, 2017

fredericDelaporte added this to the 5.0 milestone Sep 6, 2017

fredericDelaporte mentioned this pull request Nov 28, 2018

Collection was modified; enumeration operation may not execute, Bulk insert with Post Events #1914

Closed

NH-4077 - do not auto-flush while already flushing #682

NH-4077 - do not auto-flush while already flushing #682

Uh oh!

Conversation

fredericDelaporte commented Sep 4, 2017

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!

BhaaLseN left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Sep 5, 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 Sep 5, 2017

Uh oh!

fredericDelaporte commented Sep 5, 2017

Uh oh!

hazzik commented Sep 5, 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!

hazzik Sep 5, 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 Sep 5, 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 Sep 5, 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!

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 Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Sep 5, 2017

Uh oh!

fredericDelaporte commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Sep 6, 2017 via email

Uh oh!

Uh oh!

fredericDelaporte Sep 5, 2017 •

edited

Loading

hazzik commented Sep 5, 2017 •

edited

Loading

hazzik Sep 5, 2017 •

edited

Loading

fredericDelaporte Sep 5, 2017 •

edited

Loading

hazzik Sep 5, 2017 •

edited

Loading

hazzik commented Sep 5, 2017 •

edited

Loading

fredericDelaporte commented Sep 5, 2017 •

edited

Loading

fredericDelaporte commented Sep 6, 2017 •

edited

Loading