static instance is not shared between threads

Question

I am using a singleton class that suppose to have a single static instance as follows:

    private static ISingletonClass _instance = null;

    public static ISingletonClass GetInstance(string id = null)
    {
        if (_instance == null)
        {
            if (id != null)
            {
                _instance = new SingletonClass(id);
            }
            else
            {
                throw new NullReferenceException("id is missing!");
            }
        }

        if (id != null && _instance.Id != id)
        {
            _instance = new SingletonClass(id); // changing instance
        }

        return _instance;
    }

All other code in the class is not static (including the Id property). Early in the run, when there is still a single, thread I initialize the singleton with some id, like so:

SingletonClass.GetInstance(<some_not_null_id>);

The _instance is set to not null (checked it). Later I create a few threads to do some tasks, that need, among else, to read information from the SingletonClass (no writing). According to any documentation I found, and answers in StackOverflow, the same instance should be available to all threads (I didn't use [ThreadStatic] or any other alike mechanism).

However, When trying to GetInstance() with no parameters from inside the threads I get NullException (the _instance member is Null).

I'm using .NET version 4.5, and working with VS2012.

Any ideas?

Side note: NullReferenceException is a reserved exception and thus you shouldn't throw it. — Sriram Sakthivel
– Sriram Sakthivel, Commented Jan 8, 2015 at 9:49
You get your nullexception, the one with "id is missing!" message? — weston
– weston, Commented Jan 8, 2015 at 9:49
Another side note: the way you implemented this it isn't a singleton anymore. — Thomas Lielacher
– Thomas Lielacher, Commented Jan 8, 2015 at 9:54
Your code has serious problem to consider it Singleton. When different Id get passed then it will create different object. It over write previously written instance reference. — dotnetstep
– dotnetstep, Commented Jan 8, 2015 at 9:55

Peter Duniho · Accepted Answer · 2015-01-08 22:14:15Z

First, your assumption about the exception thrown is incorrect:

NullException (the _instance member is Null).

The only NullReferenceException that would be thrown from your GetInstance() method is the one you throw yourself. Assuming you have no code anywhere else that resets the _instance value to null, the only place that method dereferences the _instance value is at a statement that cannot be reached without ensuring that _instance is initialized to some non-null value.

As for the broader problem, IMHO the biggest issue is that you have a poorly-defined problem, and a broken implementation.

Even ignoring the question of whether the "singleton" (it's not really a singleton, but for the sake of argument let's call it that for now) is ever changed, the initialization is not thread safe. You have the following potential race (assuming a single CPU core to make the illustration simple):

Thread 1                   Thread 2
--------                   --------
call GetInstance()
if (_instance == null)
--> preempted <--
                           call GetInstance()
                           if (_instance == null)
                           ...
                           _instance = new SingletonClass(id);
                           ...
                           return _instance;
                           --> preempted <--
if (_instance == null)
...
_instance = new SingletonClass(id);
...
return _instance;

As you can see in the above example, the way the code is written now, each thread could independently try to retrieve the instance, would see the current value as null, and would create a new instance object to be returned.

For a true singleton, the best way to implement this is with the Lazy<T> class:

private static readonly Lazy<SingletonClass> _instance =
   new Lazy<SingletonClass>(() => new SingletonClass());

public static SingletonClass Instance { get { return _instance.Value; } }

In this case, the Lazy<T> class handles all of the initialization work, including ensuring it's done in a thread-safe way.

In your case, where you do not have a true singleton, the above won't work. The Lazy<T> pattern only works when something is to be initialized just once, but you want to be able to change it on the fly. Given that, you need something more like this:

private static ISingletonClass _instance = null;
private static readonly object _lock = new object();

public static ISingletonClass GetInstance(string id = null)
{
    lock (_object)
    {
        if (_instance == null || (id != null && _instance.Id != id))
        {
            if (id == null)
            {
                throw new ArgumentNullException("id");
            }

            _instance = new SingletonClass(id);
        }

        return _instance;
    }
}

The above will ensure against threads initializing the field concurrently. They can race only to the lock, and then one thread is guaranteed to be the only one to initialize the object, assuming each thread passes the same value for id.

That said, that only fixes the basic thread-safety issue in your code. There are some bigger problems.

First, what do you expect the code to do if and when one thread retrieves the current instance, then some other thread changes the current instance before the first thread is done using the instance it retrieved? I'm not saying this is inherently broken, but it is at the very least very fragile, and you absolutely need to think about this scenario and decide for yourself what the right behavior should be in that case.

Second, this is a really fragile mutation of the singleton pattern. A true singleton will have a single object, allocated only once during the lifetime of the process. This ensures simplicity of design and predictability of behavior.

The implementation you have, will necessarily make it a lot harder to understand what the code is doing at any given point. It is practically guaranteed to add large amounts of time to some developer's day, either yours or someone else's, when some bug comes up and they are trying to track down the actual cause of the bug.

The fact that the instances of this class are tied to a string ID suggests that a better approach might be to maintain a Dictionary<string, SingletonClass> objects, requiring all callers to always specify the ID, and using that ID to retrieve (possibly initializing lazily of course) the object that thread needs at that moment.

I strongly recommend a different design. But at a minimum, if you decide you must go this direction, make sure that you have considered all of the various combinations of thread events, and not only decided what the correct behavior is in each given scenario, but added code to ensure any assumed constraints.

Thank you for comprehensive response. There are some clarifications i need to do: the _instance member IS null. That's a fact. I debugged it. The scenario you described is not possible in this case because, like I wrote originally, the instance is set successfully (debugged it) before threads are created. The threads are ONLY reading from the instance, not writing, or changing it. I do need to add some thread safty mechanism to the code, that art is true.
The fact that the _instance member is null doesn't mean that the NullReferenceException occurs while using the _instance value (i.e. dereferencing it). If you simply meant that the value being null led to your own code throwing the NullReferenceException explicitly, then I misunderstood...my point was that the it's your own throw new NullReferenceException... that's causing the exception, not the actual use of the `_instance_ value.

weston · Accepted Answer · 2015-01-08 10:15:12Z

0

I believe you want a true singleton with a value you can update. This updating needs to be threadsafe. The creation should be a separate method from the getting.

private static readonly MyContainerClass _instance = new MyContainerClass(); //true singleton

private sealed class MyContainerClass //this is the singleton
{
   private ISingletonClass _value = null;

   public ISingletonClass Value //threadsafe access to your object
   {
      get { lock(this){ return _value; } }
   }

   public ISingletonClass CreateOrUpdateValue(string id) //threadsafe updating of your object
   {
      if (id==null) throw new ArgumentNullException("id is missing!");
      lock(this)
      {
        var instance = _instance.Value;

        if (instance == null || instance.Id != id)
          _instance.Value = new SingletonClass(id);

        return _instance.Value;
       }
    }
}

public static void CreateOrUpdateInstance(string id)
{
    _instance.CreateOrUpdateValue(id);
}

public static ISingletonClass GetInstance()
{
    var instance = _instance.Value;

    if (instance == null)
       throw new Exception("Instance has not been created");

    return _instance;
}

// this is like your original method if you really want it
public static ISingletonClass GetInstance(string id)
{
    return _instance.CreateOrUpdateValue(id);
}

edited Jan 8, 2015 at 10:15

answered Jan 8, 2015 at 10:09

weston

55k23 gold badges151 silver badges209 bronze badges

2 Comments

Ziv Over a year ago

Thanks for the input, In general - you're correct. In this case, like I wrote, the instance is created BEFORE (serially) more threads are created. And the threads are NOT trying to replace the instance, only to get it (supplying no id). Still - it doesn't explain the problem - Why is the Static member not shared between threads?

weston Over a year ago

Maybe because your code is not threadsafe. You have no locks.

Collectives™ on Stack Overflow

static instance is not shared between threads

2 Answers 2

2 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related