NH-2176 - Add test case #410

hazzik · 2015-03-04T01:45:58Z

No description provided.

hazzik · 2015-04-14T05:19:15Z

Need to cleanup whitespaces

hazzik · 2017-04-10T23:44:58Z

Related to #532

hazzik · 2017-04-11T04:06:28Z

@fredericDelaporte I need someone with fresh eyes to look into this

fredericDelaporte · 2017-04-14T11:22:20Z

I may be completely wrong, but I fear NHibernate tries to support an invalid programming model about transaction scope currently.

The session detects ambiant transaction and enlist with them.
A transaction scope in which a session is enlisted needs the session for completing the transaction (distributed or not).
If the session is closed (ISession.Close()) before transaction scope disposal, the transaction scope will fail.
What I see as a cursed hack causes ISession.Dispose to not actually dispose the session if it is enlisted with an ongoing transaction scope. It just set a flag for transaction completion to cause the session disposal. This allows "disposing" the session although it participates in a an ongoing transaction scope.

It looks to me that point 4 is an attempt at supporting a model that violates IDisposable pattern. We should not try supporting the disposal of a ressource which is still in use.

If we look at how this is handled in EF 6 (doc updated on October 2016), it dodges the trouble in a quite more valid way:

The context to be disposed inside a transaction scope has to be provided with a SQL connection it will not handle.
The context has to be flushed to db before being disposed.

That way, the context is indeed no more needed by the transaction scope for completing the transaction.

Point 1. is important for allowing other contextes nested inside the transaction scope to access the changes done on the data by a previous context while still in the ongoing transaction scope. (Otherwise they have this issue.)

I think we need to do breaking changes for having a more reasonable implementation:

Cease delaying the session disposal on dispose, even when nested in a transaction scope.
Cease throwing exception in connection manager when closing a connection while in a transaction scope: just pass along the call to the connection provider (or de-reference non-owned connection). Do not dispose the transaction context anymore: move it to transaction completion event.
Mandates session Flush before disposal for changes to be persisted. (It can still be implicit if the transaction is committed before the session disposal; alternative: change FlushMode.Auto for auto flushing on dispose if inside a transaction scope, or add a new mode for that.)
Change the enlistment: if the session has been disposed, reply Done on prepare, no-op on session.

Well ideally, we should still perform post-transaction operations required for things like the second level cache, although the session is disposed. I do not know if those operations can be moved outside session responsibility easily or not.

An additional point could be added for an alternate to point 1. of EF case:

Make connection provider transaction scope aware: it will need to enlist too (at least receive transaction completed event), and will give the same connection to sessions executing in the same scope. It will no more close them if in a transaction scope, but will wait for the transaction scope completion.

fredericDelaporte · 2017-04-23T17:56:50Z

Checking the state of Hibernate, for the JPA case, they do something similar to NHibernate delayed disposal of the session. But for the JDBC and JTA (JTA is what looks closest to TransactionScope) case, they actually close the session, but not the JdbcCoordinator if a shared transaction is ongoing. The connection is handled by the JdbcCoordinator.

I think we should replicate the JDBC handling.

fredericDelaporte · 2017-04-26T09:59:24Z

Still more thoughts on the subject: currently on NHibernate side, it looks like people try to use the old entity mode switching capability (GetSession(EntityMode mode)) for sharing the connection among sessions. And when doing NH-3722 as a follow up to HHH-6330, instead of completely removing the functionality as Hibernate has done, we have just removed the entity mode argument and renamed it GetChildSession().

We are quite diverging from Hibernate by doing so. They do not need this because they have decoupled connexion handling from the session.

The more I look to all that, the more I think we should re-align our session/transaction/connection handling with Hibernate.

hazzik · 2017-04-26T10:08:52Z

I agree

hazzik · 2017-05-17T04:29:19Z

It seems that session.Connection.EnlistTransaction(System.Transactions.Transaction.Current); at Line 33 of AdoNetWithDistributedTransactionFactory fixes this issue.

fredericDelaporte · 2017-05-17T05:24:32Z

It seems that session.Connection.EnlistTransaction(System.Transactions.Transaction.Current); at Line 33 of AdoNetWithDistributedTransactionFactory fixes this issue.

It will very likely fix NH-2176, but it will not fix:

NH-2237 - unlocking of entities occurring while next transaction already started: due to TransactionCompleted executing in DTC cases after transaction scope completion in separated and concurrent thread.
NH-3968 - connection already prepared during session prepare phase: due to trying to re-use the session connection for flushing from session prepare phase while this connection is itself enlisted for its own 2PC in the same transaction.

Unlikely to fix:

NH-2107 - connection not released on rollback. This bug lacks information for me to actually understand how it gets triggered. But I highly suspect this is bounded to the current pattern of delaying session disposal and connection release to transaction completion events.
NH-2238 - prepare phase failure. Again lack of informations, but looks bounded to trying to flush from session prepare phase with a connection already enlisted itself for its own 2PC.
NH-3023 - deadlock causing connection pool corruption: very likely same root cause than NH-2107.
NH-3227 - rollback causing connection pool corruption: almost duplicate of NH-3023.

That is why I consider this change is not enough to fix current approach.

hazzik · 2017-05-17T05:27:06Z

NH-2238 is exactly the same as NH-3968

fredericDelaporte · 2017-05-17T05:27:17Z

(Well, the time for me to consolidate my previous comment, it seems you have met NH-3968. A comment notification on #532 indicates that in my mailbox, but I do not see it online.)

hazzik · 2017-05-17T05:29:01Z

but I do not see it online.

hazzik · 2017-05-17T05:30:46Z

but I do not see it online.

I deleted it along with the changes it was mentioning

fredericDelaporte · 2017-05-17T05:33:53Z

NH-2238 is exactly the same as NH-3968

Having the same root cause, very likely. But the former writes about a closed connection exception while the later is about a timeout trying to use it.

but I do not see it online.

I'd guessed you have deleted it of course, but sometime I prefer tell facts (not seeing it) rather than guesses (being deleted) even when obvious. (And sometime that's Github auto-loading of new comments which does not always trigger when I would have expected.)

fredericDelaporte

The custom transaction factory is in a dead end unless we do some code changes in NHibernate.

fredericDelaporte · 2017-05-17T10:17:35Z

src/NHibernate.Test/NHSpecificTest/DtcFailures/DtcFailuresFixture.cs

@@ -19,6 +19,11 @@ public class DtcFailuresFixture : TestCase
 	{
 		private static readonly ILog log = LogManager.GetLogger(typeof(DtcFailuresFixture));

+		protected override void Configure(Configuration configuration)
+		{
+			configuration.SetProperty(Cfg.Environment.TransactionStrategy, "NHibernate.Test.NHSpecificTest.NH2176.CustomAdoNetTransactionFactory, NHibernate.Test");


Testing the custom factory with DTC testes.

fredericDelaporte · 2017-05-17T10:18:28Z

src/NHibernate.Test/NHSpecificTest/DtcFailures/DtcFailuresFixture.cs

-			if (Dialect is FirebirdDialect)
-				Assert.Ignore("Firebird driver does not support distributed transactions");
+			/*if (Dialect is FirebirdDialect)
+				Assert.Ignore("Firebird driver does not support distributed transactions");*/


In my local test Firebird supports them (at least does not throw), so just letting them run to test with teamcity too.

fredericDelaporte · 2017-05-17T10:22:46Z

src/NHibernate.Test/NHSpecificTest/DtcFailures/DtcFailuresFixture.cs

@@ -113,8 +118,8 @@ public void Can_roll_back_transaction()
 		[Description("Another action inside the transaction do the rollBack outside nh-session-scope.")]
 		public void RollbackOutsideNh()


This one showcase the roadblock for the custom transaction factory: session close within an ongoing transaction scope is rejected by connection manager.

The connection manager close does close any pending explicit transaction, then call disconnect which checks there are no pending transaction, explicit or ambient. The ambient check on disconnect should be removed, ambient transaction supports connection disconnect.

This need changes in NHibernate project even for testing: the connection manager is not pluggable.

fredericDelaporte · 2017-05-17T10:28:28Z

src/NHibernate.Test/NHSpecificTest/NH2176/Fixture.cs

+				// This damn thing may yield an invalid operation exception (instead of null
+				// or of a completed transaction) if we are between scope.Complete() and
+				// scope.Dispose(). This happen when having completed the scope then disposing
+				// the session and only after that disposing the scope.


Another bug fixed in custom transaction factory: accessing Transaction.Current may throw, need to catch.

fredericDelaporte · 2017-05-17T18:25:01Z

Doomed attempt, I have seen locally that the second phase of 2PC can be executed after transaction scope disposal. (Illustrated by NH-2420 test.) I need to do some new edits on NH-4011!

I may still add one more attempt, if I find a way through lock to ensure second phase is done before any new session usage. But it may be a crutch reducing risks but not eliminating them.

…andling.

fredericDelaporte · 2017-05-17T20:48:03Z

src/NHibernate.Test/NHSpecificTest/NH1632/Fixture.cs

+					using (var s = sessions.OpenSession(connection))
+					{
+						Nums nums = null;
+						Assert.DoesNotThrow(() => nums = s.Load<Nums>(29), "Failed loading entity from second level cache.");


Leaving an explicit message instead of "caching failure" => then "failing to tear down", and letting not much clues.

fredericDelaporte · 2017-05-17T20:48:28Z

src/NHibernate.Test/NHSpecificTest/NH1632/Fixture.cs

-
-			using (var s = sessions.OpenSession())
-			using (var tx = s.BeginTransaction())
+			finally


Avoiding additional tear down failure.

fredericDelaporte

That attempt seems now to work completely with SQL Server, even DTC test that was ignored as "not fixed" being green.

But relying on thread synchronization does look good to me. I would rather introduce the new factories I envision in NH-4011.

Of course, need to see if this works with all db still. And then, need to cleanup previous tests (custom factory, commits, ...)

fredericDelaporte · 2017-05-17T23:55:20Z

src/NHibernate.Test/NHSpecificTest/NH2420/Fixture.cs

+			// here. When having TransactionCompleted event, this event and the second phase
+			// tend to occur before reaching here. But some other NH cases demonstrate that
+			// TransactionCompleted may also occur "too late".
+			((ISessionImplementor) s).TransactionContext?.WaitOne();


The test use a session property (IsConnected) that does not sync with Transaction factory. So for the test, I have added an explicit sync call. I was feeling like really required to cause IsConnected to sync.

fredericDelaporte · 2017-05-17T23:56:48Z

src/NHibernate/AdoNet/ConnectionManager.cs

 		public IDisposable FlushingFromDtcTransaction
 		{
 			get
 			{
+				if (ownConnection)


Not trying to handle use supplied connection case. We would have to clone it otherwise.

fredericDelaporte · 2017-05-18T00:15:09Z

src/NHibernate/Transaction/AdoNetWithDistributedTransactionFactory.cs

@@ -24,52 +25,36 @@ public ITransaction CreateTransaction(ISessionImplementor session)

 		public void EnlistInDistributedTransactionIfNeeded(ISessionImplementor session)
 		{
+			// Ensure the session does not run on a thread supposed to be blocked, waiting
+			// for transaction completion.
+			session.TransactionContext?.WaitOne();


Transaction completion may concurrently nullify it, but ?. handles that (correctly evaluate it only once, guaranteeing it will not try to call WaitOne on null).

fredericDelaporte · 2017-05-18T00:19:52Z

src/NHibernate/Transaction/AdoNetWithDistributedTransactionFactory.cs

+
+			private readonly ISessionImplementor _sessionImplementor;
+			private readonly ManualResetEvent _waitEvent = new ManualResetEvent(true);
+			private readonly AsyncLocal<bool> _bypassWait = new AsyncLocal<bool>();


Not already needing async but well, better avoid adding new thread locals.

fredericDelaporte · 2017-05-18T00:26:11Z

src/NHibernate/Transaction/AdoNetWithDistributedTransactionFactory.cs

+					// Dispose releases blocked threads by the way.
+					// Must dispose in case !ShouldCloseSessionOnDistributedTransactionCompleted, since
+					// we nullify session TransactionContext, causing it to have nothing still holding it.
+					Dispose();


This was missing and is a transaction leak in NHibernate 4.1. When scopes are disposed inside session life-span, the TransactionContext was nullified on session without being disposed, causing the AmbientTransaction clone to not be disposed of explicitly.

fredericDelaporte · 2017-05-18T00:28:26Z

src/NHibernate/Transaction/AdoNetWithDistributedTransactionFactory.cs

+				{
+					_logger.Warn(
+						"Synchronization failure, assuming it has been concurrently disposed and do not need sync anymore.",
+						ex);


I have not witnessed this, but it may happen if between the test on _isDisposed and the call to WaitOne, the transaction completion event dispose the wait event.

fredericDelaporte · 2017-05-18T00:30:47Z

src/NHibernate/Transaction/AdoNetWithDistributedTransactionFactory.cs

+						AmbientTransation = null;
+					}
+					_waitEvent.Set();
+					_waitEvent.Dispose();


In my tests, disposing right after signaling does not cause an exception for thread potentially waiting. (I have run all test first without the catch around WaitOne, and do not got any failures, all tests passed.)

hazzik · 2017-05-18T01:03:14Z

src/NHibernate.Test/NHSpecificTest/NH2420/Fixture.cs

+			// here. When having TransactionCompleted event, this event and the second phase
+			// tend to occur before reaching here. But some other NH cases demonstrate that
+			// TransactionCompleted may also occur "too late".
+			((ISessionImplementor) s).TransactionContext?.WaitOne();


shall be s.GetSessionImplementor()

Probably not the first time... Maybe should we remove all the other alike catches which still liter the tests.

hazzik · 2017-05-18T01:07:40Z

src/NHibernate/Transaction/AdoNetWithDistributedTransactionFactory.cs

-			void IEnlistmentNotification.Rollback(Enlistment enlistment)
+			#endregion
+
+			public void TransactionCompleted(object sender, TransactionEventArgs e)


TransactionCompleted shall be incorporated to IEnlistmentNotification.Commit/IEnlistmentNotification.Rollback/IEnlistmentNotification.InDoubt

hazzik · 2017-05-18T01:08:44Z

src/NHibernate/Transaction/AdoNetWithDistributedTransactionFactory.cs

+				e.Transaction.TransactionCompleted -= TransactionCompleted;
+				// This event may execute before second phase, so we cannot try to get the success from second phase.
+				// Using this event is required in case the prepare phase failed and called force rollback: no second
+				// phase would occur for this ressource.


This shall be handled on force rollback then

Documentation on which conditions the second phase will not be called at all lacks. At first I have put it all in second phase callbacks, just to realize thanks to some tests there were cases for which it was not good.

So the forced rollback from session volatile prepare phase is maybe only one case in which second phase do not occur. For staying on the safe side, we would better keep that handling in TransactionCompleted.

The fact the completion actions were called from in-doubt too made me wonder whether in in-doubt case, the transaction completed call be not called either. There too, for staying on the safe side, I have decided to keep that call. This transaction factory is the result of many years of fiddling with many failing case for trying to support them all. During my own fiddling with it, I have seen that many design decisions in it, lacking comments and which were looking as overly complicating the code, where indeed needed for some reasons. So I had to kept them, but at least adding comments for most of them.

Moreover, handling transaction completion actions from the prepare phase in case of force rollback may cause other issues, since the session connection state at that point is unknown: has it already processed its own prepare phase? Is it processing it concurrently? Raising completion actions there would trigger its release, and I do not know if all connection implementations would support that.

This is also why I consider we should still provide other transaction factories even if we appear to get this one working: there too many points where we still act on the connection with concurrency risks. Providing other transaction factories would allow whoever wishing it to not take those risks, at the cost of some more explicit flushes calls.

hazzik added investigate tests labels Mar 4, 2015

hazzik added this to the 4.1.0 milestone Mar 4, 2015

hazzik force-pushed the NH-2176 branch from 23b6312 to 6833bce Compare March 17, 2015 06:53

hazzik force-pushed the NH-2176 branch from 6833bce to 1e7790b Compare April 14, 2015 05:18

hazzik added the to clean up label Apr 14, 2015

hazzik force-pushed the master branch from e20411b to 9b7a5a3 Compare October 26, 2016 01:26

oskarb modified the milestones: 5.0.0, 4.1.0 Nov 26, 2016

NH-2176 - Add test case

1f55f46

hazzik force-pushed the NH-2176 branch from 1e7790b to 1f55f46 Compare April 10, 2017 23:44

hazzik removed the to clean up label Apr 10, 2017

NH-2176 - test with a custom transaction factory.

d184a6e

fredericDelaporte mentioned this pull request May 15, 2017

[WIP] Try fix ODP.NET DTC issues #532

Closed

fredericDelaporte force-pushed the NH-2176 branch 8 times, most recently from 2ddb025 to 47070c0 Compare May 16, 2017 14:56

NH-2176 - explicit connection enlistment.

a0c062d

NH-2176 - Fixing a trouble then hitting a road-block.

91c7aa3

fredericDelaporte force-pushed the NH-2176 branch from 47070c0 to 91c7aa3 Compare May 17, 2017 10:16

fredericDelaporte reviewed May 17, 2017

View reviewed changes

NH-2176 - disable custom factory in tests.

88491fc

fredericDelaporte force-pushed the NH-2176 branch from 9c8147b to 5d1fbdf Compare May 17, 2017 20:38

NH-2176 - best attempt I can imagine for fixing current transaction h…

5d167af

…andling.

fredericDelaporte reviewed May 17, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-2176 branch from 5d1fbdf to fafc546 Compare May 17, 2017 23:50

NH-2176 - adding thread synchronization in the mix.

2a4c2a5

fredericDelaporte force-pushed the NH-2176 branch from fafc546 to 2a4c2a5 Compare May 18, 2017 00:16

fredericDelaporte reviewed May 18, 2017

View reviewed changes

hazzik commented May 18, 2017

View reviewed changes

fredericDelaporte mentioned this pull request May 18, 2017

NH-4011 - transaction scope failures #627

Merged

hazzik closed this Jun 2, 2017

hazzik deleted the NH-2176 branch June 2, 2017 10:36

hazzik removed this from the 5.0 milestone Aug 3, 2017

hazzik added c: Tests and removed tests labels Oct 13, 2017

fredericDelaporte added the r: Replaced label Mar 31, 2023

		@@ -113,8 +118,8 @@ public void Can_roll_back_transaction()
		[Description("Another action inside the transaction do the rollBack outside nh-session-scope.")]
		public void RollbackOutsideNh()

NH-2176 - Add test case #410

NH-2176 - Add test case #410

Uh oh!

Conversation

hazzik commented Mar 4, 2015

Uh oh!

hazzik commented Apr 14, 2015

Uh oh!

hazzik commented Apr 10, 2017

Uh oh!

hazzik commented Apr 11, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

fredericDelaporte commented Apr 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

hazzik commented May 17, 2017

Uh oh!

fredericDelaporte commented May 17, 2017

Uh oh!

hazzik commented May 17, 2017

Uh oh!

fredericDelaporte commented May 17, 2017

Uh oh!

hazzik commented May 17, 2017

Uh oh!

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

Uh oh!

Uh oh!

fredericDelaporte commented May 17, 2017

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte May 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 May 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

fredericDelaporte May 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!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte left a comment

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

fredericDelaporte commented Apr 14, 2017 •

edited

Loading

fredericDelaporte commented Apr 23, 2017 •

edited

Loading

hazzik commented Apr 26, 2017 •

edited

Loading

hazzik commented May 17, 2017 •

edited

Loading

fredericDelaporte May 17, 2017 •

edited

Loading

fredericDelaporte May 17, 2017 •

edited

Loading

fredericDelaporte commented May 17, 2017 •

edited

Loading

fredericDelaporte May 17, 2017 •

edited

Loading