Counting of threads locked

Question

Here is the code:

private int _count = 0;
public bool Ongoing
{
    get
    {
        return _count > 0;
    }
}

public void Method1(object param)
{
    new Thread(new ParameterizedThreadStart(Method2)).Start(param);
}
private void Method2(object param)
{
    _count++;
    lock (_lock)
        {
            // Something
        }
    _count--;
}

The variable _count, as you can guess, is used to count up how many threads are locked. It is initialized to 0 and only modified inside this method and I use it to know if the class is doing something.

Now the problem: sometimes _count goes below 0. It's like _count++ get sometimes ignored.

This happens very seldom, like about once every 500 times I start this method, maybe even less.

Should I declare _count as volatile maybe?

Answer 1

You need to ensure the operations are applied atomically and the value isn't cached. To achieve the first you must use Interlocked.Increment and Interlocked.Decrement, for the second mark the field as volatile:

private volatile int _count = 0;
public bool Ongoing
{
    get
    {
        return _count > 0;
    }
}

public void Method1(object param)
{
    new Thread(new ParameterizedThreadStart(Method2)).Start(param);
}
private void Method2(object param)
{
    Interlocked.Increment(ref _count);
    lock (_lock)
        {
            // Something
        }
    Interlocked.Decrement(ref _count);
}

Answer 2

As has been correctly pointed out in the accepted answer, you need to perform thread synchronization around the increment and decrement operations. This can be achieved with the Interlocked class.

However, you also need to use some memory synchronization mechanism to ensure that the up-to-date value is available to other threads. The MSDN documentation on volatile is abysmally wrong:

The MSDN documentation states that use of the volatile keyword ensures that the most up-to-date value is present in the field at all times. This is incorrect, since as we’ve seen, a write followed by a read can be reordered. (Joseph Albahari)

Specifically, the volatile keyword instructs the compiler to generate an acquire-fence every read from that field. However, the acquire-fence takes effect after the read, in order to prevent other reads/writes from being moved before it. This means that the read itself may be moved up, allowing a stale value to be retrieved the first time the variable is read.

Unfortunately, there is no clean method for reading an integer with the required memory barriers, but the nearest candidate is a redundant Interlocked.CompareExchange (see this answer):

private int _count = 0;

public bool Ongoing => Interlocked.CompareExchange(ref _count, 0, 0) > 0;

public void Method1(object param)
{
    new Thread(new ParameterizedThreadStart(Method2)).Start(param);
}

private void Method2(object param)
{
    Interlocked.Increment(ref _count);
    lock (_lock)
    {
        // Something
    }
    Interlocked.Decrement(ref _count);
}