Почему это логическое значение не является потокобезопасным?

Question

Я написал фрагмент кода ниже для интеграционного теста, который вызывает дорогую функцию DoSomething () для нескольких различных сборок. Я бы подумал, что применения блокировки будет достаточно для обеспечения безопасности потока, но иногда логическое значение равно true, когда оно всегда должно быть ложным в моем текущем тестовом примере.

Я также попробовал решение здесь с Interlocked.CompareExchange, который, похоже, не работает и для меня.

Может ли кто-нибудь указать мне правильное направление относительно того, что именно я делаю неправильно?

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

Answer 1

volatile должно быть достаточно. Из документов:

Модификатор volatile обычно используется для поля, к которому обращается несколько потоков без использования оператора блокировки для сериализации доступа.

Эффективно сериализует доступ.

Тем не менее, я не думаю, что это проблема

Вы говорите, что тест всегда должен возвращать false, но иногда он возвращает true. Чтобы «знать», он всегда будет возвращать false, тогда все ваши вызовы DoSomething () должны возвращать false. Это правда? Если бы это был стандартный (последовательный) foreach, он всегда выполнялся бы в одном и том же порядке, поэтому, как только вызов DoSomething вернул false, _isSuccessful остается ложным до конца итерации. Когда вы запускаете эту параллель, порядок выходит из окна. В итоге вы получаете порядок выполнения, который может отличаться каждый раз. Это усугубляется тем фактом, что ваши вызовы DoSomething, вероятно, имеют разное время завершения и могут каждый раз выполняться в разном порядке. Ваши результаты могут быть неповторяемыми и непоследовательными.

Я предполагаю, что вы действительно хотите IsSuccessuful, который верен, только если все DoSomething возвращают true. Один из способов сделать это - удалить && и использовать простое выражение if. Например:

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

Как только «хороший» становится ложным, нет способа вернуть его в значение «истина». Следовательно, если один тест возвращает значение false, то IsSuccessful вернет значение false. Это также может прояснить смысл вашего кода для будущих разработчиков, которые с ним работают.

Answer 2

Вы сказали, что DoSomething() дорого (так что я думаю время отнимает).

Теперь представьте два параллельных вызова, один из которых DoSometing() успешен (A), а другой - неудачен (B). Вот что, очевидно, может случиться:

1. A проверяет IsSuccessful, что true, поэтому DoSomething называется
2. B проверяет IsSuccessful, который все еще равен true, поэтому DoSomething называется
3. DoSomething возвращает false для B (ну, это оказалось немного быстрее), и поэтому B устанавливает IsSuccessful в false.
4. Теперь DoSomething возвращает true для A. Теперь A имеет true & true (потому что IsSuccessful было true при его чтении) и, следовательно, устанавливает IsSuccessful в true еще раз.

Проблема, с которой вы сталкиваетесь, заключается в том, что вы используете отдельные методы для проверки значения и установки значения. Это классическая проблема TOCTTOU .

Я предлагаю использовать Interlocked.Increment, но на одном int внутри Parallel.ForEach.

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

    return failed == 0;
}

Если вам интересно только, если один тестов не пройден, вы, конечно, можете просто сделать:

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

    return IsSuccessful;
}