views:

523

answers:

4

I have an extension method on the HttpApplicationState object for getting my IoC container out of the application. This same code will also create the container if it doesn't exist.

I have 2 questions:

  1. Is my code actually thread safe as I intend it to be
  2. Is this considered the best practice for dealing with the application state

Code as follows:

private const string GlobalContainerKey = "UnityContainerKey";

public static IUnityContainer GetContainer(this HttpApplicationState application)
{
    var container = application[GlobalContainerKey] as IUnityContainer;

    if (container == null)
    {
        try
        {
            application.Lock();
            container = application[GlobalContainerKey] as IUnityContainer;

            if (container == null)
            {
                container = new UnityContainer();
                application[GlobalContainerKey] = container;
            }
        }
        finally
        {
            application.UnLock();
        }
    }

    return container;
}
+1  A: 

Technically, that won't work given the EMCA specification. Jon Skeet goes into this in his C# FAQ:

http://www.yoda.arachsys.com/csharp/singleton.html

Specifically, see the section with the "Third version"

I would read further down and use his suggestion for how to implement the singleton to see how to implement what you are trying to do.

casperOne
The difference is I'm already working with a singleton not trying to create a new singleton. Reading point 2 in his rationale against DCL is the only legitimate point however there's no elaboration why it won't work or what "explicit memory barrier calls" would be to ensure it.
Chris Marisic
+1  A: 

Why dou you check for "container == null" the first time? I think you should lock first and then check for container being null. All sort of dodgy stuff may happen between the first if and the return in other threads.

DrJokepu
I don't want to lock the application every single time after the container exists because this gets called on every single page request. It only needs to be locked for the initial insert. I did however mean to have a second "container = application[GlobalContainerKey] as IUnityContainer;" after Lock
Chris Marisic
+2  A: 

You need to put

var container = application[GlobalContainerKey] as IUnityContainer;

in the lock as well, otherwise many threads may create a new container in sequence.

private const string GlobalContainerKey = "UnityContainerKey";
private const object lockObject = new object();

public static IUnityContainer GetContainer(this HttpApplicationState application)
{
    var IUnityContainer container = null;

    lock (lockObject)
    {
        container = application[GlobalContainerKey] as IUnityContainer;
        if (container == null)
        {
            container = new UnityContainer();
            application[GlobalContainerKey] = container;
        }
    }

    return container;
}
Sunny
You're right I meant for that to be inside there before the second if statement.
Chris Marisic
There's no need of the second if statement if the first one is in the lock. Also, if using lock(), you will not need the try/finally (lock() is a syntax sugar for try/finally.
Sunny
Right but I don't want it to lock the application on every single page request which is why I have the outer if. Can you elaborate on the usage of .Lock without a try/finally and why it's safe? Do I even need to call .Unlock?
Chris Marisic
if you do not want to lock the application to lock on each call, then go with a static get property for the container (and make the underlying filed readonly). Then set it up in a static ctor for the application class.
Sunny
The static ctor is guaranteed to run before any instance or static method or field is accessed. You are loosing the "lazy" initialization though.
Sunny
I suppose I could move my container to be a standalone singleton class.
Chris Marisic
A: 

Double check with lock is used inside the .NET Framework's code, for singletons (see System.Web.Profile.ProfileManager for example).

So I think your implementation is OK.

Nicolas Dorier