SessionFactoryObjectFactory Early Cache Population Causes Threading Issue

[mjkent]

In #2190 the time that the SessionFactoryObjectFactory cache for an instance was moved up before the SessionFactoryImpl is fully constructed. This can mean that during the ISessionFactory constructor, SessionFactoryObjectFactory.GetNamedInstance can return a partially constructed ISessionFactory

In the case where application start wasn't the right time to create the ISessionFactory and double checked locking was recommended. An example:

private static ISessionFactory GetSessionFactory(sessionFactoryName)
{
    var sessionFactory = SessionFactoryObjectFactory.GetNamedInstance(sessionFactoryName);
    if (sessionFactory == null)
    {
        lock (_sessionFactoryApplicationLock)
        {
            sessionFactory = SessionFactoryObjectFactory.GetNamedInstance(sessionFactoryName);
            if (sessionFactory == null)
            {
                sessionFactory = GetConfig(sessionFactoryName).BuildSessionFactory();
            }
        }
    }
    return sessionFactory;
}

When multiple threads hit this at the same time, please imagine:
Thread 1 comes in, SessionFactoryObjectFactory.GetNamedInstance returns null.
Thread 1 acquires the lock the second SessionFactoryObjectFactory.GetNamedInstance returns null.
Thread 1 starts building the session factory. Early in the ISessionFactory the cache is populated with a not yet built SessionFactory
Thread 2 calls SessionFactoryObjectFactory.GetNamedInstance and gets the partially built ISessionFactory
Bad behavior: Thread 1 is writing to the various dictionaries while thread 2 is reading them. We've seen entityPersisters not get fully populated(which populates one at a time in a loop on a normal dictionary) while classMetadata is populated(which populates a temporary dictionary and then uses a readonly dictionary). Dictionaries are thread safe if multiple threads are reading from them at once, but if any writing is going on at that time, the behavior is undefined and will break

Suggested improvement
The ISessionFactory is meant to be an unchanging, thread safe(in use, but not creation) object. The various non-readonly dictionaries could be changed to readonly dictionaries to have better safety(although this doesn't fully fix the issue - it would make the behavior more deterministic, the dictionary either exists with all the values or is null). This would help people who have older systems that followed best practice recommendations of the time not get session factories that are broken for their lifetime, but only for the critical point before the readonly dictionaries are populated.

It also probably makes sense to make SessionFactoryObjectFactory's dictionaries be thread safe. Pretty low cost to the change here and it would help an issue in the same vein with readers and writers both using the dictionaries at the same time.

On the side of the code using SessionFactoryObjectFactory.GetNamedInstance, moving the lock to cover the reader will(I believe) fix this as well. But the odd partial build behavior can be fixed here, at least improving the situation. Removing the use of SessionFactoryObjectFactory and handling caching outside NH for the ISessionFactory also is an option.

While it is important that one(and only one) ISessionFactory be built at a time, this behavior causes a very hard to debug and understand issue, where the object gets partially built successfully and partially not built, causing very odd errors that would confuse people who have historically used the current SessionFactoryObjectFactory implementation based on historical best practices.

Some historical context:
NH requires that the SessionFactory is only built once per NamedInstance. In some cases, application start is a tough time to do this(for example in a multi-tenant system with multiple databases sharing the same schema).
Some context here and in the links contained(including the book "Hibernate in Action (In Action series) [ILLUSTRATED] (Paperback)")
https://web.archive.org/web/20071011005024/http://www.codeproject.com/aspnet/NHibernateBestPractices.asp

[fredericDelaporte]

In the case where application start wasn't the right time to create the ISessionFactory and double checked locking was recommended.

I would like to know where is it recommended. I do not find any mention of it in your webarchive link. The only documentation I can find about SessionFactoryObjectFactory is its xml doc, stating:

Resolves <see cref="ISessionFactory"/> lookups and deserialization.

With additional comments further stating this is meant to handle deserialization.

This is a class in the Nhibernate.Impl namespace. Classes there are usually not meant for direct use by application code, although many are still public. There a no mention of it in the reference documentation.

So, well, unless I am proven wrong, using this class for resolving factories directly from it inside application code is not documented, and it being thread safe about such an usage even less.

The troubles you can have by reading it concurrently to creating session factories due to its non-threadsafe internal dictionaries, as you note in #3658, further enforce my assumption this class was never meant to support such usage. These troubles were already there prior to #2190 and its fix #2196. It has never used thread safe collections supporting reads concurrently to writes. (In 2004, it was using System.Collections.Hashtable, which were replaced by Dictionary<string, ISessionFactory> in 2008.)

Actually, the static type SessionFactoryObjectFactory has all its methods synchronized. So, no concurrency trouble can occurs with its non threadsafe dictionaries. They will not be read while being modified by another thread, because only a single thread can execute any method of the type at the same time. All other threads will wait, whatever the method they attempt to call on the type.

So, it looks to me you should either, as you note:

• Remove the double check locking: first lock, then only check, ensuring no concurrent read will happen during writing.
• Or use your own implementation of a session factories dictionary. That is what most applications would do, as seems to imply this old blog I can find about it.

The second option would mean threading bugs inside the SessionFactoryObjectFactory can still happen if two session factories are built concurrently, since it would still mean the non threadsafe dictionaries could be concurrently written. (Not possible, the type is fully synchronized.)

So, we should do something about theses dictionaries.

About SessionFactoryObjectFactory being capable of yielding session factories still under construction, I am not convinced we should change it back, because this class appears to me meant for deserialization purpose, a case in which session factories are already built.

[fredericDelaporte]

Looking again at this, prior to #2190, the trouble was already there actually. The factory was registered a bit later in its construction, but it was not yet fully built either, and getting it through the session factory object factory and a concurrent thread could already lead to failures.

After registration prior to #2190, the session factory build process had still still these operations to do:

• Schema management, meaning the database may not have the expected schema for concurrent threads in applications using this feature while resolving session factories like yours.
• Creating session context, causing failure for concurrent threads for application using this feature while resolving session factories like yours.
• Creating query cache, causing failure for concurrent threads for application using this feature while resolving session factories like yours.
• Setting up the entity not found callback, causing a null reference exception in case a concurrent thread attempts to load an non-existent entity from a session built from the still building session factory while resolving session factories like yours.

[fredericDelaporte]

I propose to add a GetOrBuild to handle the need of applications like yours, see #3668.