[WIP] Try fix ODP.NET DTC issues

[hazzik]

No description provided.

[lnu]

@hazzik What is the status of your pull request? We have this issue right now:

• NServicebus(5.2.19) handler with distributed transaction(msmq+oracle 12c client unmanaged driver)
• If the handler succeeds no issue
• If there is an exception, the Distributed transaction is rollbacked but it seems the oracle connection stays open. After a a few fails, the pool is exhausted and we cannot get a connection anymore. If pooling is disabled, the number of connections get higher and higher.
• We don't open an hibernate transaction inside the handler but rely on the fact on will be created automatically. We tried also to open one manually but it fails with an exception(I don't remember which one but I can reproduce it).

Do you think your pull request could solve that kind of issue?

[hazzik]

@lnu can you help me understand which connection leaks? I have oracle only in our test agent, and it's extremely hard to run memory profiling there.

[lnu]

I only have access to oracle at work and won't be there before next year. A simple repro should be to throw an exception inside a TransactionScope. By default the max pool is 100. If I loop 100 times and throw an exception each time the pool should be exhausted and you'll receive this exception: pooled connection request timeout. I'll try to do a repro.

ps: it occurs only with oracle, with sql server the connection is closed as expected(99% sure)

[lnu]

@hazzik It seems a lot better but from time to time I get an ObjectDisposedException

If it occurs multiple times it leads to a pool exhaust.

[hazzik]

thanks a lot, @lnu. Can you please show the full stack trace of the exception?

[lnu]

@hazzik I ran more tests(1 handler with 4 threads, 5000 messsages with 5 retries and 2 slr) and no exception. I've also configured odp.net tracing and the connection are opened and close correclty(in this case max 9 connections at peak). I guess I had the exception only when debugging with visual studio and having some breakpoints.
Do you think it could be merged and integrated in a hotfix release?

[hazzik]

@lnu which version did you check? Was it e145322 or 6c48231?

[lnu]

@hazzik e145322

[fredericDelaporte]

@lnu, I tend to think that the way NHibernate enlist session into ambient transaction is the major cause of the trouble.

May you try to reproduce the trouble with a modified transaction factory to validate that point?

The transaction factory can be change with configuration setting transaction.factory_class (NHibernate.Cfg.Environment.TransactionStrategy). The modified class I propose is:

// Unfortunately, cannot derive and override NHibernate impl, methods are not virtual.
public class CustomAdoNetTransactionFactory : ITransactionFactory
{
	private readonly AdoNetTransactionFactory _adoNetTransactionFactory =
		new AdoNetTransactionFactory();

	private readonly ConcurrentDictionary<DbConnection, System.Transactions.Transaction> _sessionsTransaction =
		new ConcurrentDictionary<DbConnection, System.Transactions.Transaction>();

	public void Configure(IDictionary props) { }

	public ITransaction CreateTransaction(ISessionImplementor session)
	{
		return new AdoTransaction(session);
	}

	public void EnlistInDistributedTransactionIfNeeded(ISessionImplementor session)
	{
		// No session enlistment. This disables automatic flushes before ambient transaction
		// commits. Explicit Flush calls required.
		// Still make sure the session connection is enlisted, in case it was acquired before 
		// transaction scope start.
		// Will not support nested transaction scope. (Will throw, while current NHibernate
		// just stay in previous scope.)
		// Will cause an "earlier than required" connection acquisition.
		// It is required to enlist with null when the scope is ended, otherwise using
		// the transaction without a new scope will fail by attempting to use it inside
		// the completed scope.
		// If an explicit transaction is ongoing, we must not enlist. We should not enlist
		// either if the connection was supplied by user (let him handle that in such case),
		// but there are currently no ways to know this from here.
		if (!session.ConnectionManager.Transaction.IsActive)
		{
			// Enlist is called terribly frequently, and in some circumstances, it will
			// not support to be called with the same value. So track what was the previous
			// call and do not call it again if unneeded.
			// (And Sql/OleDb/Odbc/Oracle manage/PostgreSql/MySql/Firebird/SQLite connections
			// support multiple calls with the same ongoing transaction, but some others may not.)
			var current = GetCurrentTransaction();
			var connection = session.Connection;
			System.Transactions.Transaction previous;
			if (!_sessionsTransaction.TryGetValue(connection, out previous) || previous != current)
			{
				_sessionsTransaction.AddOrUpdate(connection, current, (s, t) => current);
				if (current == null &&
					// This will need an ad-hoc property on Dialect base class instead.
					(session.Factory.Dialect is SQLiteDialect || session.Factory.Dialect is MsSqlCeDialect))
				{
					// Some connections does not support enlisting with null
					// Let them with their previous transaction if any, the application
					// will fail if the connection was left with a completed transaction due to this.
					return;
				}
				session.Connection.EnlistTransaction(current);
			}
		}
	}

	public bool IsInDistributedActiveTransaction(ISessionImplementor session)
	{
		// Avoid agressive connection release while a transaction is ongoing. Allow
		// auto-flushes (flushes before queries on dirtied entities).
		return GetCurrentTransaction() != null;
	}

	public void ExecuteWorkInIsolation(ISessionImplementor session, IIsolatedWork work,
		bool transacted)
	{
		using (var tx = new TransactionScope(TransactionScopeOption.Suppress))
		{
			// instead of duplicating the logic, we suppress the DTC transaction
			// and create our own transaction instead
			_adoNetTransactionFactory.ExecuteWorkInIsolation(session, work,
				transacted);
			tx.Complete();
		}
	}

	private System.Transactions.Transaction GetCurrentTransaction()
	{
		try
		{
			return System.Transactions.Transaction.Current;
		}
		catch (InvalidOperationException)
		{
			// This damn thing may yield an invalid operation exception (instead of null
			// or of a completed transaction) if we are between scope.Complete() and
			// scope.Dispose(). This happen when having completed the scope then disposing
			// the session and only after that disposing the scope.
			// Instead of testing System.Transactions.Transaction.Current here, storing in
			// connection manager the ambient transaction associated to the connection
			// (and updating it when enlisting) then checking that stored transaction would
			// reduce testes on Transaction.Current.
			return null;
		}
	}
}

The major consequence of this change will be that all sessions will need to be explicitly flushed before being disposed or before the transaction is committed, whichever come first. Otherwise, the changes will be lost (or done outside of a transaction if flushed after transaction commit).

I hope your app can accommodate with this.

If the application opens a session before a transaction scope, causes it to acquire a connection, then opens a transaction scope, the connection may not participate in it. But this is unlikely to occur, unless the connection release mode is OnClose (which is not the default).

Using this class could be a workaround for the trouble, without having to compile your own NHibernate version or wait for a next release.

[fredericDelaporte]

If you use second level cache, this transaction factory will cause it to be disabled when data is modified.

[hazzik]

I think it worth make a test PR to verify your assumptions

[fredericDelaporte]

Maybe only as a test coded in test project. I do not think adding this as a new transaction factory in the core would be suitable "as is". Will do.

[fredericDelaporte]

I have made a first test within #410.

[lnu]

By using TransactionScopeOption.Suppress, the session won't be involved in the current TransationScope. It means if it fails, the transaction won't be rollbacked?
Right now I have to call session.Flush() before the end of the TransactionScope otherwise nothing is saved in the database(autoflush issue?).
As I'm using it inside an NServiceBus handler(with msmq), the TransactionScope start/commit/rollback is handled by NServiceBus. It's easier If you just can open a session and be automatically enlisted if an ambient transaction is already started.
Moreover, I'm using Oracle and the behavior is different than its sql server counterpart.
Thanks for having a look at the issue a it's a sensible subject.

[fredericDelaporte]

By using TransactionScopeOption.Suppress, the session won't be involved in the current TransationScope. It means if it fails, the transaction won't be rollbacked?

It means everything happening inside this suppressed transaction scope will be outside the previously ambiant transaction. But once this suppressed transaction scope ends, the previous ambient transaction comes back. I guess you refer to ExecuteWorkInIsolation implementation: that is the same in current NHibernate default transaction factory. The aim of this method is to allow executing some statements on the database outside of the current ambient transaction if any ("in isolation"). This is used for retrieving a new high value when a hilo generator required it, by example.

Right now I have to call session.Flush() before the end of the TransactionScope otherwise nothing is saved in the database(autoflush issue?).

Is this a statement or a question?

With current NHibernate code (at least v3 and v4), you were not supposed to need explicit flushing before transaction scope completion with default flush mode (Auto) or with Commit flush mode. You can even dispose a session having changes in an ongoing transaction scope without flushing, it will be flushed at transaction commit.

With the modified transaction factory, flushing before transaction scope completion (or before session disposal) is required for changes to be persisted, provided the transaction scope ends up committed.

As I'm using it inside an NServiceBus handler(with msmq), the TransactionScope start/commit/rollback is handled by NServiceBus. It's easier If you just can open a session and be automatically enlisted if an ambient transaction is already started.

Under the hood the session connection is still enlisted in the transaction, since it is acquired within the transaction scope. So your operation on the session are transacted. The lack of session enlistment just mean there will be no more auto-flushing from transaction scope end.
There will be also no more auto-releasing of connection from session, meaning that if you use many transaction scope inside the life span of a session, the connection will not enlist in all of them. For avoiding this, I have made right now some additional change in the custom transaction factory above.

Moreover, I'm using Oracle and the behavior is different than its sql server counterpart.

And its behavior further depends on the used driver (managed or not).

[fredericDelaporte]

Now it works for all db in #410, for NH-2176 test. I am going to test it with other transaction scope tests. Code above updated.

[lnu]

Sorry I read your first comment a bit too fast.
For the flush, I have to call it because we use envers and if I don't, envers insert/update are not committed.

[fredericDelaporte]

@lnu, no need to try the custom transaction factory, it suffers from a major roadblock: some internal checks forbid actually closing a session within a transaction scope.

The current transaction factory does not suffer from it because it hijacks the session dispose for preventing it to actually close the session, then close it only at transaction completion.

The custom factory cease doing that, and cannot prevent the check forbidding closing the session within a transaction scope to get triggered.

This check is done by ConnectionManager, we need to change it, and this class usage is currently hard-coded in sessions (cannot ask NHibernate to use another one).

[hazzik]

Close in favor of #627