C# Singleton thread safe

Question

I have a singleton class similar to this

public class Singleton
{
    private static Singleton m_instance;
    private Timer m_timer;
    private static List<CustomObject> m_cacheObjects;

    private Singleton()
    {    
        m_cacheObjects = new List<CustomObject>();
        m_timer= new Timer(MyTimerCallBack, 
                           null, 
                           TimeSpan.FromSeconds(60), 
                           TimeSpan.FromSeconds(60));           
    }

    public static Singleton Instance
    {
        get
        {
            if (m_instance == null)
            {
                m_instance = new Singleton();
            }
            return m_instance;
        }
    }

    private void MyTimerCallBack(object state)
    {
        //******** Update the list by interval here ******************

        m_cacheObjects = UpdateTheList();
    }

    public void CallMe()
    {
        foreach (CustomObject obj in m_cacheObjects)
        {
            // do something here based on obj

            // The question is, does the m_cacheObjects is thread safe??
            // what happen if the m_cacheObjects is changed
            // during the loop interation?
        }
    }
}

The CallMe method will be called by web service:

  [WebMethod]
    public void CallMeWebService()
    {
        Singleton.Instance.CallMe();
    }

The questions: 1) Is the m_cacheObjects is thread safe? what happen if the m_cacheObjects is changed(because of the timer) during the loop interation (in the CallMe() )?

2) Is a new thread will be created when the Webservice CallMeWebService() is being called?

Solution 1

1: No, a static list is not automatically thread-safe; you must protect m_cacheObjects manually

2: That is an implementation detail; at first glance it looks like it exposes itself as a sync method, but how it does that is entirely up to it

Actually, your static initialization isn't thread-safe either; I could brute-force a scenario where two different Singleton instances were used. It would take repetition to produce it, but it would happen.

Frankly, unless you have a really good reason not to, the simplest but safest singleton pattern is simply:

private static readonly Singleton m_instance = new Singleton();

Solution 2

//using System.Runtime.CompilerServices;

private static volatile Singelton _instance;

public static Singelton Instance
{
    [MethodImpl(MethodImplOptions.Synchronized)]
    get
    {
        if (_instance == null)
        {
            _instance = new Singelton();
        }
        return _instance;
    }
}

Explain:

[MethodImpl(MethodImplOptions.Synchronized)] This will tell the compiler that the access to "Instance" is "Synchronized" so the system take care about the calls to this Parameter.

This is Thread-Safe.

EDIT: (Also, the "Lock()" examples are not safe! Coz, u can disable the thread Safety with "Monitor.Exit(Singleton);")

Solution 3

I'd suggest you have aread on Jon Skeets article on how to create thread safe singletons over at http://csharpindepth.com/Articles/General/Singleton.aspx

Solution 4

This is a pretty good resource on how to implement the singleton pattern in a thread safe way: http://msdn.microsoft.com/en-us/library/ff650316.aspx

public sealed class Singleton
{
   private static volatile Singleton instance;
   private static object syncRoot = new Object();

   private Singleton() {}

   public static Singleton Instance
   {
      get 
      {
     if (instance == null) 
     {
        lock (syncRoot) 
        {
           if (instance == null) 
          instance = new Singleton();
        }
     }

     return instance;
      }
   }
}

This simply makes sure that there will never be more than one instance. You also need to apply locking for your custom method.

public void CallMe()
{
    lock (syncRoot) 
    {
        foreach (CustomObject obj in m_cacheObjects)
        {
            // do something here based on obj
        }
    }
}

View more solutions

Comments

[Marc Gravell]

@L.B. note I mentioned "simplest". To be frank, the situations that need a lazily initialized singleton are very rare; a: singletons should be rare, b: you'd need to know that Singleton was expensive to create, and c: you'd need to know that there are useful things that can be done without needing it. In this case, I think it actually fails all 3 criteria.

[Luis Filipe]

By manually, do you mean using the lock keyword or something else?

[Marc Gravell]

@Luis yes I do indeed mean "the lock keyword or something else" ;p obviously lock would be the simplest approach (and simplicity counts for a lot), but there are plenty of ways it could be done.

[Gaute Løken]

@Downvoter I don't mind a downvote if I deserve it, but would you please put a comment in along with it? As is it simply seems like one of the other answerers wanted to push me down the list of answers regardless of what I said.

[user1553857]

i understand the new CallMe method. but not sure about the MyTimerCallBack , if im sure that the m_cacheObjects will be updated before the next timer tick, do i still need to lock it?

[Alexander]

Single lock statement doesn't make sense. By using lock in MyTimerCallback you can be sure that CallMe will wait until MyTimerCallback completes. And vise versa - lock in CallMe makes sure that m_cacheObjects won't be updated until it'll be copied to a localCopy.