Fix dependent transaction failure

Fix #2172

Author: fredericDelaporte

src/NHibernate.Test/Async/SystemTransactions/DistributedSystemTransactionFixture.cs

@@ -13,13 +13,14 @@
 using System.Threading;
 using System.Transactions;
 using log4net;
-using log4net.Repository.Hierarchy;
 using NHibernate.Cfg;
 using NHibernate.Engine;
-using NHibernate.Linq;
 using NHibernate.Test.TransactionTest;
 using NUnit.Framework;
 
+using sysTran = System.Transactions;
+using NHibernate.Linq;
+
 namespace NHibernate.Test.SystemTransactions
 {
 	using System.Threading.Tasks;
@@ -48,7 +49,7 @@ public async Task SupportsEnlistingInDistributedAsync()
 
 				Assert.AreNotEqual(
 					Guid.Empty,
-					System.Transactions.Transaction.Current.TransactionInformation.DistributedIdentifier,
+					sysTran.Transaction.Current.TransactionInformation.DistributedIdentifier,
 					"Transaction lacks a distributed identifier");
 
 				using (var s = OpenSession())
@@ -75,7 +76,7 @@ public async Task SupportsPromotingToDistributedAsync()
 					"Failure promoting the transaction to distributed while already having enlisted a connection.");
 				Assert.AreNotEqual(
 					Guid.Empty,
-					System.Transactions.Transaction.Current.TransactionInformation.DistributedIdentifier,
+					sysTran.Transaction.Current.TransactionInformation.DistributedIdentifier,
 					"Transaction lacks a distributed identifier");
 			}
 		}
@@ -688,6 +689,39 @@ public async Task EnforceConnectionUsageRulesOnTransactionCompletionAsync()
 			Assert.That(interceptor.AfterException, Is.TypeOf<HibernateException>());
 		}
 
+		[Theory]
+		public async Task CanUseDependentTransactionAsync(bool explicitFlush)
+		{
+			if (!TestDialect.SupportsDependentTransaction)
+				Assert.Ignore("Dialect does not support dependent transactions");
+			IgnoreIfUnsupported(explicitFlush);
+
+			using (var committable = new CommittableTransaction())
+			{
+				sysTran.Transaction.Current = committable;
+				using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+				{
+					sysTran.Transaction.Current = clone;
+
+					using (var s = OpenSession())
+					{
+						if (!AutoJoinTransaction)
+							s.JoinTransaction();
+						await (s.SaveAsync(new Person()));
+
+						if (explicitFlush)
+							await (s.FlushAsync());
+						clone.Complete();
+					}
+				}
+
+				sysTran.Transaction.Current = committable;
+				committable.Commit();
+			}
+
+			sysTran.Transaction.Current = null;
+		}
+
 		private Task DodgeTransactionCompletionDelayIfRequiredAsync(CancellationToken cancellationToken = default(CancellationToken))
 		{
 			try
@@ -716,7 +750,7 @@ public class ForceEscalationToDistributedTx : IEnlistmentNotification
 			public static void Escalate(bool shouldRollBack = false)
 			{
 				var force = new ForceEscalationToDistributedTx(shouldRollBack);
-				System.Transactions.Transaction.Current.EnlistDurable(Guid.NewGuid(), force, EnlistmentOptions.None);
+				sysTran.Transaction.Current.EnlistDurable(Guid.NewGuid(), force, EnlistmentOptions.None);
 			}
 
 			private ForceEscalationToDistributedTx(bool shouldRollBack)

src/NHibernate.Test/Async/SystemTransactions/SystemTransactionFixture.cs

@@ -16,9 +16,10 @@
 using NHibernate.Cfg;
 using NHibernate.Driver;
 using NHibernate.Engine;
-using NHibernate.Linq;
 using NHibernate.Test.TransactionTest;
 using NUnit.Framework;
+using sysTran = System.Transactions;
+using NHibernate.Linq;
 
 namespace NHibernate.Test.SystemTransactions
 {
@@ -523,6 +524,44 @@ public async Task EnforceConnectionUsageRulesOnTransactionCompletionAsync()
 			// Currently always forbidden, whatever UseConnectionOnSystemTransactionEvents.
 			Assert.That(interceptor.AfterException, Is.TypeOf<HibernateException>());
 		}
+
+		[Theory]
+		public async Task CanUseDependentTransactionAsync(bool explicitFlush)
+		{
+			if (!TestDialect.SupportsDependentTransaction)
+				Assert.Ignore("Dialect does not support dependent transactions");
+			IgnoreIfUnsupported(explicitFlush);
+
+			try
+			{
+				using (var committable = new CommittableTransaction())
+				{
+					sysTran.Transaction.Current = committable;
+					using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+					{
+						sysTran.Transaction.Current = clone;
+
+						using (var s = OpenSession())
+						{
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							await (s.SaveAsync(new Person()));
+
+							if (explicitFlush)
+								await (s.FlushAsync());
+							clone.Complete();
+						}
+					}
+
+					sysTran.Transaction.Current = committable;
+					committable.Commit();
+				}
+			}
+			finally
+			{
+				sysTran.Transaction.Current = null;
+			}
+		}
 	}
 
 	[TestFixture]

src/NHibernate.Test/SystemTransactions/DistributedSystemTransactionFixture.cs

@@ -3,13 +3,13 @@
 using System.Threading;
 using System.Transactions;
 using log4net;
-using log4net.Repository.Hierarchy;
 using NHibernate.Cfg;
 using NHibernate.Engine;
-using NHibernate.Linq;
 using NHibernate.Test.TransactionTest;
 using NUnit.Framework;
 
+using sysTran = System.Transactions;
+
 namespace NHibernate.Test.SystemTransactions
 {
 	[TestFixture]
@@ -37,7 +37,7 @@ public void SupportsEnlistingInDistributed()
 
 				Assert.AreNotEqual(
 					Guid.Empty,
-					System.Transactions.Transaction.Current.TransactionInformation.DistributedIdentifier,
+					sysTran.Transaction.Current.TransactionInformation.DistributedIdentifier,
 					"Transaction lacks a distributed identifier");
 
 				using (var s = OpenSession())
@@ -64,7 +64,7 @@ public void SupportsPromotingToDistributed()
 					"Failure promoting the transaction to distributed while already having enlisted a connection.");
 				Assert.AreNotEqual(
 					Guid.Empty,
-					System.Transactions.Transaction.Current.TransactionInformation.DistributedIdentifier,
+					sysTran.Transaction.Current.TransactionInformation.DistributedIdentifier,
 					"Transaction lacks a distributed identifier");
 			}
 		}
@@ -711,6 +711,39 @@ public void AdditionalJoinDoesNotThrow()
 			}
 		}
 
+		[Theory]
+		public void CanUseDependentTransaction(bool explicitFlush)
+		{
+			if (!TestDialect.SupportsDependentTransaction)
+				Assert.Ignore("Dialect does not support dependent transactions");
+			IgnoreIfUnsupported(explicitFlush);
+
+			using (var committable = new CommittableTransaction())
+			{
+				sysTran.Transaction.Current = committable;
+				using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+				{
+					sysTran.Transaction.Current = clone;
+
+					using (var s = OpenSession())
+					{
+						if (!AutoJoinTransaction)
+							s.JoinTransaction();
+						s.Save(new Person());
+
+						if (explicitFlush)
+							s.Flush();
+						clone.Complete();
+					}
+				}
+
+				sysTran.Transaction.Current = committable;
+				committable.Commit();
+			}
+
+			sysTran.Transaction.Current = null;
+		}
+
 		private void DodgeTransactionCompletionDelayIfRequired()
 		{
 			if (Sfi.ConnectionProvider.Driver.HasDelayedDistributedTransactionCompletion)
@@ -725,7 +758,7 @@ public class ForceEscalationToDistributedTx : IEnlistmentNotification
 			public static void Escalate(bool shouldRollBack = false)
 			{
 				var force = new ForceEscalationToDistributedTx(shouldRollBack);
-				System.Transactions.Transaction.Current.EnlistDurable(Guid.NewGuid(), force, EnlistmentOptions.None);
+				sysTran.Transaction.Current.EnlistDurable(Guid.NewGuid(), force, EnlistmentOptions.None);
 			}
 
 			private ForceEscalationToDistributedTx(bool shouldRollBack)

src/NHibernate.Test/SystemTransactions/SystemTransactionFixture.cs

@@ -6,9 +6,9 @@
 using NHibernate.Cfg;
 using NHibernate.Driver;
 using NHibernate.Engine;
-using NHibernate.Linq;
 using NHibernate.Test.TransactionTest;
 using NUnit.Framework;
+using sysTran = System.Transactions;
 
 namespace NHibernate.Test.SystemTransactions
 {
@@ -522,6 +522,44 @@ public void AdditionalJoinDoesNotThrow()
 				Assert.DoesNotThrow(() => s.JoinTransaction());
 			}
 		}
+
+		[Theory]
+		public void CanUseDependentTransaction(bool explicitFlush)
+		{
+			if (!TestDialect.SupportsDependentTransaction)
+				Assert.Ignore("Dialect does not support dependent transactions");
+			IgnoreIfUnsupported(explicitFlush);
+
+			try
+			{
+				using (var committable = new CommittableTransaction())
+				{
+					sysTran.Transaction.Current = committable;
+					using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+					{
+						sysTran.Transaction.Current = clone;
+
+						using (var s = OpenSession())
+						{
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							s.Save(new Person());
+
+							if (explicitFlush)
+								s.Flush();
+							clone.Complete();
+						}
+					}
+
+					sysTran.Transaction.Current = committable;
+					committable.Commit();
+				}
+			}
+			finally
+			{
+				sysTran.Transaction.Current = null;
+			}
+		}
 	}
 
 	[TestFixture]

src/NHibernate.Test/TestDialect.cs

@@ -157,6 +157,14 @@ public bool SupportsSqlType(SqlType sqlType)
 		/// </summary>
 		public virtual bool SupportsUsingConnectionOnSystemTransactionPrepare => true;
 
+		/// <summary>
+		/// Some databases fail with dependent transaction, typically when their driver try to access the transaction
+		/// state from its two PC: the dependent transaction is meant to be disposed of before completing the actual
+		/// transaction, so it is usually disposed at this point, and its state cannot be read. (Drivers should always
+		/// clone transactions for avoiding this trouble.)
+		/// </summary>
+		public virtual bool SupportsDependentTransaction => true;
+
 		/// <summary>
 		/// Some databases (provider?) fails to compute adequate column types for queries which columns
 		/// computing include a parameter value.

src/NHibernate.Test/TestDialects/PostgreSQL83TestDialect.cs

@@ -16,5 +16,11 @@ public PostgreSQL83TestDialect(Dialect.Dialect dialect)
 		/// Npgsql 3.2.4.1.
 		/// </summary>
 		public override bool SupportsUsingConnectionOnSystemTransactionPrepare => false;
+
+		/// <summary>
+		/// Npgsql does not clone the transaction in its context, and uses it in its prepare phase. When that was a
+		/// dependent transaction, it is then usually already disposed of, causing Npgsql to crash.
+		/// </summary>
+		public override bool SupportsDependentTransaction => false;
 	}
 }

src/NHibernate/Async/Transaction/AdoNetWithSystemTransactionFactory.cs

@@ -16,7 +16,6 @@
 using NHibernate.AdoNet;
 using NHibernate.Engine;
 using NHibernate.Engine.Transaction;
-using NHibernate.Impl;
 using NHibernate.Util;
 
 namespace NHibernate.Transaction

src/NHibernate/Transaction/AdoNetWithSystemTransactionFactory.cs

@@ -6,7 +6,6 @@
 using NHibernate.AdoNet;
 using NHibernate.Engine;
 using NHibernate.Engine.Transaction;
-using NHibernate.Impl;
 using NHibernate.Util;
 
 namespace NHibernate.Transaction
@@ -186,6 +185,7 @@ public class SystemTransactionContext : ITransactionContext, IEnlistmentNotifica
 			private readonly System.Transactions.Transaction _originalTransaction;
 			private readonly ManualResetEventSlim _lock = new ManualResetEventSlim(true);
 			private volatile bool _needCompletionLocking = true;
+			private bool _preparing;
 			// Required for not locking the completion phase itself when locking session usages from concurrent threads.
 			private static readonly AsyncLocal<bool> _bypassLock = new AsyncLocal<bool>();
 			private readonly int _systemTransactionCompletionLockTimeout;
@@ -236,7 +236,7 @@ public virtual void Wait()
 					// Remove the block then throw.
 					Unlock();
 					throw new HibernateException(
-						"Synchronization timeout for transaction completion. Either raise {Cfg.Environment.SystemTransactionCompletionLockTimeout}, or this may be a bug in NHibernate.");
+						$"Synchronization timeout for transaction completion. Either raise {Cfg.Environment.SystemTransactionCompletionLockTimeout}, or this may be a bug in NHibernate.");
 				}
 				catch (HibernateException)
 				{
@@ -285,16 +285,17 @@ protected virtual void Unlock()
 				{
 					// Cloned transaction is not disposed "unexpectedly", its status is accessible till context disposal.
 					var status = EnlistedTransaction.TransactionInformation.Status;
-					if (status != TransactionStatus.Active)
+					if (status != TransactionStatus.Active || _preparing)
 						return status;
 
-					// The clone status can be out of date when active, check the original one (which could be disposed if
-					// the clone is out of date).
+					// The clone status can be out of date when active and not in prepare phase, in case of rollback.
+					// In such case the original transaction is already disposed, and trying to check its status will
+					// trigger a dispose exception.
 					return _originalTransaction.TransactionInformation.Status;
 				}
 				catch (ObjectDisposedException ode)
 				{
-					_logger.Warn(ode, "Enlisted transaction status was wrongly active, original transaction being already disposed. Will assume neither active nor committed.");
+					_logger.Warn(ode, "Enlisted transaction status is maybe wrongly active, original transaction being already disposed. Will assume neither active nor committed.");
 					return null;
 				}
 			}
@@ -310,6 +311,7 @@ protected virtual void Unlock()
 			/// <param name="preparingEnlistment">The object for notifying the prepare phase outcome.</param>
 			public virtual void Prepare(PreparingEnlistment preparingEnlistment)
 			{
+				_preparing = true;
 				using (_session.BeginContext())
 				{
 					try
@@ -343,10 +345,12 @@ public virtual void Prepare(PreparingEnlistment preparingEnlistment)
 						Lock();
 
 						_logger.Debug("Prepared for system transaction");
+						_preparing = false;
 						preparingEnlistment.Prepared();
 					}
 					catch (Exception exception)
 					{
+						_preparing = false;
 						_logger.Error(exception, "System transaction prepare phase failed");
 						try
 						{
@@ -404,7 +408,7 @@ protected virtual void ProcessSecondPhase(Enlistment enlistment, bool? success)
 			/// <summary>
 			/// Handle the transaction completion. Notify <see cref="ConnectionManager"/> of the end of the
 			/// transaction. Notify end of transaction to the session and to <see cref="ConnectionManager.DependentSessions"/>
-			/// if any. Close sessions requiring it then cleanup transaction contextes and then <see cref="Unlock"/> blocked
+			/// if any. Close sessions requiring it then cleanup transaction contexts and then <see cref="Unlock"/> blocked
 			/// threads.
 			/// </summary>
 			/// <param name="isCommitted"><see langword="true"/> if the transaction is committed, <see langword="false"/>