1

I am not an expert on multi-threading in C#; I want to be sure to prevent a race condition that would be a hard surely be a difficult to trigger in testing, while also being nearly impossible to debug. The requirement (my application is a utility class to be used in a possibly multi-threaded HTTP server, not using IIS or ASP.NET) is that each instance of a class have a unique identifier for that instance.

I would rather not use heavy-weight GUIDs to avoid the issue, due to their serialized length, as in an HTML representation.

My question is this: is the simple pattern to set the Unique value in the Widget class below a good pattern for this rquirement? Is there a better, safer pattern that is hopefully not to burdensome to implement?

public abstract class Widget : Node
{
    private static int UniqueCount = 0;
    private static object _lock = new object();
    protected int Unique { private set; get; }

    protected Widget() : base()
    {
        // There could be a subtle race condition if this is not thread-safe
        // if it is used with a multi-threaded web server
        lock (_lock)
        {
            UniqueCount += 1;
            Unique = UniqueCount;
        }
    }  

}
I4V
  • 34,891
  • 6
  • 67
  • 79
SAJ14SAJ
  • 1,698
  • 1
  • 13
  • 32
  • What do you mean unique counter values? Do you have multiple threads that are incrementing a running counter? – Codeman Feb 01 '13 at 20:29
  • Possible duplicate of http://stackoverflow.com/questions/9011908/how-to-easy-make-this-counter-property-thread-safe – Codeman Feb 01 '13 at 20:30
  • They might, that is the risk, and I don't want any instance to have a unique value shared or assigned another instance due to a race condition between the two assignments. Now that the code is in the question, it should be more clear. The shared counter is the static member `UniqueCount`. – SAJ14SAJ Feb 01 '13 at 20:31
  • Check my answer (copied from another). It should answer your question. – Codeman Feb 01 '13 at 20:32
  • I don't think it is a duplicate (or the copied answer applies) because this question is not just the inherent possible race in non-atomic `X = X + 1` type code. The safe transaction must encompass `X += 1; Y = X;' – SAJ14SAJ Feb 01 '13 at 20:34

2 Answers2

2

From this answer:

If you're looking to implement the property in such a way that DoneCounter = DoneCounter + 1 is guaranteed not to be subject to race conditions, it can't be done in the property's implementation. That operation is not atomic, it actually three distinct steps:

  1. Retrieve the value of DoneCounter.
  2. Add 1
  3. Store the result in DoneCounter.

You have to guard against the possibility that a context switch could happen in between any of those steps. Locking inside the getter or setter won't help, because that locks scope exists entirely within one of the steps (either 1 or 2). If you want to make sure all three steps happen together without being interrupted, then your synchronization has to cover all three steps. Which means it has to happen in a context that contains all three of them. That's probably going to end up being code that does not belong to whatever class contains the DoneCounter property.

It is the responsibility of the person using your object to take care of thread safety. In general, no class that has read/write fields or properties can be made "thread-safe" in this manner. However, if you can change the class's interface so that setters aren't necessary, then it is possible to make it more thread-safe. For example, if you know that DoneCounter only increments and decrements, then you could re-implement it like so:

private int _doneCounter;
public int DoneCounter { get { return _doneCounter; } }
public void IncrementDoneCounter() { Interlocked.Increment(ref _doneCounter); }
public void DecrementDoneCounter() { Interlocked.Decrement(ref _doneCounter); }
Community
  • 1
  • 1
Codeman
  • 12,157
  • 10
  • 53
  • 91
  • The question is not just the inherent possible race in `X = X + 1` type code. The safe transaction must encompass `X += 1; Y = X;'. For this reason, I don't think your old answer applies. – SAJ14SAJ Feb 01 '13 at 20:34
  • Why not just use X++ and Y++? why do you need to synchronize the two? – Codeman Feb 01 '13 at 20:36
  • I believe that `++` operators with their side effects are a bad idea and I avoid them, in favor of the 'X += 1' type pattern, which has the semantics I want in almost every case. However, that is irrelevant to the non-atomic of the race condition. Writeing the code as 'Y = X++' will generate extremely similar non-atomic bytecode if I understand correctly. – SAJ14SAJ Feb 01 '13 at 20:38
  • I meant using X++ in the context of my answer. Interlocked.Increment(ref X). `Interlocked` is indeed what you want to use for this. – Codeman Feb 01 '13 at 20:39
  • Okay, based on the other answer, now I understand what you were trying to say. Thank you. I will upvote for the information. – SAJ14SAJ Feb 01 '13 at 20:41
1

As mentioned, the += (and related) operations are not threadsafe. When doing simple number increments, just use Interlocked.Increment; it will return the old value and is fully threadsafe:

public abstract class Widget : Node
{
    private static int UniqueCount = 0;
    protected int Unique { private set; get; }

    protected Widget() : base()
    {
        Unique = Interlocked.Increment(ref UniqueCount);
    }  
}
Chris Sinclair
  • 22,858
  • 3
  • 52
  • 93
  • I am accepting this answer for its beautiful clarity. I thank you both both folks who answered (at least so far). I was completely unaware of the existence of the `Interlocked` class in the library. That is extremely helpful. – SAJ14SAJ Feb 01 '13 at 20:42
  • @SAJ14SAJ `Interlocked` is awesome; take a look at the other useful methods it has! Glad I could help! – Chris Sinclair Feb 01 '13 at 20:43