Thread safety of static

Question

I came across some code like this.

public class ConnectionUtility
{
    private static SqlConnection con;

    public static SqlConnection GimmeConnection()
    {
        if(con==null)
            con = new SqlConnection();
        return con;
    }
}

This is in an ASP.NET web application. Can one expect there to be race conditions where one request/page tries to open/close execute things on the connection and other request tries to do those things as well?

Answer 1

Yes, a race condition is definitely possible there. Multiple threads might concurrently call GimmeConnection() and all create a new connection, so there is a race condition on the initialization of con.

The correct way to do this in .NET is actually fairly simple:

public class ConnectionUtility
{
    private static SqlConnection con = new SqlConnection();

    public static SqlConnection GimmeConnection()
    {
        return con;
    }
}

This is guaranteed to work, without any race conditions.

Of course, there is also another possible race condition, which is not fixed by this:

Because you create one single globally accessible connection, it is easy for multiple threads to use it concurrently, which is not safe.

If the connection is used by multiple threads, these accesses must be explicitly serialized, for example through locking.

Of course, the best course of action is still to just avoid singletons in the first place...

Answer 2

Yes... You shouldn't share a single static connection along multiple threads. This is not only a race condition issue - you will have many web pages trying to use the same connection at the same time which won't work.

You can use the [ThreadStatic] attribute on your "con" which will make it global on thread scope.

Answer 3

Yes, you can expect such race conditions. Good spotting!

Answer 4

Two race conditions, one potentially harmless, one dreadful. There's also a logical error.

The potentially harmless one is in the constructor logic of:

if(con==null)
  con = new Thing();

This can be harmless if you want to use a single object as an optimisation, but having a period where more than one is used is merely sub-optimal rather than wrong (not every race is the end of the world). It depends on just what Thing is and how it's used.

The disastrous race is:

return con;

Because in this case the type is SqlConnection, which is not thread-safe, so every single use of the property is a race that is courting disaster.

The logical error is in having a singleton cache an object which is light to produce and which handles its own thread-safe pooling of the heavy part. Because of this pooling, you shouldn't hold on to an SqlConnection any longer than absolutely necessary. Indeed, if you've a gap between one use and another (a bad smell in itself though), you should close it and re-open it again. This makes the thread-safe pooling that SqlConnection provides for you, work at it most optimal between uses.

Answer 5

Whenever writing singletons, you should make make them threadsafe:

  class Singleton 
  {
    private Singleton() { }
    private static volatile Singleton instance;

    public static Singleton GetInstance() 
    {
       // DoubleLock
       if (instance == null) 
       {
          lock(m_lock) {  
             if (instance == null) 
             { 
                instance = new Singleton();
             }   
          }
       }
       return instance;
    }

    // helper
    private static object m_lock = new object();
  }