NHibernate, Flush, TransactionScope

Topics: Core
Feb 17, 2013 at 8:33 PM
Hello everyone,

I hope some experienced Orchard people, especially contributors, will take part in this discussion, as I have made some interesting observations regarding transaction handling in Orchard and would really like to know if it is intentional.
  1. I started out by looking for a reason why my queries were returning stale data, i.e. still returning records that had been IRepository<T>.Deleted. It turns out, that NHibernate only does automatic session flushing if running in a transaction:
    NHibernate FlushMode Auto Not Flushing Before Find
    However, Orchard does run in a transaction. The reason flushing still isn't performed is that NHibernate does not consider a TransactionScope for enabling autoflushing:
    NHibernate 3.0: TransactionScope and Auto-Flushing
    The solution people are suggesting is to simply do ISession.BeginTransaction in addition to a TransactionScope. I tried fixing Orchard's TransactionManager this way, and it seems to work, however it also required changing ITransactionManager interface to allow passing of the ISession object from SessionLocator. Would you consider taking this fix in? This seems a very risky change, but the alternative of being forced to use Flush all over the place or risk data inconsistencies feels even worse.
  2. As a side-note, why is there an IInterceptor on the session created by SessionLocator? Also, why does ISessionLocator.For take the entity type as a parameter but ignore it altogether?
  3. There is also a second issue. Using a TransactionScope forces a distributed transaction everytime a second connection is initiated. Have you ever considered having this configurable somehow? I believe there are valid reasons to run 2 connections in a single transaction, but also to call external services without the need for a DTC, in separate transactions.
Best regards,
Marcin
Developer
Feb 17, 2013 at 9:31 PM
Thanks for sharing your observations.

We are currently considering removing TransactionScope altogether and use NHibernate's Transaction object directly.
This will resolve a couple of issues, perhaps the ones you mentioned as well.
Developer
Feb 17, 2013 at 10:34 PM
Hey Marcin, Those are the same observations made by Oren are they not? on the TekPub podcast?
Coordinator
Feb 18, 2013 at 1:56 AM
They are.
Feb 18, 2013 at 9:07 AM
Heh, wasn't aware of any podcasts :) Good to hear that there is some work being done on this. For the record here are my quite simple changes:
    public interface IFixTransactionManager : ITransactionManager, IDisposable
    {
        void Demand(ISession session);
        void Enlist(IDbCommand command);
    }

    [OrchardSuppressDependency("Orchard.Data.TransactionManager")]
    public class FixTransactionManager : IFixTransactionManager
    {
        private readonly bool useScope;
        private readonly bool useTransaction;
        private TransactionScope scope;
        private ITransaction transaction;
        private bool cancelled;

        public FixTransactionManager()
            : this(true, true)
        {
        }

        public FixTransactionManager(bool useScope, bool useTransaction)
        {
            this.useScope = useScope;
            this.useTransaction = useTransaction;
            Logger = NullLogger.Instance;
        }

        public ILogger Logger { get; set; }

        public void Demand(ISession session)
        {
            if (scope == null && useScope)
            {
                Logger.Debug("Creating transaction scope as demanded");

                scope = new TransactionScope(
                    TransactionScopeOption.Required,
                    new TransactionOptions
                    {
                        IsolationLevel = IsolationLevel.ReadCommitted
                    });
            }

            if (transaction == null && useTransaction)
            {
                Logger.Debug("Creating transaction as demanded");
                transaction = session.BeginTransaction();
            }
        }

        public void Demand()
        {
            throw new NotSupportedException();
        }

        public void Enlist(IDbCommand command)
        {
            if (scope == null && transaction != null)
                transaction.Enlist(command);
        }

        public void Cancel()
        {
            Logger.Debug("Transaction cancelled flag set");
            cancelled = true;
        }

        public void Dispose()
        {
            if (transaction != null)
            {
                if (!cancelled)
                {
                    Logger.Debug("Marking transaction as complete");
                    transaction.Commit();
                }
                else
                {
                    Logger.Debug("Rolling transaction back");
                    transaction.Rollback();
                }

                Logger.Debug("Final work for transaction being performed");
                transaction.Dispose();
                Logger.Debug("Transaction disposed");
                transaction = null;
            }

            if (scope != null)
            {
                if (!cancelled)
                {
                    Logger.Debug("Marking transaction scope as complete");
                    scope.Complete();
                }

                Logger.Debug("Final work for transaction scope being performed");
                scope.Dispose();
                Logger.Debug("Transaction scope disposed");
                transaction = null;
                scope = null;
            }
        }
As you can see this code actually lets you decide which transaction handling you want. Good for experimenting. The Enlist method is neccessary for DefaultDataMigrationManager to work without a TransactionScope.

And to support all that, in SessionLocator:
        public ISession For(Type entityType)
        {
            Logger.Debug("Acquiring session for {0}", entityType);

            if (_session == null)
            {

                var sessionFactory = _sessionFactoryHolder.GetSessionFactory();

                Logger.Information("Openning database session");
                _session = sessionFactory.OpenSession(new SessionInterceptor());
                _transactionManager.Demand(_session);
            }
            return _session;
        }
Perhaps slightly off-topic, but there are still the questions in point no. 2 - could anyone share some thoughts on that?
Coordinator
Feb 19, 2013 at 12:18 AM
Sipke, can you also share your implementation to see if it fulfills Marcin's request ?
Developer
Feb 19, 2013 at 12:38 AM
Edited Feb 19, 2013 at 12:40 AM
Yes, I am maintaining a fork here: (http://orchard.codeplex.com/SourceControl/network/forks/sfmskywalker/transactionscope?branch=transaction)


The code works on first sight, but I started to see a strange issue where items did not get saved, so beware, it has not yet been fully tested and there are known issues with the current implementation.

Especially the BackgroundService, it's completing any current transaction for each task, but I'm not sure if there could potentially be another thread accessing the same transaction manager. If it does, then this obviously is going to cause weird behavior.

SessionLocator
using System;
using NHibernate;
using Orchard.Logging;

namespace Orchard.Data {
    public class SessionLocator : Component, ISessionLocator {
        private readonly ITransactionManager _transactionManager;
        private ISession _session;

        public SessionLocator(ITransactionManager transactionManager) {
            _transactionManager = transactionManager;
        }

        public ISession For(Type entityType) {
            Logger.Debug("Acquiring session for {0}", entityType);

            if (_session == null) {
                _transactionManager.Demand();
                Logger.Information("Openning database session");
                _session = _transactionManager.GetSession();
            }
            return _session;
        }
    }
}
TransactionManager
using System;
using System.Collections;
using System.Web.Mvc;
using NHibernate;
using NHibernate.SqlCommand;
using NHibernate.Type;
using Orchard.Logging;
using Orchard.Mvc.Filters;
using IsolationLevel = System.Data.IsolationLevel;

namespace Orchard.Data {
    public interface ITransactionManager : IDependency {
        void Demand();
        void Cancel();
        ISession GetSession();

        /// <summary>
        /// Commits any current transaction and starts a new one
        /// </summary>
        void StartNewTransaction();
    }

    public class TransactionManager : ITransactionManager, IDisposable {
        private ITransaction _transaction;
        private bool _cancelled;
        private readonly ISessionFactoryHolder _sessionFactoryHolder;
        private ISession _session;

        public TransactionManager(ISessionFactoryHolder sessionFactoryHolder) {
            _sessionFactoryHolder = sessionFactoryHolder;
            Logger = NullLogger.Instance;
        }

        public ILogger Logger { get; set; }

        void ITransactionManager.Demand() {
            if(_cancelled && _transaction != null) {
                try {
                    _transaction.Dispose();
                }
                catch {
                    // swallowing the exception
                }

                _transaction = null;
            }

            if (_transaction == null) {
                Logger.Debug("Creating transaction on Demand");
                var session = GetSession();
                _transaction = session.BeginTransaction(IsolationLevel.ReadCommitted);
            }
        }

        /// <summary>
        /// Commits any current transaction and starts a new one
        /// </summary>
        void ITransactionManager.StartNewTransaction() {
            if (_transaction != null) {
                try {
                    if (_cancelled)
                        _transaction.Dispose();
                    else {
                        _transaction.Commit();
                        _session.Flush();
                    }
                }
                catch {
                    // swallowing the exception
                }

                _transaction = null;
            }

            Logger.Debug("Starting new transaction");
            var session = GetSession();
            _transaction = session.BeginTransaction(IsolationLevel.ReadCommitted);
        }
        
        public ISession GetSession() {
            if (_session == null) {
                var factory = _sessionFactoryHolder.GetSessionFactory();
                _session = factory.OpenSession(new SessionInterceptor());
            }
            return _session;
        }

        void ITransactionManager.Cancel() {
            Logger.Debug("Transaction cancelled flag set");
            _cancelled = true;
        }

        void IDisposable.Dispose() {
            if (_transaction != null) {
                if (!_cancelled) {
                    Logger.Debug("Marking transaction as complete");
                    _transaction.Commit();
                    _session.Flush();
                }

                Logger.Debug("Final work for transaction being performed");
                try {
                    _transaction.Dispose();
                    _session.Close();
                    _session.Dispose();
                }
                catch {
                    // swallowing the exception
                }
                Logger.Debug("Transaction disposed");
            }
        }

        
    }
BackgroundService
using System;
using System.Collections.Generic;
using System.Linq;
using JetBrains.Annotations;
using Orchard.Data;
using Orchard.Events;
using Orchard.Logging;

namespace Orchard.Tasks {

    public interface IBackgroundService : IDependency {
        void Sweep();
    }

    [UsedImplicitly]
    public class BackgroundService : IBackgroundService {
        private readonly IEnumerable<IEventHandler> _tasks;
        private readonly ITransactionManager _transactionManager;

        public BackgroundService(IEnumerable<IEventHandler> tasks, ITransactionManager transactionManager) {
            _tasks = tasks;
            _transactionManager = transactionManager;
            Logger = NullLogger.Instance;
        }

        public ILogger Logger { get; set; }

        public void Sweep() {
            foreach(var task in _tasks.OfType<IBackgroundTask>()) {
                try {
                    _transactionManager.StartNewTransaction();
                    task.Sweep();
                }
                catch (Exception e) {
                    Logger.Error(e, "Error while processing background task");
                }
            }
        }
    }
}
Feb 19, 2013 at 2:47 PM
I haven't gone into BackgrountService yet with my attempts....

An important question to raise here is: does any specification exist for the behaviour of ITransactionManager? In terms of its interface?

The old TransactionManager.Demand method basically ensures that a transaction exists, but in Sipke's fork it also handles cancelled transactions - this is a huge change. Are there any guidelines on using ITransactionManager.Demand? Should it be used my "consumer" code or are all the usages under strict control? They are for now, as far as I can see, only Cancel is being called by quite a few places.

As for threading, I am now fighting some issues because of our bad Quartz integration where I work and can say with all confidence that any sharing of sessions or transactions between threads is a recipe for disaster...
Coordinator
Feb 25, 2013 at 7:46 PM
Will check this code and the usage of Demand()

What I can say is that a session is not thread safe. A ITransactionManager is an IDependency so there is no way two threads could share it. Even with Background tasks which are executed in their own threads. Unless you do something not supported, then please explain what you try to accomplish.
Coordinator
Feb 25, 2013 at 11:44 PM
I pushed a new ITransactionManager implementation. It is now a common class for ISessionLocator and ITransactionManager. This is no more using TransactionScope as there are bugs related to async operation in its implementation. It also enables the NHibernate Auto Flush mode by default.

Demand() is called automatically when requesting a new Session, such that anything in a Session is under a transaction. There is also a new method called RequireNew() which lets you request a new Transaction, committing the previous one if it was not cancelled. This might break some Orchard customization, but it should only impact advanced users which will recover easily from it.
Developer
Feb 25, 2013 at 11:51 PM
Looking good. I'll try it out tomorrow and let you know.
Coordinator
Feb 26, 2013 at 1:15 AM
Could this improve import further, by letting you break it into smaller transactions?
Coordinator
Feb 26, 2013 at 1:21 AM
If you call RequiresNew() it will commit the previous one, so actually make smaller ones. Is it what you intended ?
Coordinator
Feb 26, 2013 at 1:32 AM
Yes: one of the things that makes some imports slow, besides the identity lookups (but that was fixed), is the memory taken by the ginormous transaction. This should mitigate it.
Coordinator
Feb 26, 2013 at 1:53 AM
No. Actually it's the first level cache from NHibernate, and there is already a Clear() on the Content Manager to mitigate that.
Unless NH clears the first level cache on transaction commits, which I don't know.
Coordinator
Feb 26, 2013 at 7:23 AM
Ah, ok.
Feb 26, 2013 at 7:54 AM
Edited Feb 26, 2013 at 8:31 AM
Could I ask if transactions are mandatory for NHibernate ?
Just some ideas:
  • Have a global switch: Do not do NHibernate transaction in settings
  • Have the possibility to have a transactionless session.
  • Have a possible transactionless NHibernate operation inside the current transaction
  • Have a transactionless object (interface ?) which never get enlisted in a transaction.
Another question, is NHibernate managing imbricated levels of transactions (#names) and correctly managing imbricated rollback ?
Last one if we want to use NHibernate to access another DB (external) than Orchard'one, is it a valid option and if so are these exteranal transactions enlisted in Orchard's local transaction.
Coordinator
Feb 26, 2013 at 8:27 AM
You can ask, but the answer is that yes, transactions should be used. There will never be such a switch. Read this: http://ayende.com/blog/3775/nh-prof-alerts-use-of-implicit-transactions-is-discouraged
Feb 26, 2013 at 9:07 AM
Edited Feb 26, 2013 at 9:18 AM
Sorry but I know the way SLQ DBs work with transactions since many years, especially SQL Server which I have been working on before Microsoft bought it, and that's the reason why I am asking.
This article is mixing some different concepts: the atomic transaction which is automatic and you manage usually in store procedures individually with optimisation hints on each statement inside the transaction, and the global transaction.
No SQL DB makes automatically a global transaction, they do atomic transactions which are related to locking occuring in one startement. The locking is usually depending on the global strategy you adopt according the level of risk you accept (collision/isolation and shadowing)
I large DBs with many small tables and multiples access and small updates, global transactions are a great factor of locks. This is the reason why the less you create transactions the better it is.
This is typically the case for a web site, especially with NHibernates which creates so many tables....and is unable to use the only very efficient tool in SQL DB : the store procedures and their dynamic optimizer.
Feb 26, 2013 at 9:24 AM
Edited Feb 26, 2013 at 12:21 PM
Last point, may be I have misunderstood but it seems that the Orchard DB is using a central counter (table contentitem ?) to allocate all the record numbers, each item being indexed based on this kind of unique counter ?
This is a real source of lock problems, and it may be interesting to have a dedicated access to this avoiding transactions locks on it, but I may have badly interpreted what I see in tables ?
Coordinator
Feb 26, 2013 at 5:13 PM
We don't have a choice: we are building composite objects from many tables that are potentially queried from many different places in the code. You don't want the different parts to be inconsistent, which would happen if we weren't reading in transactions.

The only really good solution is to move to a document database.
Feb 26, 2013 at 5:25 PM
Edited Feb 26, 2013 at 5:25 PM
One choice could be to isolate the counter /index generator from Nhibernate...
Concerning Document DB this could be very interesting but you will loose 80% of the actual 'customers' of Orchard for a very selected population...
I am afraid that, at that time, a code fork occurs....
Coordinator
Feb 26, 2013 at 5:48 PM
Bertrand didn't mention that the document db we want to use is pure SQL actually, in order not to comply with the current skillset of our customers as you say.
c.f. https://github.com/sebastienros/yessql
Think RavenDB but using Tables instead of Lucene indexes.
Feb 26, 2013 at 6:30 PM
Let's go
Coordinator
Feb 26, 2013 at 6:40 PM
Do you mean that sells it to you? We're really interested to hear your thoughts about this.
Feb 26, 2013 at 7:02 PM
Edited Feb 26, 2013 at 7:24 PM
I think that the move will be audacious, it could be interesting to highlight what are the targets.
Targeting only a web site +CMS fonctionality could be limited,
Nowdays the Cloud, the mobile, the network are changing the landscape (Big Data I don't know, marketing ?) as I have only seen on the end of the 2000s.

Document concept today is not linked to a single time and place unity... Orchard hability to assemble various parts in contentype looks very good for something greater than a web site could provide, if to do this we need a better engine than the old SQL, let's go.
But it could be interesting to keep an old SQL thread for more traditional needs.
...you asked, for, you get it, ( it could be better explained in my mother tongue).
Coordinator
Feb 26, 2013 at 7:07 PM
Thanks. The possibility to mingle document data with relational is intriguing. I'm not sure it can be done in a clean way, but we may have to find out.
Feb 26, 2013 at 7:25 PM
Edited Feb 26, 2013 at 10:24 PM
Could be a fork.
I had on look on YesSql from Sebastien'samples, it's very interesting,, the Lucene usage to build indexes is some way of isotating the index tasks from normal DB tasks.
May 7, 2014 at 9:37 AM
Just found out about the breaking change regarding the auto flush.

We depend at certain places in the code that it doesnt flush while doing a query.

Before 1.7 nothing got flushed, now it does...

So we created a wrapper for an ISession that will temp change the flush mode to 'commit' and then back once disposed.

But again, this was a breaking change (things behaved differently before), was this informed somewhere?