пятница, 4 марта 2011 г.

Почему плохо использовать lock(this) в C# ??


На днях на в фирме встал вопрос "Почему нельзя использовать lock(this) для синхронизации потоков?". На мелкомягком MSDN про это пишут так:

lock (this) может привести к проблеме, если к экземпляру допускается открытый доступ.
Возьмем простенький пример:

У нас есть некоторый объект, который асинхронно(в отдельном потоке) проверяет сам себя и каждые 3 секунды выдает на консоль сообщение "Все в порядке...продолжаю диагностику". Чтобы было поинтереснее, дадим нашему объекту имя и количество денег.

public class AutoTestingObj
{
protected String m_name;
protected int m_cash;

public String Name
{
  get
    {
       return m_name;
    }
    set
    {
       m_name = value;
    }
}

public int Cash
{
  get
    {
       return m_cash;
    }
    set
    {
       m_cash = value;
    }
}
}


Теперь добавим логику тестирования в отдельном потоке. При этом нужно как-то фиксировать переменные m_cash и m_name, чтобы никто из другого потока их не изменил во время тестирования. На первый взгляд логичнее и проще всего залочить весь объект и да будет на счастье. Код выглядит так:

public class AutoTestingObj
{
Thread m_thread;

protected String m_name;
protected int m_cash;

public String Name
{
  get
{
    return m_name;
    }
    set
    {
       m_name = value;
    }
}

public int Cash
{
  get
    {
       return m_cash;
    }
    set
    {
       m_cash = value;
    }
 }
public AutoTestingObj()
 {
    m_thread = new Thread(this.TestSystem);
}

public void Start()
{
  m_thread.Start();
}

public void TestSystem()
{
  for (; ; )
    {
       //лочим весь объект, чтобы никто не изменил
       lock (this)
       {
          //проводим какое-то тестирование в течении 3 секунд
          Thread.Sleep(3000);
          Console.WriteLine("Все в порядке...продолжаю диагностику");
       }
    }
}
}

Вроде бы все прекрасно. Обезопасили объект от многопоточного произвола. Правда мы не заметили нескольких неприятных моментов.
  1. Мы заблокировали объект для других потоков. Если этот объект понадобиться в другом потоке, надо будет ждать пока объект освободиться. Не очень удобно. Логичнее было бы добиться запрета на изменение(запись) полей, а чтение разрешить.
  2. Мы обезопасили объект только в случаем, когда везде перед переменной ставим lock(...). Вообще говоря, ничего мы этим способом не обезопасили, свойства Name и Cash легко можно изменить из любого потока обратившись к ним вне lock блока.
  3. Самая интересная причина. Если другой наш коллега захочет использовать экземпляр класса для синхронизации потоков, его ожидает бооольшой сюрприз. Поясню суть на примере. Написана вот такая программа

static void Main(string[] args)
{
AutoTestingObj inst = new AutoTestingObj();
inst.Start();

Thread.Sleep(100);

Console.WriteLine("Запланированна работа на 5 секунд");

DateTime start = DateTime.Now;
lock (inst)
{
  Thread.Sleep(5000);
}
TimeSpan delta = DateTime.Now - start;

Console.WriteLine("Работа выполнялась {0} секунд", delta.TotalSeconds);
}


Сколько выполнялась работа? Ответ: примерно 7.9 секунды(думаю, понятно откуда такой результат). А чтобы все было совсем хорошо, представим что у автора программы нету исходников нашего класса AutoTestingObj. Не хотел бы я оказаться в таком положении.

    Чтобы избежать этих трех неприятностей лучше действовать по хорошо известной схеме: залачивать можно только private члены класса. Например так:

    public class Locker { }

    public class AutoTestingObj
    {
    Thread m_thread;

    private Locker m_locker; //Locker-экземпляр любого класса(не путать со struct)

    protected String m_name;
    protected int m_cash;

    public String Name
    {
      get
        {
           return m_name;
        }
        set
        {
           lock (m_locker)
           {
              m_name = value;
           }
        }
    }

    public int Cash
    {
      get
        {
           return m_cash;
        }
        set
        {
           lock (m_locker)
           {
              m_cash = value;
           }
        }
    }

    public AutoTestingObj()
    {
      m_locker = new Locker();
        Name = "No name";   
        m_thread = new Thread(this.TestSystem);
    }

    public void Start()
    {
      m_thread.Start();
    }

     public void TestSystem()
    {
      for (; ; )
        {
           lock (m_locker)
           {
              Thread.Sleep(3000);
              Console.WriteLine("Все в порядке...продолжаю диагностику");
           }
        }
    }
    }

    Все 3 проблемы решили. Но стоит помнить, что присвая новое значение свойству Name или Cash поток будет ждать пока освободиться m_locker.

    3 комментария:

    1. Извините, а вы считаете, что Lock для геттеров не нужно использовать ?

      ОтветитьУдалить
    2. Дмитрий, спасибо за интересный вопрос.

      Для геттеров lock необходимо использовать, если в геттере завернута сложная логика работы с объектом. Если lock не использовать, то в процессе выполнения второй поток может установить новое значение переменной и логика первого потока сломается.
      Давайте посмотрим нужен ли lock в приведенном примере. На первый взгляд lock действительно нужен. Может возникнуть ситуация, когда во время выполнения геттера другой поток выставит новое значение переменной и геттер вернет неактуальное значение свойства. Это не хорошо и первое что хочется сделать - поставить lock. Только вот lock в геттере не спасет, можно придумать ситуацию, когда геттер вернет неактуальное значение переменной (если с этим возникнут трудности могу подсказать). Операиии чтения и установки значения ссылочной переменной являются атомарными, об этом можно почитать тут . Так что lock никак не обезопасит работу геттера в данном примере.

      ОтветитьУдалить
    3. --Если этот объект понадобитЬся в другом потоке, надо будет ждать пока объект освободитЬся.

      кто б сомневался, что истинное кодерье пишет только ться ться

      ОтветитьУдалить