0

I've written the piece of code below for an integration test which calls an expensive function DoSomething() for several different assemblies. I would have thought applying a lock would be enough to ensure thread safety but sometimes the boolean value is true when it should always be false in my current test case.

I've also tried the solution here with the Interlocked.CompareExchange which doesn't seem to work for me as well.

Can someone please point me in the right direction as to what exactly I'm doing wrong?

public class SomeClass
{
    private object _lock = new object();
    private volatile bool _isSuccessful = true;

    private bool IsSuccesful
    {
        get
        {
            lock (_lock)
            {
                return _isSuccessful;
            }
        }
        set
        {
            lock (_lock)
            {
                _isSuccessful = value;
            }
        }
    }

    public bool Get()
    {
        Parallel.ForEach(..., ... =>
        {
            IsSuccesful = IsSuccesful & DoSomething();
        });

        return IsSuccesful;
    }
}
Vqf5mG96cSTT
  • 2,561
  • 3
  • 22
  • 41
  • Can you work this up into a verifiable example of it failing? – stuartd Jul 04 '18 at 15:17
  • Isn't this just going to get you the _last_ value of `IsSuccesful`, and as you're running multiple tasks in parallel you don't know which one is the last to run? – stuartd Jul 04 '18 at 15:28
  • @stuartd Here's a test project: https://github.com/bdebaere/ThreadSafeBool/tree/master/ThreadSafeBool – Vqf5mG96cSTT Jul 04 '18 at 15:35
  • Not sure what you're trying to do here. If you just want to know if any of them fail, why not have a `Failed` property that you set when a task fails? – stuartd Jul 04 '18 at 15:46
  • @stuartd Can you explain what you mean? I thought this was an implementation of that: a boolean value which I set based on the result of the tasks. – Vqf5mG96cSTT Jul 04 '18 at 15:49
  • Not quite the same. If a test fails, set `Failed` as true. If another test succeds, then `Failed` is still true. If another test fails, `Failed` is still true. If you wanted to know _how many_ failed, that would be a bit more involved. – stuartd Jul 04 '18 at 15:52

2 Answers2

2

You said DoSomething() is expensive (so I guess time consuming).

Now imagine two parallel calls, one where DoSometing() is successful (A) and one that fails (B). This is what obviously may happen:

  1. A checks IsSuccessful which is true, so DoSomething is called
  2. B checks IsSuccessful which still is true, so DoSomething is called
  3. DoSomething returns false for B (well, it happened to be a little bit faster) and so B sets IsSuccessful to false.
  4. Now DoSomething returns true for A. Now A has true & true (because IsSuccessful was true when it read it) and hence sets IsSuccessful to true again.

The problem you face is because you use seperate methods for checking the value and setting the value. It's a classic TOCTTOU problem.

I suggest to use Interlocked.Increment, but on a single int inside the Parallel.ForEach.

public bool Get()
{
    int failed = 0;
    Parallel.ForEach(..., ... =>
    {   
        if (!DoSomething())         
            Interlocked.Increment(ref failed);
    });

    return failed == 0;
}

If you're only interested if one of the tests failed, you can of course simply do:

public bool Get()
{
    Parallel.ForEach(..., ... =>
    {   
        if (!DoSomething()) IsSuccessful = false;
    });

    return IsSuccessful;
}
René Vogt
  • 43,056
  • 14
  • 77
  • 99
  • Maybe I'm not following but I need the result of the 10 separate tests because I need to know if one of them fails but also run all the tests for extra debug information. So I have to create the variable outside the Parallel.ForEach() because I have to return it after all of the tests have completed, not just stop when one fails. – Vqf5mG96cSTT Jul 04 '18 at 15:37
  • @bdebaere jsut edited the answer. But notice that even if your code worked correctly, you would no longer call `DoSomething` when `IsSuccessful` is false once, because `&&` does not evaluate the right operand when the left is already false. – René Vogt Jul 04 '18 at 15:39
  • But the `&` alone doesn't solve your thread-safety problem: You **read** `IsSuccessful`, then **execute** `DoSomething`, then `&` the previoulsy read value with the return value and **set** this result to `IsSuccessful` again. If another thread changed `IsSuccessful` while that first thread was running `DoSomething`, you now overwrite this change again. – René Vogt Jul 04 '18 at 15:43
  • Does this mean you merely solved it because you increment an integer value instead of setting a boolean value based on the read value of this boolean? – Vqf5mG96cSTT Jul 04 '18 at 15:46
  • No, it means I solved it by doing the **check** and the **write** at the same time, while you were checking the value long before writing. Your thread-safe bool class only ensures that two threads cannot read and set the value at the same time, but the way you used it was wrong. You read the value, then did a long operation (and while doing that, all other threads can have changed the value) and then set the value again based on its state a long time ago without checking its current state. So your "thread-safety" was useless. – René Vogt Jul 04 '18 at 15:49
  • Thanks for your patience and explanations. I think I understand what you're saying when you're talking about what I did wrong. So basically if I write "if(!DoSomething()) IsSuccesful = false;" this will also solve my problem? – Vqf5mG96cSTT Jul 04 '18 at 15:55
  • Just out of curiosity. What if I wrote "IsSuccessful = DoSomething() & IsSuccesful" instead of an if, would that work too? – Vqf5mG96cSTT Jul 04 '18 at 16:07
  • It could work better, but there is still no guarantee: There still is a little probability that between _reading_ `IsSuccessful` and setting it, a different thread could have set it to `false`. The problem is the getter and the setter are separated non-atomic operations. After you read `IsSuccessful`, the lock is free, another thread can take it to _set_ `IsSuccessful` and the first thread waits to get the lock and overwrites it again. Your locks do not span over both operations. – René Vogt Jul 04 '18 at 16:12
1

The volatile should be enough. From docs:

The volatile modifier is usually used for a field that is accessed by multiple threads without using the lock statement to serialize access.

It effectively serializes the access.

That said, I don't think that's the problem

You say the test should always return false, but sometimes it returns true. In order to 'know' it will always return false, then all your DoSomething() calls must return false. Is that true? If this were a standard (serial) foreach, then it would always execute in the same order, so once a call to DoSomething returned false _isSuccessful remains false for the rest of the iteration. When you run this parallel, order is out the window. You end up with an execution order that can be different every time. This is compounded by the fact that your DoSomething calls likely have different completion times and also can run in a different order each time. Your results will be potentially be non-repeatable and inconsistent..

I assume what you really want is a IsSuccessuful that is true only if all the DoSomething's return true. One way to do this is to drop the && and use a simple if statement. For example:

public bool Get()
{
    var good = true;
    Parallel.ForEach(..., ... =>
    {
        var result = DoSomething();
        if (result == false) 
        {
            good = false;
        }
    });
    IsSuccesful = good;
    return IsSuccesful;
}

Once, 'good' becomes false there is no method to set it back to true. Therefore, if one test returns false, then IsSuccessful will return false. This might also make the intent of your code clearer to the future developers who work with it.

Community
  • 1
  • 1
Dweeberly
  • 4,668
  • 2
  • 22
  • 41