Multithreading error: Why is there more than one thread in a loop where I expected only one at a time?

Question

Offending code:

object _objLock = new object();
bool _isCurrentlyUpdatingValues = false;
public void UpdateTestReadValuesTimerCallback(object state)
{
    try
    {
        lock (_objLock)
        {
            if (_isCurrentlyUpdatingValues == true)
            {
                return;
            }

            _isCurrentlyUpdatingValues = true;
        }

        foreach (var prop in CommConfigDnp3.Dnp3Properties)
        {
            if (prop.ReadOrWrite == ReadOrWrite.Read)
            {
                if (prop.PropertyType == PropertyType.Bool)
                {
                    bool? value = CommConfigDnp3.ReadBinary(prop.PropertyName);
                    prop.LastValue = value;
                }
                else
                {
                    double? value = CommConfigDnp3.ReadDouble(prop.PropertyName);
                    prop.LastValue = value;
                }
            }
        }
    }
    finally
    {
        _isCurrentlyUpdatingValues = false;
    }

On the breakpoint (see the image below), there are three threads into the foreach. I thought it wouldn't be possible. I thought that there only could be one at the time. What's my bug?

The method is called on a timer each 10 seconds. When I debug, the 10 seconds timer times out. It happens quickly, so many threads calls the method in a debug session, but I thought only one at a time could reach the foreach loop. Why do I have three or more? See the selected threads in the attached image where it shows three threads in the foreach loop (directly in the foreach loop or somewhere in the callstack of the foreach loop).

Please note that I know I could have used SpinLock, but that is not part of my problem.

Answer 1

Why is there more than one thread in a loop where I expected only one at a time?

Finally is always called. Your finally block is getting hit in all cases, even after you call return. So, even though it's already true and you return, your code still calls the finally block setting it to false.

Use a local method variable to indicate work can proceed, and only set it to true if the lock is acquired.

try
{
    bool doWork = false;
    lock (_objLock)
    {
        if (!_isCurrentlyUpdatingValues)
        {
            _isCurrentlyUpdatingValues = true;
            doWork = true;
        }
    }

    if (doWork)
    {
        // do work here...
        lock (_objLock)
        {
            _isCurrentlyUpdatingValues = false;
        }
    }
}
catch
{
    lock (_objLock)
    {
        _isCurrentlyUpdatingValues = false;
    }
    throw;
}

You could replace the catch with finally and push the doWork variable outside the try block, then doWork could be checked in the finally block to determine if the _isCurrentlyUpdatingValues should be set to false.

bool doWork = false;
try
{
    lock (_objLock)
    {
        if (!_isCurrentlyUpdatingValues)
        {
            _isCurrentlyUpdatingValues = true;
            doWork = true;
        }
    }
    if (doWork)
    {
        // do work here...
    }
}
finally
{
    if (doWork)
    {
        lock (_objLock)
        {
            _isCurrentlyUpdatingValues = false;
        }
    }
}
```