NH-4011 - transaction scope failures #627

fredericDelaporte · 2017-05-18T17:32:16Z

~~NH-2176~~ NH-4011 - That is gone to the point of fixing it as much as possible.

Since the branch used in #410 is public and in the official NHibernate repository, rebasing it is debatable.

So for going on with my tests, I have switched on a branch in my own repository.

Rebased, custom factory dropped, slightly squashed.

DTC failure tests cleaned up and completed.

PostgreSQL reactivated for DTC in order to check what is going on with it.

fredericDelaporte · 2017-05-20T14:39:43Z

~~null enlistment does not looks required for any tested database~~. But Oracle Managed seems to be flaky, it looks like sometime it needs some "delay" between two transactions for the next one to read what the previous one has done. I have seen that locally on two different cases, one re-using the connection and the other re-acquiring it from pool. (Using Oracle 11 XE, ODAC 12 r4, and lot of fiddling for installing this db and configuring Nhibernate for it. I do not know much about Oracle.)

Update: the delay is a general trouble not specific to Oracle, due to MSDTC asynchronism. MSDTC let the scope disposal leave before the database has actually finished committing. Read more on later comments.

fredericDelaporte · 2017-05-20T19:59:45Z

It looks like Sql Server has issues with connection being released during transaction scope, when enlisting a new one in prepare phase. Removing the commit having done that change.

fredericDelaporte · 2017-05-21T19:51:10Z

CanUseSessionOutsideOfScopeAfterScopeWithFlush demonstrates enlistment with null is indeed required at least for sql-server in some cases. Going to add that back.

fredericDelaporte · 2017-05-29T11:48:55Z

src/NHibernate.Test/TestDialect.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; } }


Moved to dialect informational metadata.

fredericDelaporte · 2017-05-29T11:49:27Z

src/NHibernate.Test/TestDialect.cs

-		/// 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; } }


Moved to dialect properties, required for new ConnectionManager implementation.

fredericDelaporte · 2017-05-29T11:50:42Z

src/NHibernate/AdoNet/ConnectionManager.cs

@@ -12,144 +12,131 @@ namespace NHibernate.AdoNet
 	/// Manages the database connection and transaction for an <see cref="ISession" />.
 	/// </summary>
 	/// <remarks>
-	/// This class corresponds to LogicalConnectionImplementor and JdbcCoordinator in Hibernate,


Having done significant changes anyway, I have cleaned-up the class.

fredericDelaporte · 2017-05-29T11:51:08Z

src/NHibernate/AdoNet/ConnectionManager.cs

 	/// </remarks>
 	[Serializable]
 	public class ConnectionManager : ISerializable, IDeserializationCallback
 	{
-		private static readonly IInternalLogger log = LoggerProvider.LoggerFor(typeof(ConnectionManager));
-
-		public interface Callback


Unused interface. Seems a forgotten old trash.

fredericDelaporte · 2017-05-29T12:05:53Z

src/NHibernate/AdoNet/ConnectionManager.cs

+		/// Does nothing in case an explicit transaction ongoing.
+		/// </summary>
+		/// <param name="transaction">The transaction in which the connection should be enlisted.</param>
+		public void EnlistIfRequired(System.Transactions.Transaction transaction)


Previously NHibernate was relying solely on connection auto-enlistment. This could not work with OnClose connection release mode, and this was sometimes failing due to some race condition causing the connection to have not been un-enlisted soon enough from the transaction end. (In addition to not have been released and re-acquired.)
Enlisting explicitly avoids the "late un-enlistment" trouble.

fredericDelaporte · 2017-05-29T12:10:27Z

src/NHibernate/Dialect/Dialect.cs

+		/// Distributed transactions usually imply the use of<see cref="TransactionScope"/>, but using
+		/// <c>TransactionScope</c> does not imply the transaction will be distributed.
+		/// </remarks>
+		public virtual bool SupportsDistributedTransactions => true;


It looks in most case more the data-provider "fault" when not supported than the database itself. Maybe should it be moved to driver.

fredericDelaporte · 2017-06-15T10:54:58Z

Jim Carley, from Microsoft, has given some interesting explanations about MSDTC behavior here, upon request of the Npgsql data provider GitHub owner. It notably confirms that the second phase of the 2 phases commit can occurs after the scope disposal has left, because the transaction is considered committed as soon as MSDTC has recorded the vote of all resources. Second phases notifications are sent after that point in the commit case.

SqlClient has probably some internal locking logic preventing its consumers to be exposed to that, but most other data provider, including Oracle ODAC, lack it.

fredericDelaporte · 2017-06-16T13:40:52Z

Things get muddier... As said in the Npgsql issue, I have reproduced the "delayed transaction commit" issue with SQL Server too. That is not ADO.Net data provider which was protecting it from this issue, but its old way of handling "read committed" isolation level. Activating the read-committed snapshot mode causes SQL Server to also have that delayed commit issue, where the data committed from a distributed transaction may not be there right after the scope disposal. A Thread.Sleep then allows to query it a bit later...

So indeed, that is a general rule, after a distributed scope disposal, tests cannot assume they will be able to read the committed data...

Now enlistment ressources having knowledge of this 2PC occurring concurrently to code after scope disposal have very likely some locking mechanism similar to the one I have added to protect against concurrent usages leading to failures (protecting session for NHibernate, connections for data providers). Npgsql clearly lacks this but its maintainer seems to be willing to implement some. (I have submitted a test case demonstrating a crash due to that concurrency, in its test suit.)

The current synchronization mechanism I have put for NHibernate seems to do the job for the commit case (avoiding crashes, but not avoiding the stale data issue). It does not do it yet for rollback cases where second phase occurs directly without a first phase, I have to adjust that.

fredericDelaporte · 2017-06-18T18:51:44Z

src/NHibernate.Test/SystemTransactions/TransactionFixture.cs

 using NUnit.Framework;

 namespace NHibernate.Test.SystemTransactions
 {
 	[TestFixture]
-	public class TransactionFixture : TestCase
+	public class TransactionFixture : TransactionFixtureBase


Those tests were a bit minimalist. Replaced by those from distributed case, striped from distributed promotion.

fredericDelaporte · 2017-06-18T18:55:28Z

src/NHibernate/Driver/FirebirdClientDriver.cs

+		/// </item>
+		/// </list>
+		/// </remarks>
+		public override bool SupportsSystemTransactions => false;


Firebird .Net data provider is messy to the point of forbidding us to test even non distributed case.

fredericDelaporte · 2017-06-18T18:58:51Z

src/NHibernate.Test/SystemTransactions/DistributedTransactionFixture.cs

+			// Note that this fails with ConnectionReleaseMode.OnClose and SqlServer:
+			// System.Data.SqlClient.SqlException : Microsoft Distributed Transaction Coordinator (MS DTC) has stopped this transaction.
+			using (var s = OpenSession())
+			//using (var s = Sfi.WithOptions().ConnectionReleaseMode(ConnectionReleaseMode.OnClose).OpenSession())


Yuck, one more trouble. I do not see anyway to fix that, excepted not using the old OnClose connection release mode. Since documentation advises against using it, I think we are fine not fixing that trouble.

fredericDelaporte

In this review, I point the changes I have made for rebasing on async feature.

There are some troubles in test generation causing missing async test counterparts. Either we wait for changes in the generator which would solve them, or we should change the way we generate async test.

fredericDelaporte · 2017-09-03T15:29:00Z

Tools/packages.config

@@ -7,5 +7,5 @@
  <package id="NUnit.Extension.NUnitV2ResultWriter" version="3.6.0" targetFramework="net461" />
  <package id="NUnit.Extension.TeamCityEventListener" version="1.0.2" targetFramework="net461" />
  <package id="NUnit.Extension.VSProjectLoader" version="3.6.0" targetFramework="net461" />
-  <package id="CSharpAsyncGenerator.CommandLine" version="0.3.6" targetFramework="net461" />
+  <package id="CSharpAsyncGenerator.CommandLine" version="0.3.7" targetFramework="net461" />


Required because some of this PR code was triggering bugs (1, 2) in Async Generator.

fredericDelaporte · 2017-09-03T15:39:43Z

src/AsyncGenerator.yml

@@ -109,6 +109,12 @@
    typeConversion:
    - conversion: Ignore
      name: EnumerableImpl
+    - conversion: Ignore
+      name: SystemTransactionContext
+      containingTypeName: AdoNetWithSystemTransactionFactory


Currently ignoring only serves one purpose, avoiding bloat async code generation. The transaction context is mostly used for handling callbacks from system transaction which are not async. But the generator was generating async private methods for some private members used by those callbacks.

It may have another purpose: the SystemTransactionContext.Wait perform a ManualResetEvent.Wait call which has an async counterpart but is currently not detected by the async generator (configuration?). If it were detected, this ignore configuration here for the whole class would anyway avoid it to be taken into account.

I think it should not be taken into account because this Wait occurs on restricted conditions: only when using scopes, furthermore distributed. Generating async for it would generate an async version of AbstractSessionImpl.CheckAndUpdateSessionStatus which would in turn generates async versions of many additional session members, while their calls would be still fully synchronous in usual cases.

ManualResetEvent.Wait call which has an async counterpart but is currently not detected by the async generator

There is no WaitAsync method for the ManualResetEvent or I am missing something?

My bad, previously it was a SemaphoreSlim instead of a ManualResetEventSlim. The semaphore has async methods, but indeed the reset does not, it just have some overloads taking cancellation token.

fredericDelaporte · 2017-09-03T15:43:30Z

src/AsyncGenerator.yml

+      containingTypeName: AdoNetWithSystemTransactionFactory
+    - conversion: Ignore
+      name: DependentContext
+      containingTypeName: AdoNetWithSystemTransactionFactory


I had some glitches not ignoring DependentContext nested class too, which anyway does not hold anything to convert to async (unless we change stance for the previous one). Sometime, the class was fully copied by Async Generator to the async partial of AdoNetWithSystemTransactionFactory, causing a compilation conflict with the sync partial.
This was not always occurring and I have not find a way to reproduce it with some test code, so I have not raised an issue on Async Generator. @maca88 may still be interested in knowing that there is that trouble though.

Strange, I tried to generate the code multiple times without ignoring those two nested types and the DependentContext was never generated

I have seen the trouble only when SystemTransactionContext is ignored but not DependentContext. In such case it sometimes takes it into account in a buggy way.

Ok, now I was able to reproduce and fix the problem.

fredericDelaporte · 2017-09-03T15:47:41Z

src/NHibernate.Test/Async/DynamicProxyTests/PeVerifier.cs

+	/// Contains generated async methods
+	/// </content>
+
+	public partial class PeVerifier


This is a strange async generator addition for which I do not have an explanation. It "disappears" if I exclude DeadLockHelper from async generation, although this other class does not relates to those PeVerifier and PeVerifyFixture. Anyway, they do not seem to harm.

This is also strange, I excluded the DeadlockHelper but the PeVerifier and PeVerifyFixture were generated. In order to exclude the StreamReader.ReadToEnd method add the following setting to the test project (must be inside analyzation setting):

ignoreSearchForAsyncCounterparts: - name: ReadToEnd

with this setting, the PeVerifier and PeVerifyFixture won't be generated as they were generated only because of the async version of the StreamReader.ReadToEnd method.

The main cause why PeVerifier is generated is because of this line. As ComponentTest is a fixture all its methods bodies will get scanned for async counterparts. When the ReadToEnd is found the generator checks all its usages and there is where PeVerifier is found.

fredericDelaporte · 2017-09-03T15:56:44Z

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

+				enlistment.Done();
+			}
+		}
+	}


Here we are lacking generation of child classes which does just override settings like UseConnectionOnSystemTransactionPrepare. Due to the async test class being generated as separated types instead of partials, this causes the tests corresponding to those different settings to not be asynchronously done.

Either we need the generator to generates async test class for descendant classes too (#43), or we need to switch generation back to the partial way. (Either for all then fixing broken fixtures or turning only failing ones to new types, or only for cases having such inheritance troubles.)
I would rather avoid a solution which would imply ad-hoc configuration for supporting this scheme, since it may let us forget some cases which would then lack async tests again.

fredericDelaporte · 2017-09-03T16:00:26Z

src/NHibernate.Test/NHSpecificTest/NH3023/DeadlockHelper.cs

+					{
+						try
+						{
+							using (var scope = new TransactionScope(TransactionScopeOption.RequiresNew))


Initial version was TransactionScopeOption.Requires instead of TransactionScopeOption.RequiresNew. Changed for async test sake: with TransactionScopeAsyncFlowOption.Enabled, the transaction from the test thread was propagated to the created thread, breaking the test logic.

fredericDelaporte · 2017-09-03T16:02:10Z

src/NHibernate.Test/NHSpecificTest/NH3023/DeadlockHelper.cs

+				//
+				// Notify partner that I have finished my work
+				//
+				myLock.Release();


Now put in a finally for avoiding deadlocking the test in case code preceding the release had failed, like it was occurring due to the unexpected transaction leak to second thread.

fredericDelaporte · 2017-09-03T16:03:54Z

src/NHibernate.Test/NHSpecificTest/NH3023/DeadlockHelper.cs

+			if (!partnerLock.Wait(120000))
+			{
+				throw new InvalidOperationException("Wait for partner has taken more than two minutes");
+			}


Now added a timeout for the wait in order to avoid deadlocking for one hour teamcity runner again with this test, if it were to bug again.

…ing non distributed tests

…mentation choices.

…dings.

…t prepare phase.

…whenever possible.

…ypos.

…users. * More xml documentation. * More virtual members. * More pre-conditions.

…fications.

fredericDelaporte · 2017-09-06T23:34:15Z

Rebased, regen, no more missing async tests.

fredericDelaporte force-pushed the NH-2176 branch 3 times, most recently from 27454a0 to 3ebb8cd Compare May 20, 2017 14:34

fredericDelaporte force-pushed the NH-2176 branch from 3ebb8cd to e8cdcf5 Compare May 20, 2017 14:42

fredericDelaporte force-pushed the NH-2176 branch 2 times, most recently from 0225ccd to 5760e16 Compare May 21, 2017 18:55

fredericDelaporte mentioned this pull request May 22, 2017

Fix race conditions when committing distributed transactions npgsql/npgsql#1571

Closed

fredericDelaporte force-pushed the NH-2176 branch from 99f96f5 to 2158f4a Compare May 23, 2017 15:51

fredericDelaporte mentioned this pull request May 26, 2017

NH-4015 - Update Npgsql driver and enable DTC for it #630

Merged

fredericDelaporte force-pushed the NH-2176 branch 7 times, most recently from 39c52a6 to d52d74a Compare May 29, 2017 12:54

fredericDelaporte commented May 29, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-2176 branch from 4451a41 to 532ee83 Compare May 29, 2017 15:19

fredericDelaporte mentioned this pull request Jun 5, 2017

NH-4028 - Supports inconclusive tests in result comparison #641

Merged

fredericDelaporte force-pushed the NH-2176 branch from 04eeab6 to 72a9db9 Compare June 11, 2017 21:33

fredericDelaporte commented Jun 18, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-2176 branch from 0fcfd6c to aea3577 Compare June 18, 2017 18:57

fredericDelaporte commented Jun 18, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-2176 branch 2 times, most recently from 1011410 to 276d494 Compare June 18, 2017 21:32

fredericDelaporte removed the to review label Sep 2, 2017

fredericDelaporte force-pushed the NH-2176 branch from 6f65c59 to 1c327d7 Compare September 3, 2017 15:24

fredericDelaporte commented Sep 3, 2017

View reviewed changes

griffitd and others added 15 commits September 7, 2017 01:23

NH-2176 - Add test case for consecutive transaction scopes.

7adde5b

NH-4011 - Fix current system transaction handling.

f0bd141

NH-4018 - Option for enlisting in system transaction.

c0767c3

NH-4011 - Moving DtcFailures to system transaction tests, and complet…

683b342

…ing non distributed tests

NH-4011 - MSDTC only tests, for grounding NHibernate enlistment imple…

a61dbc7

…mentation choices.

NH-4011 - Adjusting transaction handling according to MSDTC tests fin…

25e5c48

…dings.

NH-2928 - Option for not requiring connection availability from commi…

7f478eb

…t prepare phase.

NH-3023 - Test case for pool corruption after deadlock

cdb969d

NH-3023 - Cease closing connection from transaction scope completion …

8e0e467

…whenever possible.

Cleaning up transaction patterns in documentation, some more links, t…

6141f79

…ypos.

NH-4011 - Documenting transaction handling changes.

fa5b0a2

NH-4011 - Allowing easier customization of transaction factories for …

6ae4a1b

…users. * More xml documentation. * More virtual members. * More pre-conditions.

One more try in test teardown for avoiding hiding exception...

913a848

NH-4011 - Move from transaction completion event to second phase noti…

90a2bfe

…fications.

NH-4018 - Testing manual join in distributed cases too.

75d2c9f

fredericDelaporte force-pushed the NH-2176 branch from 1c327d7 to 75d2c9f Compare September 6, 2017 23:32

hazzik approved these changes Sep 6, 2017

View reviewed changes

fredericDelaporte removed the needs work label Sep 6, 2017

fredericDelaporte merged commit f4c2764 into nhibernate:master Sep 7, 2017

fredericDelaporte deleted the NH-2176 branch September 7, 2017 07:53

fredericDelaporte added the r: Fixed label Sep 7, 2017

lnu mentioned this pull request Dec 23, 2017

NH-3568 - Problem with TransactionScope #1321

Closed

fredericDelaporte mentioned this pull request Jul 12, 2018

NH-2237 - On transaction timeout, cleanup code throws an InvalidOperationException on SequencedHashMap.OrderedEnumerator.MoveNext #1216

Closed

fredericDelaporte mentioned this pull request Nov 20, 2018

Add new session context implementation for TransactionScope #1907

Open

fredericDelaporte mentioned this pull request Mar 7, 2019

Add transaction scope and distributed transaction tests nhibernate/nhibernate-envers#8

Open

fredericDelaporte mentioned this pull request Jan 21, 2024

InvalidOperationException on SequencedHashMap.OrderedEnumerator.MoveNext #3355

Closed

NH-4011 - transaction scope failures #627

NH-4011 - transaction scope failures #627

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

fredericDelaporte commented May 20, 2017

Uh oh!

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

Uh oh!

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!

fredericDelaporte May 29, 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 Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

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!

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!

fredericDelaporte Sep 4, 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 Sep 3, 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!

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!

fredericDelaporte commented Sep 6, 2017

Uh oh!

fredericDelaporte commented May 18, 2017 •

edited

Loading

fredericDelaporte commented May 20, 2017 •

edited

Loading

fredericDelaporte commented May 21, 2017 •

edited

Loading

fredericDelaporte May 29, 2017 •

edited

Loading

fredericDelaporte commented Jun 15, 2017 •

edited

Loading

fredericDelaporte commented Jun 16, 2017 •

edited

Loading

fredericDelaporte Sep 4, 2017 •

edited

Loading

fredericDelaporte Sep 3, 2017 •

edited

Loading