Singleton Class in a multi-threaded application, blocking clause

Question

I have a singleton class that is used in multiple threads. To prevent multiple access problems, I use the Lock method when accessing a particular property of the class. The question is, is it possible to improve the code and put the Lock method inside the singleton class, rather than putting it every time the code accesses the access class?

/* Class code*/ public class ServerStatus { private static ServerStatus _instance; public static ServerStatus Instance { get { return _instance ?? (_instance = new ServerStatus()); } set { _instance = value; } } ServerStatus() { PistonCount = 0; PistonQueue = new List<string>(); ErrorList = new List<string>(); } public int PistonCount { get; set; } public List<string> PistonQueue { get; set; } public List<string> ErrorList { get; set; } } /*Code for accessing class properties*/ private static readonly object Locker = new object(); /*Skip*/ lock (Locker) { ServerStatus.Instance.PistonQueue.Add(e.FullPath); } /*Skip*/ lock (Locker) { ServerStatus.Instance.PistonCount++; } 

Answer 1

ServerStatus should support native synchronization, not external clients of this class. In this case, you will need to reorganize ServerStatus and create several thread-safe (with blocking) methods:

Remove these properties: public List<string> PistonQueue { get; set; } public List<string> PistonQueue { get; set; } public List<string> PistonQueue { get; set; } , because even if you can lock inside these properties, you cannot control what clients do when they PistonQueue actual PistonQueue .

... and replace with methods like (sorry pseudocode, I can't think today):

 public PistonQueueAdd(string fullPath) { lock(_serverStatusSyncRoot) { // ... } } 

Answer 2

This is a single-user thread-safe template that I use if you are interested:

  public class DataAccess { #region Singleton private static readonly object m_SyncRoot = new Object(); private static volatile DataAccess m_SingleInstance; public static DataAccess Instance { get { if (m_SingleInstance == null) { lock (m_SyncRoot) { if (m_SingleInstance == null) m_SingleInstance = new DataAccess(); } } return m_SingleInstance; } } private DataAccess() { } #endregion } 

Answer 3

IMHO, this is the ultimate solution for thread safe locking in singleton mode. From it (fifth on the list):

 public sealed class Singleton { private Singleton() { } public static Singleton Instance { get { return Nested.instance; } } private class Nested { // Explicit static constructor to tell C# compiler // not to mark type as beforefieldinit static Nested() { } internal static readonly Singleton instance = new Singleton(); } } 

Answer 4

This is pretty common. Locking / unlocking in getters / setters is much safer (you cannot forget to do this) and more convenient (locking should not be available directly in all places where you use a property) than external locking for each access to properties.

Rgds, Martin