Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add transaction scope and distributed transaction tests #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

IwanowM
Copy link

@IwanowM IwanowM commented Mar 7, 2019

I have added few failing tests according to #7

The main problem is that Envers tries to perform database related work during BeforeTransactionCompletion event, but it is forbidden by NH if transaction.use_connection_on_system_prepare = false (check TransactionScopeWithoutUseConnectionOnPrepare).
For now, this problem takes place only in .NET Framework, but it seems, .net core already has support of ambient transaction (but without distributed transaction), so this problem will appear soon in .net Core

Performing database related work during BeforeTransactionCompletion also causes problems in case of distributed transaction, regardless of the values of the transaction.use_connection_on_system_prepare.

1st commit phase begins concurrently for all transaction managers, so it seems to me, there is a high probability, despite of EnlistDuringPrepareRequired, that when prepare phase in AdoNetWithSystemTransactionFactory starts, corresponding sql connection has already been locked by database transaction manager and, as a result, we can not use this connection any more.

Any way, to resolve this problem we need to move all database related work from DoBeforeTransactionCompletion to some place before TransactionScope dispose.

@IwanowM IwanowM marked this pull request as ready for review March 7, 2019 09:51
@IwanowM
Copy link
Author

IwanowM commented Mar 7, 2019

Regarding to distributed transaction and transaction.use_connection_on_system_prepare = true

On my local machine, this test ShouldNotThrowWhenDistributedTransaction throws (sometimes!) exception like

NHibernate.Exceptions.GenericADOException: could not insert: [NHibernate.Envers.DefaultRevisionEntity][SQL: INSERT INTO REVINFO (REVTSTMP) VALUES (?); select SCOPE_IDENTITY()] ---> System.Data.SqlClient.SqlException: Cannot enlist in the transaction because the transaction has already been committed or rolled back.

It is not clear for me, because when transaction.use_connection_on_system_prepare = true NH uses EnlistVolatile with EnlistmentOptions.EnlistDuringPrepareRequired.

According to docs.microsoft.com

When the caching resource gets its Prepare notification, it transfers its content to the durable resource. By doing so, the durable resource enlists on the transaction to become a participant of the 2 Phase Commit (2PC) protocol. Before this happens, only the caching resource (not the durable resource) was enlisted.

But, it seems to me, that durable resources (sql) have already enlisted to transaction (during open sql connection or hit to database like Flush()), because I can see that transaction is already distributed (transaction can not be promoted to distributed without durable resources) right there.

Maybe, @fredericDelaporte could describe a little bit more.

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Mar 7, 2019

But, it seems to me, that durable resources (sql) have already enlisted to transaction

Yes, on the second session usage, it goes to distributed when we have transaction.use_connection_on_system_prepare = true. This happens because the first session, though disposed, has actually delayed its disposal and has not released its connection to the pool, in order to be able to flush at transaction completion.
So when the second session has to open a connection, the connection pool is forced to yield a new connection instead of yielding the connection already enlisted in the transaction, causing the scope to escalate to distributed.
This is normal and to be expected, and this is in part why we recommend disabling transaction.use_connection_on_system_prepare, especially when sessions lifespan are shorter than transaction scopes lifespans. (When using many scopes consecutively in a session lifespan, no such issues occur. They occurs when using and disposing many sessions in a scope lifespan.)

About the occasional failures, they look to me as bugs of the dataprovider/MS-DTC/SQLServer transaction coordinator. (With SQL-Server, distributed transactions involving the same SQL-Server and no other durable resources are not actually coordinated by MS-DTC but offloaded to the SQL-Server. For other cases it may a bit more complicated.)
But really, we are here in muddy waters, because disposed sessions have not actually released their connections, and will use yet another one on transaction completion for flushing, then will release both connections at the end of the transaction completion process, if the session was disposed inside the scope. (This writing is in the case of SQL-Server; that is a dialect dependent behavior.)
This disposal of the first connection inside the transaction completion process is very debatable and said to be unsupported (I have not searched again the reference for this, it may be in NH-4011), but at that point we have no other place to do it.
Note that before 5.0, we were re-using the connection opened inside the scope instead of opening a new one. But at that point, that connection could be executing its own first phase processing concurrently.

During nhibernate/nhibernate-core#627, I have tried many things and I think I have also tried to actually release the connection at session disposal, but it was even more unstable as far as I remember. But maybe I am wrong and it should be tried again, at least for dialects now flagged with SupportsConcurrentWritingConnectionsInSameTransaction. (I may have tried that before introducing that property.)

Anyway, the safer would be to cease relying on additional DML interactions with the database during the 2PC, which means disabling transaction.use_connection_on_system_prepare and have some kind of support for this in Envers.

From @RogerKratz

About your last sentence... Is there a suitable ext point or similar for that in NH Core you mean?

Maybe the IInterceptor.PostFlush method could be used for that, or a custom FlushEvent.

using NHibernate.Envers.Tests.Entities;
using NUnit.Framework;

namespace NHibernate.Envers.Tests.Integration.Basic
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In NHibernate.Envers.Tests.Integration there is converted Hibernate Envers tests, so these tests suits better in NetSpecific.Integration instead.

{
}

#if NETCOREAPP2_0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe put the ignore on the complete class instead?

@RogerKratz
Copy link
Collaborator

RogerKratz commented Mar 7, 2019

Will try to catch up the org nhib core thread about this one tomorrow. Maybe I miss something obvious but...

From @fredericDelaporte

Maybe the IInterceptor.PostFlush method could be used for that, or a custom FlushEvent.

Sounds like a big change. In our current project eg, we call flush twice for each commit in our nhib abstraction (why? - too long story for this thread).

@RogerKratz
Copy link
Collaborator

@fredericDelaporte @IwanowM
Maybe it's a bit off topic for this PR and instead should be a discussion in NH Core but...
It seems Hibernate doesn't have this limitation. Distributed trans seems to work fine in combination with Before/AfterTransactionCompletionProcess. Trying to understand why these hooks doesn't work well in combination with MSDTC while they seem to work just fine with distr transactions in the hibernate/Java world. Is it the tran coordinator(s) themselfs, hibernate code or something else that make this difference?

@fredericDelaporte
Copy link
Member

I do not know how the Java transaction coordinator actually works, and why that works better on Hibernate side. But I think they are just widely different.

Maybe the Java one has some transaction completion event occurring before the 2PC actually starts, allowing to do additional changes in the transaction without advanced features like EnlistDuringPrepareRequired. Support of this feature considerably varies from a data-provider to another.
(Worst, that is the whole transaction scope support which already varies considerably between data-providers...
Npgsql does not even enlist as a durable resource, by example...
MS Sql-Server supports IPromotableSinglePhaseNotification while many other data-providers do not. Some have their own transaction coordinator service (like Oracle, and in some ways SQL-Server too since the server itself may coordinate the transaction instead of MsDTC).
Firebird and ODBC tends to always escalate to distributed, even if only a single connection is implied in the transaction.
Completion behavior between distributed and non-distributed scopes also varies: in the former, all first phases of enlisted resources, included volatile ones, runs on their own thread concurrently. Same for second phases, moreover concurrently to the transaction completed event and to code following the scope disposal, which runs without waiting for the end of the second phase. While in the later, everything runs sequentially on the scope disposal thread, no matter how many volatile resources are enlisted. And of course, code following the scope disposal does not run before all this is ended, in that later case.
That is a complete mess.)

Or maybe the Java transaction coordinator is just a lot more reliable and robust than Ado.Net combined with MsDtc and .Net data-providers specific implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants