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.

I believe 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. — quaabaam, Jun 15 '22 at 21:45
*"Please note that I know I could have used SpinLock"* -- Please show us how you could use a [`SpinLock`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.spinlock) to solve this problem. It looks to me an extremely inappropriate mechanism for this kind of problem. If you don't want to show us the use of `SpinLock`, you might want to remove this claim from the question, because it serves only as a distraction from the main issue. — Theodor Zoulias, Jun 15 '22 at 23:20
You should show us where and how you are calling the code that you've posted in the question. Most likely you are creating multiple instances of the class that contains the `object _objLock`, and so you are using a different locker each time. — Theodor Zoulias, Jun 15 '22 at 23:26
As a side note, an easy way to get rid of the overlapping execution problem that is inherent to timers, is to use an asynchronous loop and the new `PeriodicTimer` class. You can find an example [here](https://stackoverflow.com/questions/30462079/run-async-method-regularly-with-specified-interval/62724908#62724908). — Theodor Zoulias, Jun 15 '22 at 23:29
@TheodorZoulias - I did wonder if there might have been multiple instances of the lock object, but I thought quaabaam's suggestion was worth chasing down first. — Enigmativity, Jun 16 '22 at 00:15
@TheodorZoulias, I can use a SpinLock instead of a regular lock (critical section) but it would not solve the problem. It would just be more efficient in my specific case. I think quaabaam has the solution and I ask him to create an answer for it. My try/finally should be after my lock otherwise it will always be called. By the way I only talk about spinlock to prevent any answer suggesting it, just in case. Probably I shouldn't say anything about it. — Eric Ouellet, Jun 16 '22 at 13:31
Don't worry, the chances of someone suggesting a `SpinLock` instead of the `lock` are close to zero. Most likely the `lock` outperforms the `SpinLock` anyway, because in case of contention it [spins for a while](https://stackoverflow.com/questions/5111779/lock-monitor-internal-implementation-in-net) before yielding to the OS. Personally I've never used the `SpinLock`, and I have written quite a lot of multithreaded code. I've never encountered a scenario where this mechanism would be a good fit. — Theodor Zoulias, Jun 16 '22 at 14:21
[Here](https://www.albahari.com/threading/part5.aspx#_SpinLock) is what Joseph Albahari has to say about it: *"A `SpinLock` makes the most sense when writing your own reusable synchronization constructs. Even then, a spinlock is not as useful as it sounds. It still limits concurrency. And it wastes CPU time doing nothing useful. Often, a better choice is to spend some of that time doing something speculative — with the help of `SpinWait`."* — Theodor Zoulias, Jun 16 '22 at 14:25
Presumably in [Visual Studio](https://en.wikipedia.org/wiki/Microsoft_Visual_Studio)'s debugger? What version of the Visual Studio? What version of .NET? — Peter Mortensen, Jun 30 '22 at 11:38

score 1 · Accepted Answer · edited Jun 30 '22 at 11:40

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

edited Jun 30 '22 at 11:40

Peter Mortensen

30,738
21
105
131

answered Jun 16 '22 at 17:28

quaabaam

1,808
1
7
15

Thank you very much for your answer. I think their is a difference in the behavior, in my code: exception will be thrown (go up the stack) while in yours they will all be swallowed. I think you have at least to add a "throw" at the end of the catch. Also it seems complicated for no reason. Having 2 locks instead of 1, to my opinion, will increase the risk of having more thread switching (due to a thread trying to acquire a lock that is already owned). So I think my proposed solution seems more efficient. But you are the one who wake me up. Can you take my code or add a throw? I want u to win! – Eric Ouellet Jun 16 '22 at 19:53
My focus was on getting your code to only call the work logic once. So, as for the `throw`, yeah, no worries if that's needed then simply add it (which I have). Also, my code only has 1 lock, same as your code. There are not 2 locks in my code. – quaabaam Jun 16 '22 at 20:02
Thank you very much for your help!!! I was sure I didn't need 2 locks but I need them, at least in that design. – Eric Ouellet Jun 16 '22 at 20:06
The `DoWork` variable is a local method variable, each caller will have their own `DoWork` variable, there is no locking on this variable. Without the `DoWork` variable you would need to add your actual work logic within your lock scope, which may not be what you want as it would hold the lock longer than needed. Using the local `DoWork` allows the actual work logic to happen outside the lock while ensuring the caller of the method actual should do the work. – quaabaam Jun 16 '22 at 20:10
1

@EricOuellet - Please avoid the `try`/`catch` where possible. Only catch specific exceptions that you can meaningfully handle. Otherwise you make your code buggy and hard to reason about. – Enigmativity Jun 16 '22 at 22:08
I'm downvoting because this is not an answer to the question asked, which is *"Please explain why I have 3 or more?"* The OP asked for an explanation about why their code is not working as they expected. They didn't ask how to fix their code. *If* they had asked that question, then I would still downvote this answer, because the correct response IMHO is *"throw all this mess away, and learn how to use the built-in [`Monitor`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.monitor) class"*. – Theodor Zoulias Jun 17 '22 at 18:21
@TheodorZoulias That's fair, how would OP implement your suggestion? I'd be happy to upvote your solution as that's something I need to learn too. – quaabaam Jun 17 '22 at 19:11
Most likely the [`Monitor.TryEnter`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.monitor.tryenter) API could be used in the OP's scenario. But I am not 100% sure because they haven't described their scenario in details. Instead they are focusing on how to invent their own synchronization mechanism from scratch. – Theodor Zoulias Jun 17 '22 at 19:44

score -2 · Answer 2 · answered Jun 17 '22 at 15:28

-2

Just for reference, this is my final code used:

object _objLock = new object();
bool _isCurrentlyUpdatingValues = false;
public void UpdateReadValuesTimerCallback(object state)
{
    if (_isCurrentlyUpdatingValues)
    {
        return;
    }

    lock (_objLock)
    {
        if (_isCurrentlyUpdatingValues)
        {
            return;
        }
        _isCurrentlyUpdatingValues = true;
    }

    try
    {
        // My work
    }
    finally
    {
        lock (_objLock)
        {
            _isCurrentlyUpdatingValues = false;
        }
    }
}

answered Jun 17 '22 at 15:28

Eric Ouellet

10,996
11
84
119

2

I didn't downvote this post, but the `return`s feel like they make it more complex that it needs to be. I edited my answer with a version that uses `finally`. – quaabaam Jun 17 '22 at 17:08
Just lock over the whole thing - locks are expensive to take - and remove the `try`/`finally`. – Enigmativity Jun 17 '22 at 22:58
@Enigmativity, I know lock are expensive and you are right about it. But the thing that does not appear here: my code call a function that has to go over the network and takes time in order of many milliseconds. So although taking a lock is relatively slow in CPU instruction time, it is super fast when you compre it to any I/O. So in order to minimize to chance to lock thread so long for no reason, I decided to go with this choice. But I would have use only one lock, like you suggested, if all the code would have not implied any I/O (like going over the network). Thanks for you comment. – Eric Ouellet Jun 19 '22 at 02:55

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

2 Answers2