Hello NH team! I started my journey in the world of System.Transactions and in the inner workings of NH. After careful review of all issues, I identified 3 areas that need changes and / or improvements: connection management, thread safety of the session and Flush() timing.
I decided to start working on connection management first, and what I'm going to implement is a close of the connection when closing (or disposing of) the session, even when an ambient tx is active. As stated in MSDN, this should not be an issue (see http://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlconnection.close.aspx). This is the heart of the discussion in NH-2928, but deferring connection close causes other issues as well (concurrency caused by async close). I started writing a new transaction factory, but unfortunately, it seems difficult to support the current, buggy AdoNetWithDistributedTransactionFactory together with a new one that would fix the issues. That's why I had to drop the current tx factory. Hope this is not a too big issue. Any comments on this? Regards, Yves On Monday, 28 April 2014 14:24:24 UTC+2, Johannes Gustafsson wrote: > > I've also been following this thread for a while and I would like to add > my support for a fix. We are getting more and more problems due to the > issues with DTC (crashes, connection leaks etc), so a fix for this would be > very much appreciated. > > I'm not sure I can be of any help regarding core development, but if you > need any help with testing or anything else, then I will try to do my best. > > However, regarding flush behaviour: IMHO, if you create a NH session > inside a scope it should be the users responsibility to make sure Flush is > called. Just like it's the users responsibility to call Commit if you use a > TX within a session. If the tx scope is created within the session, then > maybe the tx.Complete() should trigger a flush, but not the other way > around. Just my 2 cents. > > Anyway, I really appreciate the hard work you're putting into this. > > Regards, > Johannes > > > 2014-04-27 11:59 GMT+02:00 Ricardo Peres <[email protected] <javascript:>>: > >> 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, [email protected] 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 <[email protected]> >>>> >>>>> 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%20Trans >>>>>> actionScope%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 [email protected]. >>>>> 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 [email protected]<javascript:> >> . >> For more options, visit https://groups.google.com/d/optout. >> > > -- --- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
