NH-4034 - flushing sessions sharing transaction on commit. #648

fredericDelaporte · 2017-06-20T14:56:19Z

NH-4034 - Flush all sessions participating in a transaction

With NH-4003, a transaction sharing mechanism between sessions has been introduced. It works by opening new sessions from another one, while using the Connection() option on the session builder.

When committing the NHibernate transaction with FlushMode.Commit, only the originating session get flushed. The other sessions should be flushed too.

fredericDelaporte · 2017-06-20T14:56:43Z

src/NHibernate.Test/DebugSessionFactory.cs

+				allClosed = CheckSessionWasClosed(session) && allClosed;
+				// Catches only session opened from another one while sharing the connection. Those
+				// opened without sharing the connection stay un-monitored.
+				foreach (var sharingSession in session.ConnectionManager.SessionsSharingManager)


Small "bonus" by the way: now tests will also track session opened from another one while sharing the connection.

fredericDelaporte · 2017-06-20T14:58:31Z

src/NHibernate.Test/SystemTransactions/TransactionFixture.cs

 	{
-		protected override IList Mappings
-		{
-			get { return new string[] { "WZ.hbm.xml" }; }


Using a locally defined class for having more freedom changing it for the needs of the tests.

fredericDelaporte · 2017-06-20T14:59:46Z

src/NHibernate.Test/SystemTransactions/TransactionFixture.cs

 				Assert.IsNotNull(w);
 				session.Delete(w);
 				tx.Complete();
 			}
 		}
+
+		[Test]
+		public void FlushFromTransactionAppliesToDisposedSharingSession()


I still think we should no more support that pattern someday, but since it is currently supported, better test it.

fredericDelaporte · 2017-06-20T15:01:10Z

src/NHibernate.Test/TransactionTest/TransactionFixture.cs

-		protected override IList Mappings
-		{
-			// The mapping is only actually needed in one test
-			get { return new string[] {"Simple.hbm.xml"}; }


This Simple entity is anything but simple. It does not have a mapped id property, just a column in database, causing weird patterns when actually willing to use it.

fredericDelaporte · 2017-06-20T15:04:18Z

src/NHibernate.Test/TransactionTest/TransactionFixtureBase.cs

+			using (var s = OpenSession())
+			{
+				s.CreateQuery("delete from System.Object").ExecuteUpdate();
+			}


I wonder whether such Teardown should be directly added in TestCase. It would catch all cases not requiring ordering for deletes, and could still be overridden by test needing a specific delete ordering.

fredericDelaporte · 2017-06-20T15:07:06Z

src/NHibernate/ISharedSessionBuilder.cs

@@ -9,6 +9,8 @@ public interface ISharedSessionBuilder : ISessionBuilder<ISharedSessionBuilder>
 	{
 		/// <summary>
 		/// Signifies that the connection from the original session should be used to create the new session.
+		/// The original session remains responsible for it and its closing will cause sharing sessions to be no
+		/// more usable.


Documenting that pitfall by the way.

fredericDelaporte · 2017-06-20T15:09:25Z

src/NHibernate/Impl/SessionImpl.cs

+			if (!_transactionCoordinatorShared)
+				foreach (var sharingSession in ConnectionManager.SessionsSharingManager)
+				{
+					sharingSession.AfterTransactionCompletion(success, tx);


I could have put that in callers, but then it was duplicated between AdoTransaction and AdoNetWithDistributedTransactionFactory.

fredericDelaporte · 2017-06-20T15:11:18Z

src/NHibernate/Impl/SessionImpl.cs

+				if (!_transactionCoordinatorShared)
+				{
+					connectionManager.AfterTransaction();
+				}


In the transaction scope case, the connection manager was getting notified by each session using it. Better let only the originating session do that. This further justifies the ordering: better notify sharing session then originating session last.

fredericDelaporte · 2017-06-20T15:18:08Z

src/NHibernate/Impl/SessionImpl.cs

+				if (!_transactionCoordinatorShared)
+				{
+					Interceptor.AfterTransactionBegin(tx);
+				}


Bit muddy subject here.

In the case of AdoTransaction, the transaction events were not called on sharing sessions, so this check keeps that behavior for the interceptor.

In the case of transaction scope, each session was still enlisting itself, and was receiving events. So the interceptor was call for each sharing session and for the originating one, excepted for the after transaction event which was already guarded for being fired only from originating session... I have decided to align on that behavior for all events.

Now, why not firing the call on the sharing session interceptor? Maybe because by default, it is the same than the originating session. So it would get notified multiples times, likely causing issues. Maybe we should check if it is the same, and if not, then firing the events. Going to add that change.

fredericDelaporte · 2017-06-20T15:23:16Z

src/NHibernate/Transaction/AdoNetWithDistributedTransactionFactory.cs

+			if (originatingSession.TransactionContext != null)
+				return;
+
+			session = originatingSession;


Previous logic was causing each sharing session to enlist its own resource within the transaction. Then each was having its own prepare/transaction complete/... In non distributed case, that was working quite well, because it appears all those volatile resources are called in turn according to their enlistment order.

But with distributed transaction, they would have been called asynchronously, and then what a mess...

So I have changed the logic for having only the originating session enlisting itself.

hazzik · 2017-06-20T20:15:08Z

src/NHibernate.Test/TransactionTest/Person.cs

+{
+	public class Person
+	{
+        public virtual int Id { get; set; }


White space

hazzik · 2017-06-20T20:17:45Z

src/NHibernate/AdoNet/ConnectionManager.cs

+
+		public void RemoveSessionSharingManager(ISessionImplementor session)
+		{
+			_sessionsSharingManager.Remove(session);


LinkedList would be enough.

hazzik · 2017-06-20T20:22:20Z

src/NHibernate/AdoNet/ConnectionManager.cs

@@ -41,6 +42,10 @@ public interface Callback
 		private readonly ISessionImplementor session;
 		private readonly ConnectionReleaseMode connectionReleaseMode;
 		private readonly IInterceptor interceptor;
+		private readonly List<ISessionImplementor> _sessionsSharingManager = new List<ISessionImplementor>();


Can this be renamed to something more obvious? I don't understand who is managing whom from the name.

Granted... I have some difficulties with that name.

hazzik · 2017-06-20T20:44:54Z

src/NHibernate/Transaction/AdoTransaction.cs

@@ -186,6 +186,13 @@ public void Commit()

 				log.Debug("Start Commit");

+				foreach (var sharingSession in session.ConnectionManager.SessionsSharingManager)


Maybe move list of session directly to the transaction?

It serves for transaction scope too. It would be strange to involve the AdoTransaction for transaction scope handling. And at each transaction renewal, we would have to pass that list from the old to the new.

I am renaming that list DependentSessions.

fredericDelaporte · 2017-06-20T21:00:23Z

src/NHibernate.Test/TransactionTest/Person.hbm.xml

+		</id>
+    <property name="CreatedAt"/>
+    <property name="NotNullData"/> <!-- NOT NULL will be set on created schema directly to avoid NH checks. -->
+    <many-to-one name="Related" class="Person" />


white spaces still^^

hazzik · 2017-06-20T21:02:27Z

src/NHibernate/AdoNet/ConnectionManager.cs

@@ -63,6 +75,16 @@ public interface Callback
 			ownConnection = suppliedConnection == null;
 		}

+		public void AddSessionSharingManager(ISessionImplementor session)


The method name shall be renamed as well ;)

hazzik · 2017-06-20T21:03:38Z

Can you test a case with a grandchild session (a child session opened from a child session)?

hazzik · 2017-06-20T22:26:58Z

I did not run the code myself, so I would like to know: in the case of grand-child session who is the main session? Is it immediate parent or a grand-parent?

hazzik · 2017-06-20T22:50:56Z

src/NHibernate/Transaction/AdoTransaction.cs

@@ -186,6 +186,13 @@ public void Commit()

 				log.Debug("Start Commit");

+				foreach (var dependentSession in session.ConnectionManager.DependentSessions)


Shouldn't this be reversed?

Reverted? No, that is precisely the main point of this PR.
Reversed order? I do not get why.

fredericDelaporte · 2017-06-20T22:51:12Z

The main session is always the one responsible for the connection manager. The one which has created it itself instead of getting it from its constructor options.

There is actually no such things as "grand children", their are just all dependent sessions if they have taken their connection manager from "outside". Otherwise their are just sessions, without any dependency or relationship with the one having spawn them.

A "grand children" parent could even be closed without this affecting the grand children: only the session responsible for the connection manager must ot be closed before dependent sessions.

hazzik · 2017-06-20T22:55:35Z

I think the flush order shall be reverse of open order (LIFO), this would make much more sense (for me at least). This way you can, probably, use only one stack and eliminate the "Session" ("parent" session) property on ConnectionManager

hazzik · 2017-06-20T23:49:21Z

src/NHibernate/Impl/SessionImpl.cs

@@ -2092,17 +2104,33 @@ public override void AfterTransactionBegin(ITransaction tx)
 			using (new SessionIdLoggingContext(SessionId))
 			{
 				CheckAndUpdateSessionStatus();
-				Interceptor.AfterTransactionBegin(tx);
+
+				if (!_transactionCoordinatorShared || Interceptor != ConnectionManager.Session.Interceptor)


This condition is repeated several times, I think it shall be moved to a method.

hazzik · 2017-06-20T23:49:56Z

src/NHibernate/Impl/SessionImpl.cs

@@ -2092,17 +2104,33 @@ public override void AfterTransactionBegin(ITransaction tx)
 			using (new SessionIdLoggingContext(SessionId))
 			{
 				CheckAndUpdateSessionStatus();
-				Interceptor.AfterTransactionBegin(tx);
+
+				if (!_transactionCoordinatorShared || Interceptor != ConnectionManager.Session.Interceptor)


~~Why you do not call interceptors for "child" sessions? I think it is perfectly valid scenario.~~ Ok, I've misread the code, please ignore this comment.

I do call it for "children", excepted if that is the same interceptor than the originating session. It does not make sens to call it for each children session in such case, since the originating session will anyway call it, and nothing allow the interceptor to understand why it has many "begin tran" called: it does not know which session has called that.

Previous code was never calling for "children" session, either because the session event was not call at all, or because a test on _transactionCoordinatorShared was already there). I have adjusted that for always calling dependent session transaction events and also calling the interceptor only if the dependent session has another interceptor than the originating session.

"Child session" not sharing the connection will have independent transaction handling and will call their interceptor in all cases. Their are just usual sessions indeed, without any knowledge of having been spawned from another session.

Ok, I gave it another though. I think that this code makes ISharedSessionBuilder.Interceptor() to have no sense at all, as shared interceptor would never be called.

Interceptor is not here only for transaction. All the other cases are still called. But when the transaction is shared, does it make sens to notifies the same interceptor multiple time of the same transaction beginning? If the session was supplied as an argument, why not, but that is not the case.

fredericDelaporte · 2017-06-20T23:51:38Z

Flushing in reverse order is not very intuitive for the user I think. It is likely users will expect the dependent sessions to be flush in their opening order.

I am a bit reluctant to put the session owning the connection manager in the same list of others by the way. It really has a special role. I am not willing to rely on the list ordering for acting on that particular session at the right moment.

I do not see this list as a stack, as if there were a sort of nested transaction concept. Those dependent sessions are more a heap of things acting independently, eventually overlapping their usage. That is way I handle them FIFO.

hazzik · 2017-06-21T00:10:46Z

I don't see the original session as an "the special one". It was just the first one. So, I believe, it shall be a stack. The "speciality" of the session is that it causes the "dependent" session to do the same, but I think it is wrong. This causality shall happen in a connection manager itself (or, some other class).

So, for example, calls of session.BeforeTransactionCompletion shall be updated to calls to connectionManager.BeforeTransactionCompletion, which would be implemented as:

foreach(var session in this.sessions) { session.BeforeTransactionCompletion(...); }

hazzik · 2017-06-21T00:31:02Z

Can we port these tests? https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/test/java/org/hibernate/sharedSession/SessionWithSharedConnectionTest.java

hazzik · 2017-06-21T00:36:06Z

Some reading: https://hibernate.atlassian.net/browse/HHH-7273

Related: https://hibernate.atlassian.net/browse/HHH-7017

fredericDelaporte · 2017-06-21T00:46:03Z

Yes I thought about that job would be better centralized somewhere else, but only the connection manager seems currently available for taking this. That would add to it a bunch of new methods, while changing the semantics of other: it already has its own AfterTransaction, it would to be merged with the after transaction notifying the session.

But I still disagree for the rest.

I don't see the original session as an "the special one"

This one still own the connection manager. Only this one will close it, regardless of whether spawned session are still around, un-closed, or not. Maybe do you wish to change that. I have just stuck to Hibernate implementation here. That would mean something like, any call to RemoveDependentSession causing the list to get empty would then dispose the connection manager. No more needs to track the sharing, provided all the notification handling has been moved into connection manager.

I believe, it shall be a stack.

I really am unable to understand what is the benefit of working with dependent sessions as a stack: it really looks counter-intuitive for me, and from a user point of view. Those sessions do not care of each others, excepted they will start failing if the connection manager owning one is closed. Why should we ensure they work as if they were a stack? Moreover, in term of lifecycle, the user is not at all mandated to work with them in LIFO, closing first the last opened. It is even likely that in many cases, they will use them in FIFO mode, spawning one dependent session, working with it, disposing of it, then spawning another one, ...

Flushing them in reverse order just look to me as the path for maximizing troubles. I think it is way more likely that the later sessions work ends up having some dependency on the previous sessions work, than the opposite.

(I have not yet read your links, it is almost 3 in the morning here.)

hazzik · 2017-06-21T00:50:02Z

(I have not yet read your links, it is almost 3 in the morning here.)

Get some sleep:) Maybe in the morning you'll agree with me.

We, probably, shall not implement this feature at all.

hazzik · 2017-06-21T00:51:24Z

We need to port this feature first: hibernate/hibernate-orm@b476094

fredericDelaporte · 2017-06-21T10:53:23Z

A 192 files change without much explanations in the ticket. Here we are only on a 12 files change, for fixing a specific issue.

I still do not understand why it would be worst to do the changes I propose rather than to stay in current situation. Especially I am still completely unable to understand why should it be treated as a stack, even by reading the above links, which I am unable to relate to the subject. Why not porting the tests, but they almost all rely on a non ported feature: autoclose. So it may not make much sens. I will probably have a look at what impact it has to move the transaction event raising into the connection manager.

The links chain to a bunch of issues which I believe NHibernate does not suffer or do not care at its current state. Much of those issues are due to complex interactions between all the many components they currently use to get the session working: JdbcCoordinator (NHibernate ConnectionManager), TransactionCoordinator (which NHibernate does not actually have, its transaction factory and transaction context do not do the same job), LogicalConnection (merged in ConnectionManager for NHibernate).

One of their issues is to have the session holding the transaction coordinator itself holding the jdbc coordinator, and that creates them issues for allowing concurrent usage of sessions sharing the same transaction coordinator, because connection does not allow multi-threaded usage. So they want to invert that relationship, having the session holding the jdbc coordinator which in turn hold the transaction coordinator.
Fine, NHibernate is somewhat already in that state, session holds directly the connection manager. (There is no transaction coordinator.) And since currently NHibernate explicitly shares the connection, not the transaction, it looks like a no-brainer that session sharing the same connection should not be used concurrently (by different threads), and that NHibernate does not support it.

They have others issues, like connection leaks when sharing (with current NHibernate model, I do not see how that could be happening, unless not closing the "originating" session, and if I move all to connection manager, unless not closing all sessions, or using them on different threads), like "originating" session getting closed by spawned sessions own close (due to the shared transaction coordinator which, when closed, closes its "main session", behavior we do not have currently in NHibernate), ...

It looks to me that porting all their stuff is likely to creates way more issues than what we actually have now, all that for maybe supporting a scenario I have seen asked by no-one, allowing concurrent (multi-threaded) usage of sessions sharing the same transaction. Anyway DbTransaction is not thread safe... We will not be able to implement a transaction coordinator supporting multi-threaded usage unless mandating use of system transactions instead of ado transaction.

Nowhere I have understood what it was having to do with considering spawned sessions as a stack of sessions.

fredericDelaporte · 2017-06-21T11:05:36Z

I had forgotten, they also have issues with "temporary sessions". NHibernate does not have these currently. Maybe those one need to be treated in a stacked way, but they appear to me as really another beast that this feature of spawning sessions sharing the same connection.

The envers case maybe desserve a stacked treatment, but are those spawned sessions the right tool for it? (And well, stacked treatment for envers sound strange for me: flushing logs before the actual changes they correspond too?)

hazzik · 2017-06-21T11:10:55Z

Ok, let's forget about the stacked sessions, but why the "parent" session needs to be flushed last? For me, the order either does not matter or otherwise, all sessions shall be flushed in the opposite order of their opening.

So, for example, we have sessions with ids 0, 1, 2. For me, as a user, it would be extremely weird that sessions were flushing in 1 > 2 > 0 order. I would expect either 0 > 1 > 2 or 2 > 1 > 0.

fredericDelaporte · 2017-06-21T11:18:23Z

Well, I am quite agree with you on that later point, if we change the implementation for having the originating session to really be just another session, no more exclusively responsible for the connection manager. So I definitely need to move those transaction events raising into the connection manager, then handle its disposal at last session removal instead of at originating session closing.

hazzik · 2017-06-21T11:53:26Z

Ok, I've finally found how Hibernate does flush with shared transaction coordinator. ~~There is indeed no "primary"/"parent" session.~~ They handle multiple transactions flushes by "ISomethingObserver". (It looks the same as ISynchronization (but they also have ISynchronization). There is an anonymous observer class which calls beforeTransactionCompletion and afterTransactionCompletion. Flush is called from flushBeforeTransactionCompletion, which is called from beforeTransactionCompletion.

hazzik · 2017-06-21T11:55:53Z

The observer is added from a session constructor.

hazzik · 2017-06-21T12:00:43Z

Ok, I was wrong. There IS a "parent" session. But, it is called first.

private void beforeCompletionCallback() {
	log.trace( "ResourceLocalTransactionCoordinatorImpl#beforeCompletionCallback" );
	try {
		transactionCoordinatorOwner.beforeTransactionCompletion(); // this is "parent" session
		synchronizationRegistry.notifySynchronizationsBeforeTransactionCompletion();
		for ( TransactionObserver observer : observers ) { // this is for "dependant" sessions
			observer.beforeCompletion();
		}
	}
	catch (RuntimeException e) {
		if ( physicalTransactionDelegate != null ) {
			// should never happen that the physicalTransactionDelegate is null, but to be safe
			physicalTransactionDelegate.markRollbackOnly();
		}
		throw e;
	}
}

fredericDelaporte · 2017-06-21T12:07:31Z

Yes I have already seen those observers in their code, without checking all their responsibilities. Hibernate is definitely way more "meat" heavy than NHibernate currently is. Maybe I am slightly biased toward Java, but I sometime fear they fell in the hammer factory factory trap.

So on Hibernate side, they still have the "originating" session treated as a special one. I am not sure this is very desirable.

hazzik · 2017-06-21T12:10:43Z

They do not restrict calling interceptors from "child" sessions. This is how the observer is implemented:

@Override
protected void addSharedSessionTransactionObserver(TransactionCoordinator transactionCoordinator) {
	transactionCoordinator.addObserver(
			new TransactionObserver() {
				@Override
				public void afterBegin() {
				}

				@Override
				public void beforeCompletion() {
// Hazzik: this is strange, they do not just call "beforeTransactionCompletion" for some reasons.
					if ( isOpen() && getHibernateFlushMode() !=  FlushMode.MANUAL ) {
						managedFlush();
					}
					actionQueue.beforeTransactionCompletion();
					try {
						getInterceptor().beforeTransactionCompletion( getCurrentTransaction() );
					}
					catch (Throwable t) {
						log.exceptionInBeforeTransactionCompletionInterceptor( t );
					}
				}

				@Override
				public void afterCompletion(boolean successful, boolean delayed) {
					afterTransactionCompletion( successful, delayed );
					if ( !isClosed() && autoClose ) {
						managedClose();
					}
				}
			}
	);
}

fredericDelaporte · 2017-06-21T12:15:19Z

What is your comment in this code about? Do you think before completion should be called before flush on commit? NHibernate too, does flush before the "before completion" event for ado transaction. (But currently not for system transaction, which is an oddity to have it different, and I believe I have changed it in #627.)

hazzik · 2017-06-21T12:17:13Z

Observer's beforeCompletion looks the same as SessionImpl's beforeTransactionCompletion. The only difference that in former one they do not check if the transaction state (not sure if this is an optimization, or not.) and in later one they check that session is opened

hazzik · 2017-06-21T12:18:23Z

@Override
public void beforeTransactionCompletion() {
	log.tracef( "SessionImpl#beforeTransactionCompletion()" );
	flushBeforeTransactionCompletion();
	actionQueue.beforeTransactionCompletion();
	try {
		getInterceptor().beforeTransactionCompletion( getCurrentTransaction() );
	}
	catch (Throwable t) {
		log.exceptionInBeforeTransactionCompletionInterceptor( t );
	}
}

@Override
public void flushBeforeTransactionCompletion() {
	final boolean doFlush = isTransactionFlushable()
			&& getHibernateFlushMode() != FlushMode.MANUAL;

	try {
		if ( doFlush ) {
			managedFlush();
		}
	}
	catch (RuntimeException re) {
		throw exceptionMapper.mapManagedFlushFailure( "error during managed flush", re, this );
	}
}

private boolean isTransactionFlushable() {
	if ( getCurrentTransaction() == null ) {
		// assume it is flushable - CMT, auto-commit, etc
		return true;
	}
	final TransactionStatus status = getCurrentTransaction().getStatus();
	return status == TransactionStatus.ACTIVE || status == TransactionStatus.COMMITTING;
}

fredericDelaporte · 2017-06-21T12:34:48Z

Ok, looks like either they have dead code, or more likely different code paths depending on whether the transaction is JTA, "native jdbc" (no more sure of the naming), or yet something else (JPA brings its own transaction handling too, or am I wrong?).

hazzik · 2017-06-21T12:37:33Z

No, it's not dead code, it's called from different paths.

hazzik · 2017-06-21T12:40:19Z

Our AdoTransaction is their JdbcResourceLocalTransactionCoordinatorImpl + TransactionDriverControlImpl

Our on commit (including this PR):

flush "children" sessions
flush "parent" session"
notify synchronizations
beginTransactionCompletion of "parent" session
- beginTransactionCompletion of "children" sessions
- actionQueue.beforeTranactionCompletion
- interceptor.beforeTransactionCompletion
commit
after completion (did not check yet)

Their flow:

"parent" session beforeTransactionCompletion
- flush
- action queue
- interceptor
notify synchronizations
observer's beforeCompletion ("children" sessions")
- flush
- action queue
- interceptor
commit
after completion

hazzik · 2017-06-21T12:47:29Z

Hibernate does not call* AfterTransactionBegin for "children" sessions.

*It calls Observer's afterBegin, which is noop.

hazzik · 2017-06-21T13:08:48Z

@fredericDelaporte I'll try to make changes which resemble the current Hibernate state (I'll try to avoid hammer factory) and will make a PR to your branch shortly.

fredericDelaporte · 2017-06-21T14:19:31Z

By the way, I do not see that as a priority. I was doing it for removing that lack of flush from transaction commit, believing it could be done with few code changes. It could stay lacking otherwise. After all there is no regression here, compared to 4.1.

I am not very keen on replicating Hibernate implementation when its rational are not clear to me. It could be that their choices make sens in a Java context, but less in a .Net one.

hazzik · 2017-06-21T14:29:53Z

rational are not clear to me

It seems that sole purpose of TransactionCoordinator is to manage the flushes on transaction commit

hazzik · 2017-06-23T11:43:35Z

src/NHibernate.Test/SystemTransactions/TransactionFixture.cs

+			{
+				var builder = s.SessionWithOptions().Connection();
+
+				using (var t = new TransactionScope())


I think these tests are not valid.

http://nhibernate.info/doc/nhibernate-reference/transactions.html

11.2. Threads and connections

You should observe the following practices when creating NHibernate Sessions:

Never create more than one concurrent ISession or ITransaction instance per database connection.

Be extremely careful when creating more than one ISession per database per transaction. The ISession itself keeps track of updates made to loaded objects, so a different ISession might see stale data.

The ISession is not threadsafe! Never access the same ISession in two concurrent threads. An ISession is usually only a single unit-of-work!

At least it is not valid to have an entity graph with entities from different sessions. The valid approach testing that flush has happened would be to supply a listener or an interceptor.

Never create more than one concurrent ISession or ITransaction instance per database connection.

A bit contradictory with the connection sharing option indeed. By the way, previously was doable only by supplying a user connection I guess.

About having an entity graph from different sessions, yes that does not look as a good practice, I will change that.

hazzik · 2017-06-23T11:43:50Z

src/NHibernate.Test/SystemTransactions/TransactionFixture.cs

+				using (var s1 = builder.OpenSession())
+				using (var s2 = builder.OpenSession())
+				using (var s3 = s2.SessionWithOptions().Connection().OpenSession())
+				using (var t = new TransactionScope())


Same as above

hazzik · 2017-06-23T11:44:05Z

src/NHibernate.Test/TransactionTest/TransactionFixture.cs

+				using (var s1 = builder.OpenSession())
+				using (var s2 = builder.OpenSession())
+				using (var s3 = s1.SessionWithOptions().Connection().OpenSession())
+				using (var t = s.BeginTransaction())


Same as above

* Hibernate calls statistics between action queue and interceptor * Hibernate calls children AfterCompletion at the end. * Hibernate calls ConnectionManager.AfterTransaction outside of a SessionImpl.AfterTransactionCompletion * Call dependant sessions AfterTransactionCompletion * Flush dependent sessions * Hibernate does not call AfterTransactionBegin for dependent transactions (but should) * Hibernate does not restrict calling interceptors on dependant sessions * Move flush into beforeTransactionCompetion method * Add call to AfterTransactionBegin on dependent sessions * Move using(sessionImplementor.ConnectionManager.FlushingFromDtcTransaction) outside of a loop.

fredericDelaporte · 2017-06-28T11:17:47Z

src/NHibernate/Impl/AbstractSessionImpl.cs

@@ -430,6 +431,7 @@ protected void AfterOperation(bool success)
 				if (!ConnectionManager.IsInActiveTransaction)
 				{
 					ConnectionManager.AfterNonTransactionalQuery(success);
+					ConnectionManager.AfterTransaction();


Could be moved into ConnectionManager.AfterNonTransactionalQuery. This is not the Hibernate way but well, Hibernate does not have that AfterNonTransactionalQuery, which in NHibernate just do a log currently.

I think it's better to leave it as is.

hazzik

LGTM

If you are going to merge with squash - please make sure that extended message is clean and tidy, and do not have typos.

fredericDelaporte · 2017-06-29T22:02:46Z

Thanks,

I guess you have seen some such typos/misses in my recent previous merges. Sorry to have not spot them, although I have read them.

fredericDelaporte commented Jun 20, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4034 branch 2 times, most recently from 958a1cc to ada8f2e Compare June 20, 2017 17:55

hazzik reviewed Jun 20, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4034 branch from ada8f2e to 8b86bb6 Compare June 20, 2017 20:58

fredericDelaporte commented Jun 20, 2017

View reviewed changes

hazzik reviewed Jun 20, 2017

View reviewed changes

fredericDelaporte added the needs work label Jun 21, 2017

hazzik reviewed Jun 23, 2017

View reviewed changes

fredericDelaporte and others added 2 commits June 27, 2017 17:26

NH-4034 - flushing sessions sharing transaction on commit.

bfbd264

fredericDelaporte force-pushed the NH-4034 branch from 8f23086 to ea9c39a Compare June 27, 2017 15:27

fredericDelaporte removed the needs work label Jun 27, 2017

fredericDelaporte commented Jun 28, 2017

View reviewed changes

fredericDelaporte requested a review from hazzik June 29, 2017 08:42

hazzik approved these changes Jun 29, 2017

View reviewed changes

fredericDelaporte merged commit e8fb9a0 into nhibernate:master Jun 29, 2017

fredericDelaporte deleted the NH-4034 branch June 29, 2017 22:04

hazzik added r: Fixed t: New Feature labels Aug 3, 2017

hazzik modified the milestone: 5.0 Aug 3, 2017

		@@ -186,6 +186,13 @@ public void Commit()

		log.Debug("Start Commit");

		foreach (var sharingSession in session.ConnectionManager.SessionsSharingManager)

		@@ -186,6 +186,13 @@ public void Commit()

		log.Debug("Start Commit");

		foreach (var dependentSession in session.ConnectionManager.DependentSessions)

NH-4034 - flushing sessions sharing transaction on commit. #648

NH-4034 - flushing sessions sharing transaction on commit. #648

Uh oh!

Conversation

fredericDelaporte commented Jun 20, 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!

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 Jun 20, 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 Jun 20, 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!

hazzik commented Jun 20, 2017

Uh oh!

hazzik commented Jun 20, 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!

fredericDelaporte commented Jun 20, 2017

Uh oh!

hazzik commented Jun 20, 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!

hazzik Jun 20, 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!

fredericDelaporte commented Jun 20, 2017

Uh oh!

hazzik commented Jun 21, 2017

Uh oh!

hazzik commented Jun 21, 2017

Uh oh!

hazzik Jun 20, 2017 •

edited

Loading

fredericDelaporte Jun 20, 2017 •

edited

Loading

hazzik commented Jun 20, 2017 •

edited

Loading

hazzik commented Jun 20, 2017 •

edited

Loading

hazzik Jun 20, 2017 •

edited

Loading

hazzik commented Jun 21, 2017 •

edited

Loading

fredericDelaporte commented Jun 21, 2017 •

edited

Loading

hazzik commented Jun 21, 2017 •

edited

Loading

hazzik commented Jun 21, 2017 •

edited

Loading

hazzik commented Jun 21, 2017 •

edited

Loading

hazzik commented Jun 21, 2017 •

edited

Loading

hazzik commented Jun 21, 2017 •

edited

Loading