Good work, Yves! IMO, this is one of the most important things to fix in NH. TransactionScope is the de facto standard for transactions everywhere in the .NET world.
RP On Saturday, April 26, 2014 9:46:44 PM UTC+1, ydi...@gmail.com wrote: > > I have been following those issues around TransactionScope for years now, > and I was again recently facing problems regarding TS and NH. So I decided > to finally have a look at the NH code involved in tx management, and wrote > some test cases to confirm some assumptions on the behavior of TS and the > DTC. I then found your post and I'm glad you summarized the situation well. > I agree with your 3 conclusions: unnecessary use of TxCompleted event > handler, flushing in Prepare() is bad, and NH session still needs to enlist > (mostly because of 2nd level cache, but not exclusively: entity locks, > connections release (?), firing interceptors). > > Regarding the proposed solutions, here are some comments: > > - no TransactionCompleted event handler: fully agree, it is redundant with > IEnlistmentNotification > > - no Flush from Prepare(): I would say no Flush() on a connection that has > already been enlisted in the same transaction. So that leaves us with 3 > options: Flush() from Dispose when in TxScope (as you also propose), > explicit Flush() required by app code (somehow breaks the current > programming model), or Flush from Prepare on another, new connection. The > latter is possible if I refer to your comment on the behavior of Prepare(): > we could enlist a new connection and flush on this one. Now the question > is: what is really a resource manager? A DB connection, or the DB server > engine behind it? Because in the second case, that option is not doable. A > drawback of the third option is also that there is a great chance that the > tx will be promoted to distributed if it wasn't already, plus we have to > open a new connection, and both are very costly. At first sight, I would > vote for Flush() from Dispose, with new FlushMode option, but it does not > solve the issue for code scenario 2. That's why we should maybe consider > flushing on another connection if possible? > > - I guess we will not be able to avoid all code in Commit/Rollback. I > agree with your idea of having a lock (thinking about a > ManualResetEventSlim indicating that no 2PC sequence is ongoing, that is > actually how I partly solved this issue on one of my recent projects). > However, there is a supplementary challenge here. Indeed, in case of tx > timeout, Rollback will be called asynchronously (on a thread pool's thread, > since a System.Threading.Timer is firing), without Prepare being called > first... Again, we have possible concurrent access to the session, since > user code may be running on another thread at the same time. That means > that we might need an additional lock on top of the one that would signal > an ongoing Prepare/Commit or Rollback sequence, because there is no Prepare > in case of timeouts. > > - of course AfterTransactionCompletion can (and will in case of > distributed tx) be called asynchronously > > - finally, if possible, I would like to make use of the occasion to make > things clear about the usage of TxScope and explicit NH transactions. There > has always been much confusion about their use together, and no definitive > answer: compatible? required to use Begintx with Txscope? why? That > confusion has moreover been exacerbated by the buggy support of TxScope in > NH. IMHO, an application should be designed to manage transactions using a > single, consistent API, and never mix two different tx APIs. So basically, > I would require the NH user to make a choice: either use TxScope (with the > appropriate TxFactory!), and then making calls to > ISession.BeginTransaction() forbidden (throw), or use BeginTx(), with a > txfactory that does not support TxScope, and will not enlist the session in > any ambient transaction (leave it up to the programmer if the underlying > ADO connection automatically enlists if there is an ambient tx, or > throw/warning if possible). > > I would be happy to help implementing a definitive solution, because I > have the impression that this issue has not received the attention it > deserves until now. This caused some loss of confidence in the NH user > community, as the TxScope API is a de facto standard in the .NET world (and > recommended over explicit tx management), and supporting it correctly is a > must IMHO. > > Regards, > > Yves > > On Sunday, November 24, 2013 8:04:52 PM UTC+1, Oskar Berggren wrote: >> >> I would guess that new behaviour should be implemented as a new >> transaction factory, so that the old sometimes-unstable behaviour can be >> used on a case-by-case basis. >> >> It should be feasible to have the session auto-flush on dispose if inside >> a transaction scope. This helps with the session-inside-transactionscope >> pattern, but not the other way around. (FlushMode.Commit would mean >> flush-on-session-dispose-if-inside-transaction). Possibly there should be >> new FlushModes to control this. >> >> It might also be feasible to have a dirty check on transaction scope >> disposal (i.e in the prepare phase) but instead of flushing to the >> database, it would just throw an error if any dirty state is found. This >> would be a double dirty check and could have a significant negative impact >> on performance. >> >> As for unavoidable... When it comes to the abilities of >> System.Transaction I have spent some time researching it. I could be wrong, >> or maybe there is a different strategy I haven't thought about. I would be >> very happy if anyone could come up with a better approach and prove me >> wrong. >> >> >> >> >> 2013/11/24 Michael Teper <mte...@gmail.com> >> >>> I would really like to have TransactionScope support fixed, but >>> requiring a manual Flush is a very high price to pay. We would need to >>> track down, fix, and test literally hundreds of places. Is this truly >>> unavoidable? >>> >>> Thanks! >>> -Michael >>> >>> >>> On Saturday, November 16, 2013 8:45:46 PM UTC-8, Oskar Berggren wrote: >>> >>>> We have several issues relating to TransactionScope documented in Jira, >>>> and there has been, over the years, multiple, sometimes heated, >>>> discussions >>>> on this in the mailing lists. The following is an attempt to summarize the >>>> abilities and possibilities. >>>> >>>> https://nhibernate.jira.com/issues/?jql=labels%20%3D% >>>> 20TransactionScope%20AND%20status%20in%20%28Open%2C%20% >>>> 22In%20Progress%22%2C%20Reopened%2C%20Unconfirmed%29<https://www.google.com/url?q=https%3A%2F%2Fnhibernate.jira.com%2Fissues%2F%3Fjql%3Dlabels%2520%253D%2520TransactionScope%2520AND%2520status%2520in%2520%2528Open%252C%2520%2522In%2520Progress%2522%252C%2520Reopened%252C%2520Unconfirmed%2529&sa=D&sntz=1&usg=AFQjCNE4qup8-JuSqUVyXundHxl2db2yZA> >>>> >>>> >>>> SUMMARY OF WHAT System.Transactions PROVIDES: >>>> Based on official documentation, blogs, and experiments. >>>> >>>> IEnlistmentNotification.Prepare() >>>> May be called on a thread different from the one calling >>>> Transaction.Dispose(). >>>> Transaction.Dispose() will not return until all enlisted Prepare() >>>> have completed. >>>> Can be used to enlist more resource managers, but it cannot touch >>>> already >>>> enlisted resource managers, since those may have already been >>>> prepared and >>>> could now be locked for further changes in waiting for the second >>>> phase. >>>> >>>> IEnlistmentNotification.Commit() >>>> May be called on a thread different from the one calling >>>> Transaction.Dispose(). >>>> Might not run until after Transaction.Dispose() has returned. >>>> >>>> IEnlistmentNotification.Rollback() >>>> May be called on a thread different from the one calling >>>> Transaction.Dispose(). >>>> I suspect it might not run until after Transaction.Dispose() has >>>> returned. >>>> Will not run at all if our own Prepare() initiated the rollback. >>>> >>>> Transaction.TransactionCompleted >>>> May be called on a thread different from the one calling >>>> Transaction.Dispose(). >>>> Might not run until after Transaction.Dispose() has returned. >>>> Called for both successful and unsuccessful transactions. >>>> Is documented as an alternative to a volatile enlistment: >>>> "You can register for this event instead of using a volatile >>>> enlistment to get outcome information for transactions." /MSDN >>>> The same MSDN docs also notes that using this event has a negative >>>> performance impact. >>>> >>>> >>>> >>>> CURRENT CODE STATE >>>> In NHibernate 3.3 (AdoNetWithDistributedTransactionFactory): >>>> >>>> AdoNetWithDistributedTransactionFactory adds a handler for the >>>> TransactionCompleted event and also implements IEnlistmentNotification. >>>> >>>> A session opened inside a TransactionScope will automatically enlist. >>>> It will not flush on dispose, but when the TransactionScope is disposed, a >>>> flush will be triggered from the Prepare() phase in >>>> AdoNetWithDistributedTransactionFactory. Surprisingly, this sometimes >>>> works. >>>> >>>> Depending on in what order things are done, whether or not the >>>> transaction was lightweight or distributed from the start, or if promotion >>>> have occurred etc., the code in Prepare() will sometimes hang for 20 >>>> seconds when trying to flush (waiting on a call to ADO.Net) and then abort >>>> (NH-2238). I believe this is because the ADO.Net connection is also a >>>> resource manager, enlisted with the transaction, and in some circumstances >>>> the transaction manager might have called ADO.Net's Prepare() before >>>> NHibernate's. Further attempts to use that connection then blocks while >>>> the >>>> connection is waiting for its Commit() or Rollback() notification. >>>> Generally speaking, I would say that while it is allowed to enlist more >>>> resource managers during the prepare phase, it is _not_ allowed to touch >>>> other already enlisted resource managers, because there is no ordering >>>> guarantee. >>>> >>>> The current code is written with the assumption that >>>> TransactionCompleted and Commit()/Rollback() is triggered before >>>> Transaction.Dispose() returns. They call >>>> ISessionImplementor.AfterTransactionCompletion() >>>> and CloseSessionFromDistributedTransaction(). This causes race >>>> conditions in the session if the user keeps working with it (NH-2176). It >>>> is also a problem for the ADO.Net connection, since that is also not >>>> thread >>>> safe and so while CloseSessionFromDistributedTransaction() is trying >>>> to close the connection, the transaction manager may have used a different >>>> thread to issue the commit phase on the connection. >>>> >>>> Since they may in fact be called later, this causes multi-thread issues >>>> with both the ISession and the ADO.Net connection. This is because this >>>> separate thread will call ISessionImplementor.AfterTransactionCompletion() >>>> and CloseSessionFromDistributedTransaction() which will touch the >>>> session (that the application itself may already use for something else >>>> (NH-2176)) and also try to close the connection, which might cause >>>> connection pool corruption (NH-3023). >>>> >>>> >>>> CONCLUSIONS >>>> * It is redundant to both implement IEnlistmentNotification and listen >>>> for TransactionCompleted. >>>> >>>> * Flushing changes to the ADO.Net connection in >>>> IEnlistmentNotification.Prepare(), >>>> as we currently do, IS SIMPLY NOT SUPPORTED, because we cannot touch the >>>> database connection since that is itself a resource manager already >>>> enlisted. >>>> >>>> * We might need to still enlist with the transaction to release second >>>> level cache locks. >>>> >>>> >>>> CODE SCENARIO 1: SESSIONS INSIDE TRANSACTION >>>> >>>> using (transactionscope) >>>> { >>>> // WITH EXPLICIT NH TRANSACTION. >>>> using (new session) >>>> using (session.BeginTransaction) >>>> { >>>> DoEntityChanges(); >>>> >>>> // This will flush the above changes, but leave the underlying >>>> db >>>> // connection and transaction open. >>>> tx.Commit(); >>>> } >>>> >>>> // WITHOUT EXPLICIT NH TRANSACTION. >>>> using (new session) >>>> { >>>> DoEntityChanges(); >>>> >>>> // This will flush the above changes, but leave the underlying >>>> db >>>> // connection and transaction open. >>>> session.Flush(); >>>> } >>>> >>>> // Because of the current flush during the prepare phase, this >>>> change might >>>> // be committed despite being performed outside a transaction. It >>>> might also >>>> // cause a deadlock. Without this change, the flush-in-prepare will >>>> have nothing >>>> // to do, and so will not cause a problem. >>>> entity.AdditionalChange() >>>> >>>> transactionscope.Complete(); >>>> } >>>> >>>> >>>> (Improved) Support for sessions inside TransactionScope is subject to >>>> the following limitations: >>>> * Changes must be flushed explicitly before reaching >>>> TransactionScope.Dispose(). >>>> >>>> * We need to fix the code so that we also never touch the ADO.Net >>>> connection after >>>> TransactionScope.Dispose() has been reached. To achieve this, the >>>> session should >>>> release the connection no later than its own Dispose(). NH-2928. This >>>> would also >>>> reduce the need for transaction promotion when two non-overlapping >>>> sessions are >>>> used within the same TransactionScope. >>>> >>>> * If desired, it should be possible to have the session auto-flush from >>>> its Dispose() >>>> when used inside a TransactionScope (NH-2181). Drawback is that the >>>> session >>>> would behave very differently compared to not using TransactionScope >>>> (increased >>>> complexity). >>>> >>>> * Entity changes performed outside an active transaction would not be >>>> committed, >>>> just as when not using TransactionScope. Unless the entity is >>>> reattached to a >>>> new session. >>>> >>>> >>>> >>>> CODE SCENARIO 2: TRANSACTIONS INSIDE SESSION >>>> >>>> using (new session) >>>> { >>>> using (new transactionscope) >>>> { >>>> DoEntityChanges(); >>>> >>>> transactionscope.Complete(); >>>> } >>>> >>>> // Unknown amount of time before the transaction commit phase is >>>> done with the session. >>>> >>>> using (new transactionscope) >>>> { >>>> DoEntityChanges(); >>>> >>>> transactionscope.Complete(); >>>> } >>>> } >>>> >>>> >>>> (Improved) Support for TransactionScope inside sessions is subject to >>>> the following limitations: >>>> * We simply cannot rely on the TransactionScope to tell us when to >>>> flush, so again this can >>>> only work with explicit flush (either Flush() or Commit() on NH's >>>> transaction). >>>> >>>> * To support consecutive TransactionScopes (NH-2176), or really any use >>>> of the session after we have reached >>>> Dispose() on the first TransactionScope, the application must be able >>>> to reliable detect when the >>>> out-of-band Commit()/Rollback in IEnlistmentNotification have >>>> completed. >>>> >>>> >>>> >>>> COMPARISON WITH OTHER SOFTWARE >>>> It has been claimed that other software "works with System.Transactions >>>> automatically and doesn't require the use of something like NH's >>>> ITransaction". These statements seem to be both correct and wrong. Looking >>>> at eg. Linq2SQL and Entity Framework, it's true that they have no >>>> counterpart to NH's ITransaction. However, they do require explicit calls >>>> to e.g. SaveChanges() before TransactionScope.Complete() and Dispose() is >>>> called. So they too have no automatic flush-on-TransactionScope.Dispose() >>>> behaviour. >>>> >>>> Within the context of a TransactionScope, one can perceive the >>>> ITransaction.Commit() as one possibly way to trigger the explicit flush of >>>> changes. The other one is of course session.Flush(). So it's really not >>>> that different. >>>> >>>> >>>> >>>> IDEAS FOR PROPOSED SOLUTION >>>> * Use only IEnlistmentNotification, not TransactionCompleted. >>>> >>>> * Remove the flushing from IEnlistmentNotification.Prepare(). The >>>> application must flush earlier or loose the changes. >>>> >>>> * If we cannot move all code from the Commit/Rollback/TransactionCompleted >>>> stage into Prepare(), >>>> we must instead grab some sort of lock on the session at the start of >>>> Prepare(). The lock would be >>>> released at the end of the Commit or Rollback phase. In the meantime, >>>> all other attempts to use >>>> the session should block waiting for the lock to be released. (I >>>> suspect this is what happens when use of >>>> the ADO.Net connection blocks when flushing from Prepare().) >>>> >>>> * Document that IInterceptor.AfterTransactionCompletion() and similar >>>> may be called >>>> asynchronously on a different thread. >>>> >>>> * Optionally, we can also add automatic flush in the session's >>>> Dispose() method if an ambient >>>> transaction is detected. >>>> >>>> >>>> I'm still a bit unclear on the interaction with the second level cache. >>>> >>>> >>>> /Oskar >>>> >>>> -- >>> >>> --- >>> You received this message because you are subscribed to the Google >>> Groups "nhibernate-development" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to nhibernate-development+unsubscr...@googlegroups.com. >>> For more options, visit https://groups.google.com/groups/opt_out. >>> >> >> -- --- You received this message because you are subscribed to the Google Groups "nhibernate-development" group. To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-development+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
