NH-2176 - best attempt I can imagine for fixing current transaction handling.

Author: fredericDelaporte

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

@@ -169,7 +169,7 @@ public void TransactionInsertWithRollBackTask()
 			}
 		}
 
-		[Test, Ignore("Not fixed.")]
+		[Test/*, Ignore("Not fixed.")*/]
 		[Description(@"Two session in two txscope 
 (without an explicit NH transaction and without an explicit flush) 
 and with a rollback in the second dtc and a ForceRollback outside nh-session-scope.")]

src/NHibernate.Test/NHSpecificTest/NH1632/Fixture.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();
+				}
 			}
 		}
 

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

@@ -3,6 +3,7 @@
 using System.Data.Odbc;
 using System.Data.SqlClient;
 using System.Configuration;
+using System.Threading;
 using System.Transactions;
 using NHibernate.Dialect;
 using NHibernate.Driver;
@@ -55,41 +56,56 @@ 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".
+			((ISessionImplementor) s).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);

src/NHibernate/AdoNet/ConnectionManager.cs

@@ -29,6 +29,12 @@ public interface Callback
 
 		[NonSerialized]
 		private DbConnection connection;
+		[NonSerialized]
+		private DbConnection _dtcBackupConnection;
+		[NonSerialized]
+		private System.Transactions.Transaction _connectionAmbientTransaction;
+		[NonSerialized]
+		private System.Transactions.Transaction _dtcBackupConnectionAmbientTransaction;
 		// Whether we own the connection, i.e. connect and disconnect automatically.
 		private bool ownConnection;
 
@@ -184,6 +190,7 @@ public DbConnection GetConnection()
 				if (ownConnection)
 				{
 					connection = Factory.ConnectionProvider.GetConnection();
+					_connectionAmbientTransaction = System.Transactions.Transaction.Current;
 					if (Factory.Statistics.IsStatisticsEnabled)
 					{
 						Factory.StatisticsImplementor.Connect();
@@ -394,10 +401,29 @@ public IBatcher Batcher
 			get { return batcher; }
 		}
 
+		/// <summary>
+		/// Tell if according to current ambient transaction, the connection should be explicitly enlisted
+		/// for ensuring it to participate in transaction. Always <see langword="false" /> for supplied
+		/// connection, enlistment is user business in such case.
+		/// </summary>
+		public bool RequireExplicitEnlistment
+			=> ownConnection && connection != null &&
+			_connectionAmbientTransaction != System.Transactions.Transaction.Current;
+
 		public IDisposable FlushingFromDtcTransaction
 		{
 			get
 			{
+				if (ownConnection)
+				{
+					if (Batcher.HasOpenResources)
+						throw new InvalidOperationException("Batcher still has opened ressources at time of Flush from DTC.");
+					// Swap out current connection for avoiding using it concurrently to its own 2PC
+					_dtcBackupConnection = connection;
+					_dtcBackupConnectionAmbientTransaction = _connectionAmbientTransaction;
+					connection = null;
+					_connectionAmbientTransaction = null;
+				}
 				flushingFromDtcTransaction = true;
 				return new StopFlushingFromDtcTransaction(this);
 			}
@@ -415,6 +441,17 @@ public StopFlushingFromDtcTransaction(ConnectionManager manager)
 			public void Dispose()
 			{
 				manager.flushingFromDtcTransaction = false;
+
+				if (manager.ownConnection)
+				{
+					// Release the connection potentially acquired for flushing from DTC.
+					manager.DisconnectOwnConnection();
+					// Swap back current connection
+					manager.connection = manager._dtcBackupConnection;
+					manager._connectionAmbientTransaction = manager._dtcBackupConnectionAmbientTransaction;
+					manager._dtcBackupConnection = null;
+					manager._dtcBackupConnectionAmbientTransaction = null;
+				}
 			}
 		}