Using DI to add Interceptor to NHibernate Sessions in legacy code

Question

So, there's a bug in some legacy code I'm maintaining. It causes some mild data corruption, so it's rather serious. I've found the root cause, and have made a sample application that reliable reproduces the bug. I would like to fix it with as little impact on existing applications as possible, but I'm struggling.

The bug lies in the data access layer. More specifically, in how an interceptor is injected into a new Nhibernate Session. The interceptor is used to set a specific entity property when saving or flushing. The property, LoggedInPersonID, is found on nearly all our entities. All entities are generated from CodeSmith templates using the database schema, so the LoggedInPersonID property corresponds to a column that is found on nearly all tables in the database. Together with a couple of other columns and triggers, it is used to keep track of which user created and modified a record in the database. Any transaction that inserts or updates data need to supply a LoggedInPersonID value, or else the transaction will fail.

Whenever a client requires a new session, a call is made to OpenSession in the SessionFactory (not Nhibernate's SessionFactory, but a wrapper). The code below shows the relevant parts of the SessionFactory wrapper class:

public class SessionFactory
{
    private ISessionFactory sessionFactory;

    private SessionFactory()
    {
        Init();
    }

    public static SessionFactory Instance
    {
        get
        {
            return Nested.SessionFactory;
        }
    }

    private static readonly object _lock = new object();

    public ISession OpenSession()
    {
        lock (_lock)
        {
            var beforeInitEventArgs = new SessionFactoryOpenSessionEventArgs(null);

            if (BeforeInit != null)
            {
                BeforeInit(this, beforeInitEventArgs);
            }

            ISession session;

            if (beforeInitEventArgs.Interceptor != null
                && beforeInitEventArgs.Interceptor is IInterceptor)
            {
                session = sessionFactory.OpenSession(beforeInitEventArgs.Interceptor);
            }
            else
            {
                session = sessionFactory.OpenSession();
            }

            return session;
        }
    }

    private void Init()
    {
        try
        {
            var configuration = new Configuration().Configure();
            OnSessionFactoryConfiguring(configuration);
            sessionFactory = configuration.BuildSessionFactory();
        }
        catch (Exception ex)
        {
            Console.Error.WriteLine(ex.Message);
            while (ex.InnerException != null)
            {
                Console.Error.WriteLine(ex.Message);
                ex = ex.InnerException;
            }
            throw;
        }
    }

    private void OnSessionFactoryConfiguring(Configuration configuration)
    {
        if(SessionFactoryConfiguring != null)
        {
            SessionFactoryConfiguring(this, new SessionFactoryConfiguringEventArgs(configuration));
        }
    }

    public static event EventHandler<SessionFactoryOpenSessionEventArgs> BeforeInit;
    public static event EventHandler<SessionFactoryOpenSessionEventArgs> AfterInit;
    public static event EventHandler<SessionFactoryConfiguringEventArgs> SessionFactoryConfiguring;

    public class SessionFactoryConfiguringEventArgs : EventArgs
    {
        public Configuration Configuration { get; private set; }

        public SessionFactoryConfiguringEventArgs(Configuration configuration)
        {
            Configuration = configuration;
        }
    }

    public class SessionFactoryOpenSessionEventArgs : EventArgs
    {

        private NHibernate.ISession session;

        public SessionFactoryOpenSessionEventArgs(NHibernate.ISession session)
        {
            this.session = session;
        }

        public NHibernate.ISession Session
        {
            get
            {
                return this.session;
            }
        }

        public NHibernate.IInterceptor Interceptor
        {
            get;
            set;
        }
    }

    /// <summary>
    /// Assists with ensuring thread-safe, lazy singleton
    /// </summary>
    private class Nested
    {
        internal static readonly SessionFactory SessionFactory;

        static Nested()
        {
            try
            {
                SessionFactory  = new SessionFactory();
            }
            catch (Exception ex)
            {
                Console.Error.WriteLine(ex);
                throw;
            }
        }
    }
}

The interceptor is injected through the BeforeInit event. Below is the interceptor implementation:

public class LoggedInPersonIDInterceptor : NHibernate.EmptyInterceptor
{
    private int? loggedInPersonID
    {
        get
        {
            return this.loggedInPersonIDProvider();
        }
    }

    private Func<int?> loggedInPersonIDProvider;

    public LoggedInPersonIDInterceptor(Func<int?> loggedInPersonIDProvider)
    {
        SetProvider(loggedInPersonIDProvider);
    }

    public void SetProvider(Func<int?> provider)
    {
        loggedInPersonIDProvider = provider;
    }

    public override bool OnFlushDirty(object entity, object id, object[] currentState, object[] previousState,
                                        string[] propertyNames, NHibernate.Type.IType[] types)
    {
        return SetLoggedInPersonID(currentState, propertyNames);
    }

    public override bool OnSave(object entity, object id, object[] currentState,
                          string[] propertyNames, NHibernate.Type.IType[] types)
    {
        return SetLoggedInPersonID(currentState, propertyNames);
    }

    protected bool SetLoggedInPersonID(object[] currentState, string[] propertyNames)
    {
        int max = propertyNames.Length;

        var lipid = loggedInPersonID;

        for (int i = 0; i < max; i++)
        {
            if (propertyNames[i].ToLower() == "loggedinpersonid" && currentState[i] == null && lipid.HasValue)
            {
                currentState[i] = lipid;

                return true;
            }
        }

        return false;
    }
}

Below is a helper class used by applications to register a BeforeInit event handler:

public static class LoggedInPersonIDInterceptorUtil
    {
        public static LoggedInPersonIDInterceptor Setup(Func<int?> loggedInPersonIDProvider)
        {
            var loggedInPersonIdInterceptor = new LoggedInPersonIDInterceptor(loggedInPersonIDProvider);

            ShipRepDAL.ShipRepDAO.SessionFactory.BeforeInit += (s, args) =>
            {    
                args.Interceptor = loggedInPersonIdInterceptor;
            };

            return loggedInPersonIdInterceptor;
        }
    }
}

The bug is especially prominent in our web services (WCF SOAP). The web services endpoint bindings are all basicHttpBinding. A new Nhibernate session is created for each client request. The LoggedInPersonIDInterceptorUtil.Setup method is called after a client is authenticated, with the authenticated client's ID captured in the closure. Then there's a race to reach code that triggers a call to SessionFactory.OpenSession before another client request registers an event handler to the BeforeInit event with a different closure - because, it's the last handler in the BeforeInit event's invocation list that "wins", potentially returning the wrong interceptor. The bug usually happens when two clients are making requests nearly simultaneously, but also when two clients are calling different web service methods with different execution times (one taking longer from authentication to OpenSession than another).

In addition to the data corruption, there's also a memory leak as the event handlers aren't de-registered? It might be the reason why our web service process is recycled at least once a day?

It really looks like the BeforeInit (and AfterInit) events need to go. I could alter the signature of the OpenSession method, and add an IInterceptor parameter. But this would break a lot of code, and I don't want to pass in an interceptor whenever a session is retrieved - I would like this to be transparent. Since the interceptor is a cross cutting concern in all applications using the DAL, would dependency injection be a viable solution? Unity is used in some other areas of our applications.

Any nudge in the right direction would be greatly appreciated :)

Answer 1

Dependency Injection example:

DependencyInjectionInterceptor.cs:

using NHibernate;
using System;
using Microsoft.Extensions.DependencyInjection;

namespace MyAmazingApplication
{
    public class DependencyInjectionInterceptor : EmptyInterceptor
    {
        private readonly IServiceProvider _serviceProvider;

        public DependencyInjectionInterceptor(IServiceProvider serviceProvider)
        {
            _serviceProvider = serviceProvider;
        }

        public T GetService<T>() => _serviceProvider.GetService<T>();

        public T GetRequiredService<T>() => _serviceProvider.GetRequiredService<T>();
    }
}

Startup.cs:

public void ConfigureServices(IServiceCollection services)
{
    ...
    var cfg = new Configuration();
    ... // your config setup
    cfg.SetListeners(NHibernate.Event.ListenerType.PreInsert, new[] { new AuditEventListener() });
    cfg.SetListeners(NHibernate.Event.ListenerType.PreUpdate, new[] { new AuditEventListener() });

    services.AddSingleton(cfg);
    services.AddSingleton(s => s.GetRequiredService<Configuration>().BuildSessionFactory());
    services.AddScoped(s => s.GetRequiredService<ISessionFactory>().WithOptions().Interceptor(new DependencyInjectionInterceptor(s)).OpenSession());

    ... // you other services setup
}

AuditEventListener.cs:

public class AuditEventListener : IPreUpdateEventListener, IPreInsertEventListener
{
    public bool OnPreUpdate(PreUpdateEvent e)
        {
            var user = ((DependencyInjectionInterceptor)e.Session.Interceptor).GetService<ICurrentUser>();

            if (e.Entity is IEntity)
                UpdateAuditTrail(user, e.State, e.Persister.PropertyNames, (IEntity)e.Entity, false);

            return false;
        }
}

So you use interceptor to get your scoped or any other service: var myService = ((DependencyInjectionInterceptor)e.Session.Interceptor).GetService<IService>();

ICurrentUser in particular is a scoped service which uses HttpContext to get the current user.

I hope it might be helpful for everyone.

Answer 2

Instead of supplying the interceptor at each ISessionFactory.OpenSession call, I would use a single interceptor instance globally configured (Configuration.SetInterceptor()).

This instance would retrieve the data to use from an adequate context allowing to isolate this data per request/user/whatever suits the application. (System.ServiceModel.OperationContext, System.Web.HttpContext, ..., depending on the application kind.)

The context data in your case would be set where LoggedInPersonIDInterceptorUtil.Setup is currently called.

If you need to use the same interceptor implementation for applications requiring different contextes, then you will need to choose the context to use according to some configuration parameter you would add (or inject it as a dependency in your interceptor).