Keep static volatile ISessionFactory

Question

I have an implemetation of session factory to be singleton like this:

public sealed class MySessionFactory
{
    private static volatile MySessionFactory _instance;
    private ISessionFactory _sessionFactory;
    private static volatile object _locker = new object();

    private MySessionFactory()
    {

    }

    public MySessionFactory Intance
    {
        get
        {
            if (_instance != null)
                return _instance;

            lock (_locker)
            {
                if (_sessionFactory == null)
                {
                    _instance = new MySessionFactory();
                }
            }

            return _instance;
        }
    }

    public ISession OpenSession()
    {
        if (_sessionFactory != null)
            return _sessionFactory.OpenSession();

        lock (_locker)
        {
            if (_sessionFactory == null)
            {
                var cfg = FluentNHibernate.Cfg.Fluently.Configure()
                    .Database(FluentNHibernate.Cfg.Db.PostgreSQLConfiguration.Standard.ConnectionString("connectionString").UseReflectionOptimizer())
                    .Mappings(m => m.FluentMappings.AddFromAssemblyOf<MappingsAssembly>());
                _sessionFactory = cfg.BuildSessionFactory();

            }
        }

        return _sessionFactory.OpenSession();
    }
}

If i remove the volatile of static variable _instance, I will get some benefits with this change? Or this is a good practice pattern?

Answer 1

If you just remove volatile from your _instance field your code will not be thread safe anymore.

If you really want to keep the "famous" double-check locking technique you can remove volatile from your _instance field but then you need to modify the assignment to look like this:

var tmp = new MySessionFactory();
Volatile.Write(ref _instance, tmp);

This will give you some benefit because _instance field is no longer volatile so all the reads are not volatile (some performance gain). However we have volatile assignment that will guarantee that code is still thread safe.

My personal opinion - don't use double-check locking technique. If you really need lazy initialization use Lazy class. If you don't need 100% lazy initialization just write it like this:

private static readonly MySessionFactory _instance = new MySessionFactory();

This initialization will be called by static class constructor which is called automatically the first time code attempts to access a member of the class. In CLR constructors are thread safe by design so you don't need to worry about concurrency in this case. And since in your case you don't have any members of that class that are not related to MySessionFactory _instance this solution will behave as lazy as the double-check lock.

If you want to read more about this there is a whole chapter in Jeffrey Richters book CLR via C# called "The Famous Double-Check Locking Technique". Good reading;)