CODEWITHSUNDEEP
c#.netdesign-patternssingleton
exHash
exHash

Reputation: 1265

Deadlock error with a singleton

I'm trying to get a bit more in to design patterns and I was building a quick test to help me further understand singleton pattern. However I came into an error in .net that baffled me, what made it weirder I was not able to replicate the error this just added to my confusion. The code below is not the best code in the world but I was randomly testing things to help me get a deeper understanding. 

class Program
{

    static void Main(string[] args)
    {
        Thread t1 = new Thread(new ThreadStart(run1));
        Thread t2 = new Thread(new ThreadStart(run2));

        t1.Priority = ThreadPriority.Lowest;
        t1.Start();
        t2.Priority = ThreadPriority.Lowest;
        t2.Start();

        Console.ReadLine();
    }

    public static void run1()
    {
        Console.WriteLine("im in run1 \n");
        TestSingleton._instance.PopulateCrudItemsProcess();
        Thread.Sleep(1000);
        Console.WriteLine(TestSingleton._instance.GetStrValue("first", 1000));
    }
    public static void run2()
    {
        Console.WriteLine("im in run2 \n");
        TestSingleton._instance.PopulateCrudItemsProcess();
        Console.WriteLine(TestSingleton._instance.GetStrValue("second", 500));

    }

}

sealed class TestSingleton
{
    private TestSingleton() { }

    public static readonly TestSingleton _instance = new TestSingleton();

    public string GetStrValue(string str, int time)
    {
        return str;
    }

    public void PopulateCrudItemsProcess()
    {
        const string proc = "[testdb].[dbo].[tstsproc]";
        string _reportingConnStr = ConfigurationManager.ConnectionStrings["reporting"].ConnectionString;

        using (SqlConnection conn = new SqlConnection(_reportingConnStr))
        {
            using (SqlCommand cmd = new SqlCommand(proc, conn))
            {
                cmd.CommandType = CommandType.StoredProcedure;
                cmd.CommandTimeout = 7200;
                conn.Open();
                cmd.ExecuteNonQuery();
            }
        }
    }
}

Now what happened was the program crashed on cmd.ExecuteNonQuery(); and stated that there was a deadlock on a resource. I wish i could have captured the actual error but like i said I was only able to get the error once. According to MS this code is a thread safe way to create a singleton. So any thoughts on what may have caused the error? any info is greatly appreciated.

Upvotes: 0

Views: 2332

Answers (3)

ntziolis
ntziolis

Reputation: 10221

Regarding thread safety in your situation you have to devide between:

  • Creating the Singleton
  • Executing methods on the singleton instance
  • Executing the Stored Procedures

The first part you got covert, a static constructor is only called once across multiple threads and therefore all the initialization code for static variables is only executed once as well. This is implicit though and in the spirit of making things easy to understand you should use a more explicit approach also doing the initialization within the get instance method which is more common, see below. However is making your code easier to read for others, it already works as is in this regard.

The second part in itself is thread safe as well, since you not sharing any resources as JonSkeet pointed out already.

The third part however seems to be the issue as others have pointed out already. Executing StoredProcedures at the same time or any SQL for that matter CAN cause deadlocks on the DB level. In your case it is very likely that it does, which is the error you are seeing.

Stealing the stub from KarlLynch and making the instance creation explicitly threadsafe:

public class MySingleton
{
  private static MySingleton Instance{ get; set; }
  private static readonly object initLock;

  // Private constructor
  private MySingleton()
  {
    initLock = new object();
  }

  public static MySingleton GetInstance()
  {    
    if (Instance == null)
    {
      lock(initLock)
      {
        if (Instance == null)
        {
          Instance = new MySingleton();
        }
      }
    }    
    return Instance;
  }
}

Upvotes: 1

Steve
Steve

Reputation: 216291

You could try using lock keyword.
It allows developers to make scope of codes that must be synch between application threading. This is to insure that incoming threads will not interrupt the current until it is finished.

So let's an example:

declare inside your singleton a global 

private object _oLock;

rewrite PopulateCrudItemsProcess in this way

    lock(_oLock)
    {
        using (SqlConnection conn = new SqlConnection(_reportingConnStr)) 
        { 
            using (SqlCommand cmd = new SqlCommand(proc, conn)) 
            { 
                cmd.CommandType = CommandType.StoredProcedure; 
                cmd.CommandTimeout = 7200; 
                conn.Open(); 
                cmd.ExecuteNonQuery(); 
            } 
        } 
   }

Upvotes: 2

RvdK
RvdK

Reputation: 19790

Sorry, but Singleton is not about being threadsafe. Its about having only 1 instance of an object.

Therefore PopulateCrudItemsProcess and GetStrValue are not threadsafe...

Upvotes: 5

Related Questions