CODEWITHSUNDEEP
c#multithreading
Behrooz Karjoo
Behrooz Karjoo

Reputation: 4292

Is this class thread safe?

I am implementing this class as a singleton. I am not good at thread safety. Wanted to make sure the GenerateOrderID class is thread safe. More specifically that the orderCount variable cannot be incremented simultaneously by different objects and throw the count off. 

public class OrderIDGenerator

{

    private static readonly OrderIDGenerator instance = new OrderIDGenerator();
    private int orderCount;


    private OrderIDGenerator()
    {
        orderCount = 1;
    }


    public static OrderIDGenerator Instance
    {
        get { return instance; }
    }

    public string GenerateOrderID()
    {
        return String.Format("{0:yyyyMMddHHmmss}{1}", DateTime.Now, orderCount++);
    }

}

Upvotes: 1

Views: 222

Answers (4)

cdhowie
cdhowie

Reputation: 169008

It is not. The post-increment operation is not atomic. You will need to make the following changes:

Replace the GenerateOrderID method with this:

public string GenerateOrderID()
{
    return String.Format("{0:yyyyMMddHHmmss}{1}",
                         DateTime.Now,
                         Interlocked.Increment(ref orderCount));
}

And initialize orderCount to 0 instead of 1, since Interlocked.Increment returns the incremented value. (In other words, Interlocked.Increment(ref foo) is identical in every way to ++foo except that it is atomic, and therefore thread-safe.)

Note that Interlocked.Increment is much more efficient than the use of lock to synchronize threads, though lock will still work. See this question.

Also, don't use singletons.

Upvotes: 8

Adam Norberg
Adam Norberg

Reputation: 3076

This class is not threadsafe.

Increment is not an atomic operation. It is therefore entirely possible that a context switch happens at just the wrong time:

Start with orderCount at 1.
Two threads try to GenerateOrderID() at once:

Thread 1             |    Thread 2
----------------------------------------------
read orderCount = 1  |
                    -->
                     |  read orderCount = 1
                     |  add: 1 + 1
                     |  write orderCount = 2
                    <--
add: 1 + 1           |
write orderCount = 2 |
return timestamp1    |
                    -->
                     |  return timestamp1

You now have a duplicate order ID with your order count thrown off.

To fix this, lock orderCount while accessing it:

public string GenerateOrderID()
{
    lock(this){
        return String.Format("{0:yyyyMMddHHmmss}{1}", DateTime.Now, orderCount++);
    }
}

The "lock" statement only lets one thread in at a time, for each object being locked upon. If one thread is in GenerateOrderID(), then until it completes it, your OrderIDGenerator will be locked and the next thread trying to claim the lock will have to wait until the first thread is completely done.

Read more about the lock statement here.

Upvotes: 3

Jeremy Fuller
Jeremy Fuller

Reputation: 3401

I would mark orderCount as volatile (to prevent compiler optimization assumptions) and employ a lock around the increment. The volatile is probably overkill but it doesn't hurt.

public class OrderIDGenerator
{

    private static readonly OrderIDGenerator instance = new OrderIDGenerator();
    private volatile int orderCount;
    private static object syncRoot = new object();


    private OrderIDGenerator()
    {
        orderCount = 1;
    }


    public static OrderIDGenerator Instance
    {
        get { return instance; }
    }

    public string GenerateOrderID()
    {
        lock (syncRoot)
            return String.Format("{0:yyyyMMddHHmmss}{1}", DateTime.Now, orderCount++);
    }
}

Upvotes: 1

Lorenzo
Lorenzo

Reputation: 29427

You can simply have it to be thread safe with a small modification

public class OrderIDGenerator {
    private static readonly OrderIDGenerator instance = new OrderIDGenerator();
    private int orderCount;
    private static readonly object _locker = new object();

    private OrderIDGenerator() {
            orderCount = 1;
    }

    public static OrderIDGenerator Instance {
            get { return instance; }
    }

    public string GenerateOrderID() {
        string orderId = "";
        lock (_locker) {
            orderID = String.Format("{0:yyyyMMddHHmmss}{1}", DateTime.Now, orderCount++);
        }
        return orderID;
    }
}

Upvotes: 0

Related Questions