NH-2176 - Fixing current system transaction handling.

Author: fredericDelaporte

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

@@ -20,7 +20,7 @@ protected override void Configure(Configuration configuration)
 		{
 			configuration
 				.SetProperty(Environment.UseSecondLevelCache, "true")
-				.SetProperty(Environment.CacheProvider, typeof (HashtableCacheProvider).AssemblyQualifiedName);
+				.SetProperty(Environment.CacheProvider, typeof(HashtableCacheProvider).AssemblyQualifiedName);
 		}
 
 		[Test]
@@ -40,9 +40,9 @@ public void When_using_DTC_HiLo_knows_to_create_isolated_DTC_transaction()
 				var generator = sessions.GetIdentifierGenerator(typeof(Person).FullName);
 				Assert.That(generator, Is.InstanceOf<TableHiLoGenerator>());
 
-				using(var session = sessions.OpenSession())
+				using (var session = sessions.OpenSession())
 				{
-					var id = generator.Generate((ISessionImplementor) session, new Person());
+					var id = generator.Generate((ISessionImplementor)session, new Person());
 				}
 
 				// intentionally dispose without committing
@@ -56,7 +56,7 @@ public void When_using_DTC_HiLo_knows_to_create_isolated_DTC_transaction()
 				scalar2 = command.ExecuteScalar();
 			}
 
-			Assert.AreNotEqual(scalar1, scalar2,"HiLo must run with in its own transaction");
+			Assert.AreNotEqual(scalar1, scalar2, "HiLo must run with in its own transaction");
 		}
 
 		[Test]
@@ -84,43 +84,53 @@ public void When_commiting_items_in_DTC_transaction_will_add_items_to_2nd_level_
 			{
 				using (var s = sessions.OpenSession())
 				{
-					s.Save(new Nums {ID = 29, NumA = 1, NumB = 3});
+					s.Save(new Nums { ID = 29, NumA = 1, NumB = 3 });
 				}
 				tx.Complete();
 			}
-
-			using (var tx = new TransactionScope())
+			try
 			{
-				using (var s = sessions.OpenSession())
+
+				using (var tx = new TransactionScope())
 				{
-					var nums = s.Load<Nums>(29);
-					Assert.AreEqual(1, nums.NumA);
-					Assert.AreEqual(3, nums.NumB);
+					using (var s = sessions.OpenSession())
+					{
+						var nums = s.Load<Nums>(29);
+						Assert.AreEqual(1, nums.NumA);
+						Assert.AreEqual(3, nums.NumB);
+					}
+					tx.Complete();
 				}
-				tx.Complete();
-			}
 
-			//closing the connection to ensure we can't really use it.
-			var connection = sessions.ConnectionProvider.GetConnection();
-			sessions.ConnectionProvider.CloseConnection(connection);
+				//closing the connection to ensure we can't really use it.
+				var connection = sessions.ConnectionProvider.GetConnection();
+				sessions.ConnectionProvider.CloseConnection(connection);
+				// The session is supposed to succeed because the second level cache should have the
+				// entity to load, allowing the session to not use the connection at all.
+				// Will fail if transaction manager tries to enlist user supplied connection, due
+				// to the transaction scope below.
 
-			using (var tx = new TransactionScope())
-			{
-				using (var s = sessions.WithOptions().Connection(connection).OpenSession())
+				using (var tx = new TransactionScope())
 				{
-					var nums = s.Load<Nums>(29);
-					Assert.AreEqual(1, nums.NumA);
-					Assert.AreEqual(3, nums.NumB);
+					using (var s = sessions.WithOptions().Connection(connection).OpenSession())
+					{
+						Nums nums = null;
+						Assert.DoesNotThrow(() => nums = s.Load<Nums>(29), "Failed loading entity from second level cache.");
+						Assert.AreEqual(1, nums.NumA);
+						Assert.AreEqual(3, nums.NumB);
+					}
+					tx.Complete();
 				}
-				tx.Complete();
 			}
-
-			using (var s = sessions.OpenSession())
-			using (var tx = s.BeginTransaction())
+			finally
 			{
-				var nums = s.Load<Nums>(29);
-				s.Delete(nums);
-				tx.Commit();
+				using (var s = sessions.OpenSession())
+				using (var tx = s.BeginTransaction())
+				{
+					var nums = s.Load<Nums>(29);
+					s.Delete(nums);
+					tx.Commit();
+				}
 			}
 		}
 
@@ -174,8 +184,8 @@ public void Will_not_save_when_flush_mode_is_never()
 		[Test]
 		public void When_using_two_sessions_with_explicit_flush()
 		{
-			if (!TestDialect.SupportsConcurrentTransactions)
-				Assert.Ignore(Dialect.GetType().Name + " does not support concurrent transactions.");
+			if (!Dialect.SupportsConcurrentWritingConnectionsInSameTransaction)
+				Assert.Ignore(Dialect.GetType().Name + " does not support concurrent connections in same transaction.");
 
 			object id1, id2;
 			using (var tx = new TransactionScope())
@@ -212,8 +222,8 @@ public void When_using_two_sessions_with_explicit_flush()
 		[Test]
 		public void When_using_two_sessions()
 		{
-			if (!TestDialect.SupportsConcurrentTransactions)
-				Assert.Ignore(Dialect.GetType().Name + " does not support concurrent transactions.");
+			if (!Dialect.SupportsConcurrentWritingConnectionsInSameTransaction)
+				Assert.Ignore(Dialect.GetType().Name + " does not support concurrent connections in same transaction.");
 
 			object id1, id2;
 			using (var tx = new TransactionScope())

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

@@ -13,12 +13,12 @@ protected override void OnSetUp()
 			using (var s = OpenSession())
 			using (var tx = s.BeginTransaction())
 			{
-				var steve = new Person {Name = "Steve"};
-				var peter = new Person {Name = "Peter"};
-				var simon = new Person {Name = "Simon"};
-				var paul = new Person {Name = "Paul"};
-				var john = new Person {Name = "John"};
-				var eric = new Person {Name = "Eric"};
+				var steve = new Person { Name = "Steve" };
+				var peter = new Person { Name = "Peter" };
+				var simon = new Person { Name = "Simon" };
+				var paul = new Person { Name = "Paul" };
+				var john = new Person { Name = "John" };
+				var eric = new Person { Name = "Eric" };
 
 				s.Save(steve);
 				s.Save(peter);
@@ -65,8 +65,8 @@ public void MultipleConsecutiveTransactionScopesCanBeUsedInsideASingleSession()
 						scope.Complete();
 					}
 
-					// The exeption is caused by a race condition between two threads.
-					// This can be demonstracted by uncommenting the following line which
+					// The exception is caused by a race condition between two threads.
+					// This can be demonstrated by uncommenting the following line which
 					// causes the test to run without an exception.
 					//System.Threading.Thread.Sleep(1000);
 				}

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

@@ -1,12 +1,10 @@
-using System;
-using System.Data.Common;
+using System.Data.Common;
 using System.Data.Odbc;
 using System.Data.SqlClient;
 using System.Configuration;
 using System.Transactions;
 using NHibernate.Dialect;
 using NHibernate.Driver;
-using NHibernate.Engine;
 using NUnit.Framework;
 
 using Environment = NHibernate.Cfg.Environment;
@@ -55,44 +53,59 @@ public void ShouldBeAbleToReleaseSuppliedConnectionAfterDistributedTransaction()
 		{
 			string connectionString = FetchConnectionStringFromConfiguration();
 			ISession s;
-			using (var ts = new TransactionScope())
+			DbConnection connection = null;
+			try
 			{
-				// Enlisting DummyEnlistment as a durable resource manager will start
-				// a DTC transaction
-				System.Transactions.Transaction.Current.EnlistDurable(
-					DummyEnlistment.Id,
-					new DummyEnlistment(),
-					EnlistmentOptions.None);
+				using (var ts = new TransactionScope())
+				{
+					// Enlisting DummyEnlistment as a durable resource manager will start
+					// a DTC transaction
+					System.Transactions.Transaction.Current.EnlistDurable(
+						DummyEnlistment.Id,
+						new DummyEnlistment(),
+						EnlistmentOptions.None);
 
-				DbConnection connection;
-				if (sessions.ConnectionProvider.Driver.GetType() == typeof(OdbcDriver))
-					connection = new OdbcConnection(connectionString);
-				else
-					connection = new SqlConnection(connectionString);
+					if (sessions.ConnectionProvider.Driver.GetType() == typeof(OdbcDriver))
+						connection = new OdbcConnection(connectionString);
+					else
+						connection = new SqlConnection(connectionString);
 
-				using (connection)
-				{
 					connection.Open();
 					using (s = Sfi.WithOptions().Connection(connection).OpenSession())
 					{
-						s.Save(new MyTable { String = "hello!" });
+						s.Save(new MyTable {String = "hello!"});
 					}
-					connection.Close();
-				}
+					// The ts disposal may try to flush the session, which, depending on the native generator
+					// implementation for current dialect, may have something to do and will then try to use
+					// the supplied connection. dispose connection here => flaky test, failing for dialects
+					// not mandating an immediate insert on native generator save.
+					// Delaying the connection disposal to after ts disposal.
 
-				ts.Complete();
+					ts.Complete();
+				}
+			}
+			finally
+			{
+				connection?.Dispose();
 			}
 
+			// It appears neither the second phase of the 2PC nor TransactionCompleted 
+			// event are guaranteed to be executed before exiting transaction scope disposal.
+			// When having only 2PC, the second phase tends to occur after reaching that point
+			// 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".
+			s.GetSessionImplementation().TransactionContext?.WaitOne();
+
 			// Prior to the patch, an InvalidOperationException exception would occur in the
 			// TransactionCompleted delegate at this point with the message, "Disconnect cannot
 			// be called while a transaction is in progress". Although the exception can be
 			// seen reported in the IDE, NUnit fails to see it. The TransactionCompleted event
 			// fires *after* the transaction is committed and so it doesn't affect the success
 			// of the transaction.
-
 			Assert.That(s.IsConnected, Is.False);
-			Assert.That(((ISessionImplementor)s).ConnectionManager.IsConnected, Is.False);
-			Assert.That(((ISessionImplementor)s).IsClosed, Is.True);
+			Assert.That(s.GetSessionImplementation().ConnectionManager.IsConnected, Is.False);
+			Assert.That(s.GetSessionImplementation().IsClosed, Is.True);
 		}
 
 		protected override void OnTearDown()

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

@@ -9,28 +9,38 @@ namespace NHibernate.Test.SystemTransactions
 	public class TransactionFixture : TestCase
 	{
 		protected override IList Mappings
+			=> new [] { "WZ.hbm.xml" };
+
+		protected override void OnTearDown()
 		{
-			get { return new string[] { "WZ.hbm.xml" }; }
+			using (var s = sessions.OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				s.CreateQuery("delete from System.Object").ExecuteUpdate();
+				t.Commit();
+			}
 		}
 
 		[Test]
 		public void CanUseSystemTransactionsToCommit()
 		{
 			object identifier;
-			using(ISession session = sessions.OpenSession())
-			using(TransactionScope tx = new TransactionScope())
+			using(var session = sessions.OpenSession())
+			using(var tx = new TransactionScope())
 			{
-				W s = new W();
+				var s = new W();
 				session.Save(s);
 				identifier = s.Id;
 				tx.Complete();
 			}
 
-			using (ISession session = sessions.OpenSession())
-			using (TransactionScope tx = new TransactionScope())
+			using (var session = sessions.OpenSession())
+			using (var tx = new TransactionScope())
 			{
-				W w = session.Get<W>(identifier);
+				var w = session.Get<W>(identifier);
 				Assert.IsNotNull(w);
+				// Without explicit Flush, this delay the delete to prepare phase, and test flushing from there
+				// while having already acquired a connection due to the Get.
 				session.Delete(w);
 				tx.Complete();
 			}

src/NHibernate.Test/SystemTransactions/TransactionFixture.cs

@@ -221,6 +221,7 @@ private bool CheckSessionsWereClosed()
 			var allClosed = true;
 			foreach (var session in _openedSessions)
 			{
+				session.GetSessionImplementation().TransactionContext?.WaitOne();
 				if (session.IsOpen)
 				{
 					log.Error($"Test case didn't close session {session.GetSessionImplementation().SessionId}, closing");

src/NHibernate.Test/TestCase.cs

@@ -33,15 +33,6 @@ public TestDialect(Dialect.Dialect dialect)
 		public virtual bool SupportsOperatorSome { get { return true; } }
 		public virtual bool SupportsLocate { get { return true; } }
 
-		public virtual bool SupportsDistributedTransactions { get { return true; } }
-
-		/// <summary>
-		/// Whether two transactions can be run at the same time.  For example, with SQLite
-		/// the database is locked when one transaction is run, so running a second transaction
-		/// will cause a "database is locked" error message.
-		/// </summary>
-		public virtual bool SupportsConcurrentTransactions { get { return true; } }
-
 		public virtual bool SupportsFullJoin { get { return true; } }
 
         public virtual bool HasBrokenDecimalType { get { return false; } }

src/NHibernate.Test/TestDialect.cs

@@ -21,13 +21,5 @@ public override bool SupportsNullCharactersInUtfStrings
         {
             get { return false; }
         }
-
-        /// <summary>
-        /// Npgsql's DTC code seems to be somewhat broken as of 2.0.11.
-        /// </summary>
-        public override bool SupportsDistributedTransactions
-        {
-            get { return false; }
-        }
 	}
 }

src/NHibernate.Test/TestDialects/PostgreSQL83TestDialect.cs

@@ -27,16 +27,6 @@ public override bool SupportsLocate
 			get { return false; }
 		}
 
-		public override bool SupportsDistributedTransactions
-		{
-			get { return false; }
-		}
-
-		public override bool SupportsConcurrentTransactions
-		{
-			get { return false; }
-		}
-
 		public override bool SupportsFullJoin
 		{
 			get { return false; }