Is this class thread safe?

Question

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++);
    }

}

score 8 · Accepted Answer · edited May 23 '17 at 11:47

8

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.

edited May 23 '17 at 11:47

Community

1
1

answered Dec 07 '10 at 00:12

cdhowie

158,093
24
286
300

Thank you for your elaborate answer. Just curious though, why not use singletons, especially in this case? – Behrooz Karjoo Dec 07 '10 at 01:16
@Behrooz: Because singletons effectively limit flexibility for no good reason. Pretend you expanded your service/application to handle order numbers for two databases. A singleton would make this impossible. You should instead use dependency injection or some "context" object passed around, referencing an `OrderIDGenerator` instance. – cdhowie Dec 07 '10 at 01:28

score 3 · Answer 2 · answered Dec 07 '10 at 00:18

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.

As a note: CDHowie is right. Interlocked.Increment(ref orderCount) is better than using the entire lock statement. — Adam Norberg, Dec 07 '10 at 00:20
+1 very clean explanation of the thread un-safety problem! Congrats! — Lorenzo, Dec 07 '10 at 00:21

Jeremy Fuller · Answer 3 · 2010-12-07T00:17:34.003

1

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++);
    }
}

edited Dec 07 '10 at 00:17

answered Dec 07 '10 at 00:10

Jeremy Fuller

3,391
1
32
27

1

Locking is overkill too; `Interlocked.Increment` is more appropriate for this scenario. – cdhowie Dec 07 '10 at 00:12
Ah yes, of course. Leaving my original answer for posterity (but go with cdhowie's answer above). – Jeremy Fuller Dec 07 '10 at 00:15

score 0 · Answer 4 · answered Dec 07 '10 at 00:13

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;
    }
}

Is this class thread safe?

4 Answers4