Make Most of the Dictionaries in SessionFactoryImpl ReadOnly Improves #3657

[mjkent]

Requesting comments on this PR that improves a regression documented in #3657

The core of this is to make SessionFactoryImpl dictionary's read only - this makes the behavior easier to understand(and safer) for those who accidentally do it(in the issue I provided a case that a change to the NH source caused this to be a new behavior on the existing use of the API in a recommended/best practice way). It doesn't fully fix the "issue" - because SessionFactoryImpl is meant to not be created multiple at once, so it's not an issue in that way, but giving more deterministic and easier to debug behavior on a regression is an improvement here. It also means that instead of having a half constructed object(that has some Dictionary's populated and others half populated, it will either be null or set correctly).

There is also an unlike case of writing and reading from [SessionFactoryObjectFactory.cs's dictionary's at the same time, which is documented as an undefined behavior - in practice it can cause very odd issues like dictionaries losing values.

I was sure what the NH team has set for rules on creating tests around these. Somewhat tough to create a failing test because of the non-deterministic behavior.

It would be appreciated if this could be in v5.5.y? I paid attention to keep the public API's the same and keep all existing behavior through the public API's.

Note: I did read through contributing and ran all the SqlServer tests. I have some failing local on both master and this branch that are the same(GH3516) - I assume it's an environment issue.

[fredericDelaporte]

All theses changes look superfluous to me, because this will never be enough to get the build of a session factory threadsafe with concurrent uses of the factory being built. The only reasonable fix is to not end up using a factory that is concurrently being built.

[fredericDelaporte]

These changes do not actually fix the reported issue and do not improve the robustness of the code:

• A session factory is no meant to be used concurrently to its own build. No change should be made attempting to support this.
• Changing a dictionary for a concurrent one while using it through a non concurrent interface does not improve concurrency support.
• Using a concurrent dictionary in an already fully synchronized class (all methods being synchronized) is redundant and unneeded.

[mjkent]

@fredericDelaporte I'm hoping you'll reconsider.

For the changes to src/NHibernate/Impl/SessionFactoryImpl.cs - I think there is some benefit and low risk.
One major benefit is that future developers don't accidentally do something to change the values in the List's that the changes protect. Since use(and not construction) of the ISessionFactory is often multi-threaded, explicitly using data structures that protect this capability has value. Once built, these Dictionary's should not be added to/changed.

It's a pretty low cost change for this. I'd be happy to provide a PR with just those changes.

On the changes to SessionFactoryObjectFactory - I see your meaning. I missed the attribute on the methods contained.