Is Structuremap singleton thread safe?

Question

Currently I have the following class:

    public class PluginManager
{
    private static bool s_initialized;
    private static object s_lock = new object();

    public static void Initialize() {
        if (!s_initialized) {
            lock (s_lock) {
                if (!s_initialized) {
                    // initialize

                    s_initialized = true;
                }
            }
        }
    }
}

The important thing here is that Initialize() should only be executed once whilst the application is running. I thought that I would refactor this into a singleton class since this would be more thread safe?:

    public sealed class PluginService
{
    static PluginService() { }
    private static PluginService _instance = new PluginService();
    public static PluginService Instance { get { return _instance; } }

    private bool s_initialized;

    public void Initialize() {
        if (!s_initialized)
        {
            // initialize

            s_initialized = true;
        }
    }
}

Question one, is it still necessary to have the lock here (I have removed it) since we will only ever be working on the same instance?

Finally, I want to use DI and structure map to initialize my servcices so I have refactored as below:

    public interface IPluginService {
    void Initialize();
}

public class NewPluginService : IPluginService
{
    private bool s_initialized;
    public void Initialize() {
        if (!s_initialized) {
            // initialize

            s_initialized = true;
        }
    }
}

And in my registry:

            ForRequestedType<IPluginService>()
            .TheDefaultIsConcreteType<NewPluginService>().AsSingletons();

This works as expected (singleton returning true in the following code):

            var instance1 = ObjectFactory.GetInstance<IPluginService>();           
        var instance2 = ObjectFactory.GetInstance<IPluginService>();

        bool singleton = (instance1 == instance2);

So my next question, is the structure map solution as thread safe as the singleton class (second example). The only downside is that this would still allow NewPluginService to be instantiated directly (if not using structure map).

Many thanks, Ben

Answer 1

I would make several recommendations:

• the boolean flag should be volatile
• make your singleton instance readonly

• the initialization is not thread safe, regardless of the fact that you have only one instance... so it should be synchronized

  public sealded class PluginService
  {
  
  static PluginService() { }
  
  //make the instance readonly
  private static readonly PluginService _instance = new PluginService();
  public static PluginService Instance { get { return _instance; } }
  
  // make the flag volatile
  private static volatile bool s_initialized = false;
  private static object s_lock = new object();
  
  
  // you still need to synchronize when you're initializing
  public void Initialize() {
      lock(s_lock)
      {
          if (!s_initialized)
          {
              // initialize
  
                  s_initialized = true;
              }
          }
      }
  }
  

There is no contention on the structured map, so its thread safety doesn't seem compromised...

The singleton class you had was not thread safe. The main thing to remember is that a single instance does not ensure a single thread can only access it. If there are multiple threads that have a reference to the instance, then there is contention on the instance and the data it's holding. If there is contention then you should ensure thread safety (synchronize at the very minimum).