Поточно-безопасное использование членов синглтона

Цитировать:

if (value == 0) { value = 1; }
if (value == 1) { value = 0; }
return value;

value всегда будет 0 ...

Ваш поток может остановиться на середине этого метода и передать управление другому потоку. Вам нужен критический раздел вокруг этого кода ...

private static object _lockDummy = new object();


...

lock(_lockDummy)
{
   //do stuff
}

Что ж, я на самом деле не так хорошо знаю C# ... но я хорошо разбираюсь в Java, поэтому я дам ответ на этот вопрос, и, надеюсь, оба они достаточно похожи, чтобы быть полезными. Если нет, прошу прощения.

Ответ - нет, это небезопасно. Один поток может вызвать Toggle () одновременно с другим, и возможно, хотя и маловероятно с этим кодом, что Thread1 может установить value между моментами, когда Thread2 его проверяет, и когда он его устанавливает.

Чтобы исправить это, просто сделайте Toggle () synchronized. Он ничего не блокирует и не вызывает ничего, что могло бы порождать другой поток, который мог бы вызвать Toggle (), так что все, что вам нужно сделать, это сохранить.

Как отмечает Бен, исходная реализация не является потокобезопасной.

Простой способ сделать его потокобезопасным - ввести оператор блокировки. Например. так:

public class MyClass
{
    private Object thisLock = new Object();
    private static readonly MyClass instance = new MyClass();
    public static MyClass Instance
    {
        get { return instance; }
    }
    private Int32 value = 0;
    public Int32 Toggle()
    {
        lock(thisLock)
        {
            if (value == 0) 
            {
                value = 1; 
            }
            else if (value == 1) 
            { 
                value = 0; 
            }
            return value;
        }
    }
}

Я бы также добавил в MyClass защищенный конструктор, чтобы компилятор не создавал общедоступный конструктор по умолчанию.

Is access through 'Instance' to the 'Toggle()' class threadsafe? If yes, by what assumptions, rules, etc. If no, why and how can I fix it?

Нет, это небезопасно.

По сути, оба потока могут запускать функцию Toggle одновременно, так что это могло произойти.

    // thread 1 is running this code
    if (value == 0) 
    {
        value = 1; 
        // RIGHT NOW, thread 2 steps in.
        // It sees value as 1, so runs the other branch, and changes it to 0
        // This causes your method to return 0 even though you actually want 1
    }
    else if (value == 1) 
    { 
        value = 0; 
    }
    return value;

Вам нужно исходить из следующего предположения.

Если работают 2 потока, они могут и будут чередоваться и случайным образом взаимодействовать друг с другом в любой момент. Вы можете наполовину записать или прочитать 64-битное целое число или число с плавающей запятой (на 32-битном процессоре), а другой поток может подключиться и изменить его из-под вас.

Если два потока никогда не обращаются к чему-либо общему, это не имеет значения, но как только они это сделают, вам нужно не дать им наступить друг другу на ноги. В .NET это можно сделать с помощью блокировок.

Вы можете решить, что и где заблокировать, подумав о таких вещах:

Будет ли это иметь значение для данного блока кода, если значение something будет изменено из-под меня? Если это так, вам нужно заблокировать этот something на время действия кода, где это будет иметь значение.

Снова посмотрим на ваш пример

    // we read value here
    if (value == 0) 
    {
        value = 1; 
    }
    else if (value == 1) 
    { 
        value = 0; 
    }
    // and we return it here
    return value;

Чтобы это вернуло то, что мы ожидаем, мы предполагаем, что value не изменится между чтением и return. Чтобы это предположение действительно было правильным, вам необходимо заблокировать value на время этого кодового блока.

Итак, вы бы сделали это:

lock( value )
{
     if (value == 0) 
     ... // all your code here
     return value;
}

ТЕМ НЕ МЕНИЕ

В .NET вы можете заблокировать только ссылочные типы. Int32 - это тип значения, поэтому мы не можем его заблокировать. Мы решаем эту проблему, вводя «фиктивный» объект и блокируя который там, где мы хотели бы заблокировать «значение».

Это то, что имеет в виду Бен Шейрман.

That is what I thought. But, I I'm looking for the details... 'Toggle()' is not a static method, but it is a member of a static property (when using 'Instance'). Is that what makes it shared among threads?

Если ваше приложение является многопоточным, и вы можете предвидеть, что несколько потоков будут обращаться к этому методу, это делает его общим для потоков. Поскольку ваш класс является синглтоном, вы знаете, что другой поток будет обращаться к ЖЕСТКОМУ объекту, поэтому будьте осторожны с потокобезопасностью ваших методов.

And how does this apply to singletons in general. Would I have to address this in every method on my class?

Как я сказал выше, поскольку это синглтон, вы знаете, что другой поток будет обращаться к одному и тому же объекту, возможно, в одно и то же время. Это не означает, что вы должны заставить каждый метод получать блокировку. Если вы заметили, что одновременный вызов может привести к повреждению состояния класса, вам следует применить метод, упомянутый @Thomas

Can I assume that the singleton pattern exposes my otherwise lovely thread-safe class to all the thread problems of regular static members?

Нет. Ваш класс просто небезопасен. Синглтон тут ни при чем.

(I'm getting my head around the fact that instance members called on a static object cause threading problems)

Это тоже не при чем.

Вы должны думать так: возможно ли в моей программе для 2 (или более) потоков одновременный доступ к этому фрагменту данных?

Тот факт, что вы получаете данные через одноэлементную или статическую переменную или передаете объект в качестве параметра метода, не имеет значения. В конце концов, это всего лишь несколько бит и байтов в оперативной памяти вашего ПК, и все, что имеет значение, - это то, могут ли несколько потоков видеть одни и те же биты.

I was thinking that if I dump the singleton pattern and force everyone to get a new instance of the class it would ease some problems... but that doesn't stop anyone else from initializing a static object of that type and passing that around... or from spinning off multiple threads, all accessing 'Toggle()' from the same instance.

Бинго :-)

I get it now. It's a tough world. I wish I weren't refactoring legacy code :(

К сожалению, многопоточность - это сложно, и вы должны относиться к вещам очень параноидально :-) Самое простое решение в этом случае - придерживаться синглтона и добавить блокировку вокруг значения, как в примерах.