Merge pull request #2207 from fredericDelaporte/5.0.x-2172

Fix dependent transaction failure in 5.0.x

build-common/NHibernate.props

@@ -2,7 +2,7 @@
   <PropertyGroup>
     <VersionMajor Condition="'$(VersionMajor)' == ''">5</VersionMajor>
     <VersionMinor Condition="'$(VersionMinor)' == ''">0</VersionMinor>
-    <VersionPatch Condition="'$(VersionPatch)' == ''">7</VersionPatch>
+    <VersionPatch Condition="'$(VersionPatch)' == ''">8</VersionPatch>
     <VersionSuffix Condition="'$(VersionSuffix)' == ''"></VersionSuffix>
 
     <VersionPrefix>$(VersionMajor).$(VersionMinor).$(VersionPatch)</VersionPrefix>

build-common/common.xml

@@ -30,8 +30,8 @@
 
 	<!-- This is used only for build folder -->
 	<!-- TODO: Either remove or refactor to use NHibernate.props -->
-	<property name="project.version" value="5.0.7" overwrite="false" />
-	<property name="project.version.numeric" value="5.0.7" overwrite="false" />
+	<property name="project.version" value="5.0.8" overwrite="false" />
+	<property name="project.version.numeric" value="5.0.8" overwrite="false" />
 
 	<!-- properties used to connect to database for testing -->
 	<include buildfile="nhibernate-properties.xml" />

releasenotes.txt

-Build 5.0.7
+Build 5.0.8
+=============================
+
+Release notes - NHibernate - Version 5.0.8
+
+** Bug
+    * #2172 Using DependentTransaction fails
+
+Build 5.0.7
 =============================
 
 Release notes - NHibernate - Version 5.0.7

src/AsyncGenerator.yml

@@ -164,6 +164,18 @@
   applyChanges: true
   analyzation:
     methodConversion:
+    - conversion: Ignore
+      name: CanUseDependentTransaction
+      containingTypeName: DistributedSystemTransactionFixture
+    - conversion: Ignore
+      name: CanUseSessionWithManyDependentTransaction
+      containingTypeName: DistributedSystemTransactionFixture
+    - conversion: Ignore
+      name: CanUseDependentTransaction
+      containingTypeName: SystemTransactionFixture
+    - conversion: Ignore
+      name: CanUseSessionWithManyDependentTransaction
+      containingTypeName: SystemTransactionFixture
     - conversion: Copy
       hasAttributeName: OneTimeSetUpAttribute
     - conversion: Copy

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

@@ -16,9 +16,9 @@
 using log4net.Repository.Hierarchy;
 using NHibernate.Cfg;
 using NHibernate.Engine;
-using NHibernate.Linq;
 using NHibernate.Test.TransactionTest;
 using NUnit.Framework;
+using NHibernate.Linq;
 
 namespace NHibernate.Test.SystemTransactions
 {
@@ -780,4 +780,4 @@ protected override void Configure(Configuration configuration)
 		protected override bool AppliesTo(ISessionFactoryImplementor factory)
 			=> base.AppliesTo(factory) && factory.ConnectionProvider.Driver.SupportsEnlistmentWhenAutoEnlistmentIsDisabled;
 	}
-}
\ No newline at end of file
+}

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

@@ -14,10 +14,11 @@
 using System.Threading;
 using System.Transactions;
 using NHibernate.Cfg;
+using NHibernate.Driver;
 using NHibernate.Engine;
-using NHibernate.Linq;
 using NHibernate.Test.TransactionTest;
 using NUnit.Framework;
+using NHibernate.Linq;
 
 namespace NHibernate.Test.SystemTransactions
 {
@@ -529,4 +530,4 @@ public class SystemTransactionWithoutAutoJoinTransactionAsync : SystemTransactio
 	{
 		protected override bool AutoJoinTransaction => false;
 	}
-}
\ No newline at end of file
+}

src/NHibernate.Test/SystemTransactions/DistributedSystemTransactionFixture.cs

@@ -6,7 +6,6 @@
 using log4net.Repository.Hierarchy;
 using NHibernate.Cfg;
 using NHibernate.Engine;
-using NHibernate.Linq;
 using NHibernate.Test.TransactionTest;
 using NUnit.Framework;
 
@@ -728,6 +727,117 @@ public void AdditionalJoinDoesNotThrow()
 			}
 		}
 
+		[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())
+				{
+					System.Transactions.Transaction.Current = committable;
+					using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+					{
+						System.Transactions.Transaction.Current = clone;
+
+						using (var s = OpenSession())
+						{
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							s.Save(new Person());
+
+							if (explicitFlush)
+								s.Flush();
+							clone.Complete();
+						}
+					}
+
+					System.Transactions.Transaction.Current = committable;
+					committable.Commit();
+				}
+			}
+			finally
+			{
+				System.Transactions.Transaction.Current = null;
+			}
+		}
+
+		[Theory]
+		public void CanUseSessionWithManyDependentTransaction(bool explicitFlush)
+		{
+			if (!TestDialect.SupportsDependentTransaction)
+				Assert.Ignore("Dialect does not support dependent transactions");
+			IgnoreIfUnsupported(explicitFlush);
+
+			try
+			{
+				using (var s = Sfi.WithOptions().ConnectionReleaseMode(ConnectionReleaseMode.OnClose).OpenSession())
+				{
+					using (var committable = new CommittableTransaction())
+					{
+						System.Transactions.Transaction.Current = committable;
+						using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+						{
+							System.Transactions.Transaction.Current = clone;
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							// Acquire the connection
+							var count = s.Query<Person>().Count();
+							Assert.That(count, Is.EqualTo(0), "Unexpected initial entity count.");
+							clone.Complete();
+						}
+
+						using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+						{
+							System.Transactions.Transaction.Current = clone;
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							s.Save(new Person());
+
+							if (explicitFlush)
+								s.Flush();
+
+							clone.Complete();
+						}
+
+						using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+						{
+							System.Transactions.Transaction.Current = clone;
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							var count = s.Query<Person>().Count();
+							Assert.That(count, Is.EqualTo(1), "Unexpected entity count after committed insert.");
+							clone.Complete();
+						}
+
+						System.Transactions.Transaction.Current = committable;
+						committable.Commit();
+					}
+				}
+			}
+			finally
+			{
+				System.Transactions.Transaction.Current = null;
+			}
+
+			DodgeTransactionCompletionDelayIfRequired();
+
+			using (var s = OpenSession())
+			{
+				using (var tx = new TransactionScope())
+				{
+					if (!AutoJoinTransaction)
+						s.JoinTransaction();
+					var count = s.Query<Person>().Count();
+					Assert.That(count, Is.EqualTo(1), "Unexpected entity count after global commit.");
+					tx.Complete();
+				}
+			}
+		}
+
 		private void DodgeTransactionCompletionDelayIfRequired()
 		{
 			if (Sfi.ConnectionProvider.Driver.HasDelayedDistributedTransactionCompletion)
@@ -820,4 +930,4 @@ public void SessionIsNotEnlisted()
 			}
 		}
 	}
-}
\ No newline at end of file
+}

src/NHibernate.Test/SystemTransactions/SystemTransactionFixture.cs

@@ -4,8 +4,8 @@
 using System.Threading;
 using System.Transactions;
 using NHibernate.Cfg;
+using NHibernate.Driver;
 using NHibernate.Engine;
-using NHibernate.Linq;
 using NHibernate.Test.TransactionTest;
 using NUnit.Framework;
 
@@ -502,6 +502,120 @@ 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())
+				{
+					System.Transactions.Transaction.Current = committable;
+					using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+					{
+						System.Transactions.Transaction.Current = clone;
+
+						using (var s = OpenSession())
+						{
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							s.Save(new Person());
+
+							if (explicitFlush)
+								s.Flush();
+							clone.Complete();
+						}
+					}
+
+					System.Transactions.Transaction.Current = committable;
+					committable.Commit();
+				}
+			}
+			finally
+			{
+				System.Transactions.Transaction.Current = null;
+			}
+		}
+
+		[Theory]
+		public void CanUseSessionWithManyDependentTransaction(bool explicitFlush)
+		{
+			if (!TestDialect.SupportsDependentTransaction)
+				Assert.Ignore("Dialect does not support dependent transactions");
+			IgnoreIfUnsupported(explicitFlush);
+			// ODBC with SQL-Server always causes system transactions to go distributed, which causes their transaction completion to run
+			// asynchronously. But ODBC enlistment also check the previous transaction in a way that do not guard against it
+			// being concurrently disposed of. See https://github.com/nhibernate/nhibernate-core/pull/1505 for more details.
+			if (Sfi.ConnectionProvider.Driver is OdbcDriver)
+				Assert.Ignore("ODBC sometimes fails on second scope by checking the previous transaction status, which may yield an object disposed exception");
+
+			try
+			{
+				using (var s = WithOptions().ConnectionReleaseMode(ConnectionReleaseMode.OnClose).OpenSession())
+				{
+					using (var committable = new CommittableTransaction())
+					{
+						System.Transactions.Transaction.Current = committable;
+						using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+						{
+							System.Transactions.Transaction.Current = clone;
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							// Acquire the connection
+							var count = s.Query<Person>().Count();
+							Assert.That(count, Is.EqualTo(0), "Unexpected initial entity count.");
+							clone.Complete();
+						}
+
+						using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+						{
+							System.Transactions.Transaction.Current = clone;
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							s.Save(new Person());
+
+							if (explicitFlush)
+								s.Flush();
+
+							clone.Complete();
+						}
+
+						using (var clone = committable.DependentClone(DependentCloneOption.RollbackIfNotComplete))
+						{
+							System.Transactions.Transaction.Current = clone;
+							if (!AutoJoinTransaction)
+								s.JoinTransaction();
+							var count = s.Query<Person>().Count();
+							Assert.That(count, Is.EqualTo(1), "Unexpected entity count after committed insert.");
+							clone.Complete();
+						}
+
+						System.Transactions.Transaction.Current = committable;
+						committable.Commit();
+					}
+				}
+			}
+			finally
+			{
+				System.Transactions.Transaction.Current = null;
+			}
+
+			using (var s = OpenSession())
+			{
+				using (var tx = new TransactionScope())
+				{
+					if (!AutoJoinTransaction)
+						s.JoinTransaction();
+					var count = s.Query<Person>().Count();
+					Assert.That(count, Is.EqualTo(1), "Unexpected entity count after global commit.");
+					tx.Complete();
+				}
+			}
+		}
 	}
 
 	[TestFixture]
@@ -539,4 +653,4 @@ public void SessionIsNotEnlisted()
 			}
 		}
 	}
-}
\ No newline at end of file
+}

src/NHibernate.Test/TestDialect.cs

@@ -72,5 +72,13 @@ public bool SupportsSqlType(SqlType sqlType)
 				return false;
 			}
 		}
+
+		/// <summary>
+		/// Some databases fail with dependent transaction, typically when their driver tries 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;
 	}
 }

src/NHibernate.Test/TestDialects/PostgreSQL83TestDialect.cs

@@ -16,5 +16,11 @@ public override bool SupportsNullCharactersInUtfStrings
         {
             get { return 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/AdoNet/ConnectionManager.cs

@@ -456,7 +456,13 @@ public DbCommand CreateCommand()
 		public void EnlistIfRequired(System.Transactions.Transaction transaction)
 		{
 			if (transaction == _currentSystemTransaction)
+			{
+				// Short-circuit after having stored the transaction : they may be equal, but not the same reference.
+				// And the previous one may be an already disposed dependent clone, in which case we need to update
+				// our reference.
+				_currentSystemTransaction = transaction;
 				return;
+			}
 
 			_currentSystemTransaction = transaction;
 

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 readonly AsyncLocal<bool> _bypassLock = new AsyncLocal<bool>();
 			private readonly int _systemTransactionCompletionLockTimeout;
@@ -236,7 +236,9 @@ 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 check all scopes are properly" +
+						$"disposed and/or all direct System.Transaction.Current changes are properly managed.");
 				}
 				catch (HibernateException)
 				{
@@ -284,16 +286,44 @@ 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 or
+					// dependent clone usage.
+					// 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("Enlisted transaction status was wrongly active, original transaction being already disposed. Will assume neither active nor committed.", ode);
+					// For ruling out the dependent clone case when possible, we check if the current transaction is
+					// equal to the context one (System.Transactions.Transaction does override equality for this), and
+					// in such case, we check the state of the current transaction instead. (The state of the current
+					// transaction if equal can only be the same, but it will be inaccessible in case of rollback, due
+					// to the current transaction being already disposed.)
+					// The current transaction may not be reachable during 2PC phases and transaction completion events,
+					// but in such cases the context transaction is either no more active or in prepare phase, which is
+					// already covered by _preparing test.
+					try
+					{
+						var currentTransaction = System.Transactions.Transaction.Current;
+						if (!ReferenceEquals(currentTransaction, _originalTransaction) &&
+							currentTransaction == EnlistedTransaction)
+							return currentTransaction.TransactionInformation.Status;
+					}
+					catch (ObjectDisposedException)
+					{
+						// Just ignore that one, no use to log two dispose exceptions which are indeed the same.
+					}
+					catch (InvalidOperationException ioe)
+					{
+						_logger.Warn("Attempting to dodge a disposed transaction trouble, current" +
+						             "transaction was unreachable.", ioe);
+					}
+
+					_logger.Warn("Enlisted transaction status is maybe wrongly active, original " +
+					             "transaction being already disposed. Will assume neither active nor committed.", ode);
 					return null;
 				}
 			}
@@ -309,6 +339,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
@@ -342,10 +373,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("System transaction prepare phase failed", exception);
 						try
 						{
@@ -403,7 +436,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"/>