CODEWITHSUNDEEP
c#.netasp.net
Ryan S
Ryan S

Reputation: 3268

Are static variables thread-safe? C#

I want to create a class which stores DataTables, this will prevent my application to import a list of details each time I want to retrieve it. Therefore this should be done once, I believe that the following code does so, but I am not sure if it is thread-safe.

The below code is in the Business Layer Section of my three tier application, it is returning a DataTable to the Presentation Layer.

public class BusinessLayerHandler
{
    public static DataTable unitTable;
    public static DataTable currencyTable;

    public static DataTable GetUnitList()
    {
        //import lists each time the application is run
        unitTable = null;
        if (unitTable == null)
        {
            return unitTable = DatabaseHandler.GetUnitList();
        }
        else
        {
            return unitTable;
        }
    }

    public static DataTable GetCurrencyList()
    {
        //import lists each time the application is run
        currencyTable = null;
        if (currencyTable == null)
        {
            return currencyTable = DatabaseHandler.GetCurrencyList();
        }
        else
        {
            return currencyTable;
        }
    }

Any help is appreciated, if there is a better way how to cache a DataTable please let me know.

Update:

Thanks to your opinions, this is the suggested method to do it, if I understood correctly:

public class BusinessLayerHandler
{
    private static DataTable unitTable;
    private static DataTable currencyTable;

    private static readonly object unitTableLock = new object();
    private static readonly object currencyTableLock = new object();

    public static DataTable GetUnitList()
    {
        //import lists each time the application is run
        //unitTable = null;

        lock (unitTableLock)
        {
            if (unitTable == null)   
            {
                return unitTable = DatabaseHandler.GetUnitList();
            }
        }
        return unitTable;
    }

    public static DataTable GetCurrencyList()
    {
        //import lists each time the application is run
        lock (currencyTableLock)
        {
            if (currencyTable == null)
            {
                return currencyTable = DatabaseHandler.GetCurrencyList();
            }
        }
        return currencyTable;
    }
}

Upvotes: 48

Views: 44205

Answers (7)

Lloyd Powell
Lloyd Powell

Reputation: 18760

I thought it would be worth adding that Double Check Locking has since been implemented in .net framework 4.0 in a class named Lazy. So if you would like your class to include the locking by default then you can use it like this:

public class MySingleton
{
    private static readonly Lazy<MySingleton> _mySingleton = new Lazy<MySingleton>(() => new MySingleton());

    private MySingleton() { }

    public static MySingleton Instance
    {
        get
        {
            return _mySingleton.Value;
        }
    }
}

Upvotes: 22

scptre
scptre

Reputation: 334

If the DataTables are read-only then you should lock them when you populate them and if they never change then they will be thread safe.

public class BusinessLayerHandler
{
    public static DataTable unitTable;
    public static DataTable currencyTable;

    private static readonly object unitTableLock = new object();
    private static readonly object currencyTableLock = new object();

    public static DataTable GetUnitList()
    {
        //import lists each time the application is run
        lock(unitTableLock)
        {
            if (unitTable == null)
            {
                unitTable = DatabaseHandler.GetUnitList();
            }
        }

        return unitTable;
    }

    public static DataTable GetCurrencyList()
    {
        //import lists each time the application is run
        lock(currencyTableLock)
        {
            if (currencyTable == null)
            {
                currencyTable = DatabaseHandler.GetCurrencyList();
            }
        }

        return currencyTable;
    }
}

If you need really high performance on this lookup you can use the ReaderWriterLockSlim class instead of a full lock everytime to limit the number of waits that will happen in the application.

Check out http://kenegozi.com/blog/2010/08/15/readerwriterlockslim-vs-lock for a short article on the differences between lock and ReaderWriterLockSlim

EDIT: (Answer to comments below)

The unitTableLock object is used like a handle for the Monitor class in to synchronize against.

For a full overview of Theading and synchronization in the .NET framework I would point you over to this very extensive tutorial http://www.albahari.com/threading/

Upvotes: 1

Adam Houldsworth
Adam Houldsworth

Reputation: 64467

It appears as though all you want to do is load it once and keep a reference to it. All you need to guard is initialising the variable if it's null. Null checking, locking and null checking again is called Double Check Locking and will work well for you. It's best practice to provide a separate locking object, so you have good control over granularity of locks.

Note this doesn't stop people from mutating the value inside the DataTable it only stops people from trying to initialise the static member at the same time.

private static readonly object UnitTableLock = new object();
private static DataTable unitTable;
private static bool _ready = false;

public static DataTable GetUnitList()
{
    if (!_ready)
    {
        lock (UnitTableLock)
        {
            if (!_ready)
            {
                unitTable = new DataTable; //... etc
                System.Threading.Thread.MemoryBarrier();
                _ready = true;
            }
        }
    }

    return unitTable;
}

Only read from the result of GetUnitList never write to it.

Amended with reference to http://en.wikipedia.org/wiki/Double-checked_locking

Upvotes: 36

gjvdkamp
gjvdkamp

Reputation: 10516

I think you should be fine. There is a liight chance that 2 threads will determine that the datatable is null and both read the table, but only one gets to assign the unitTable / currencyTable reference last, so worst case you be initalizing them more than once. But once they're set I think you'd be good. AS LONG AS YOU DON'T WRITE TO THEM. Theat could leave one in an inconsistent state.

If you want to avoid the double init you could wrap the whole getter code in a lock statement. It's a lot like initializing a singleton.

Also add a method that let's you set the references to null again so you can force a refresh.

GJ

Upvotes: 2

alun
alun

Reputation: 3441

If you are on .net 4 you could use ThreadLocal wrappers on your datatables

Upvotes: 6

Gopher
Gopher

Reputation: 927

They are not thread safe. You should think about making your logic thread safe by your self, for example, by using lock operator.

Upvotes: 9

Matteo Mosca
Matteo Mosca

Reputation: 7448

Static variables aren't thread safe per-se. You should design with thread safety in mind.

There's a good link to get you started: http://en.csharp-online.net/Singleton_design_pattern%3A_Thread-safe_Singleton

Apart from this, I would strongly recommend you to use a more modern approach than the legacy DataTable. Check out the Entity Framework or NHibernate. Implementing them in your datalayer will allow you to hide database details from the rest of the software and let it work on a higher level abstraction (POCO objects).

Upvotes: 2

Related Questions