Анализатор PVS-Studio умеет выявлять неправильные варианты реализации шаблона "блокировка с двойной проверкой" в коде программ на языке C#. Иногда пользователям не понятно, в чём опасность кода, на который указывает анализатор и как его исправить. Поэтому разберём на практическом примере, как может выглядеть ошибка, обнаруживаемая с помощью предупреждения V3054, и как нужно изменить код.
Анализатор PVS-Studio может выявить ошибку небезопасной реализации шаблона "блокировка с двойной проверкой" (double checked locking). Блокировка с двойной проверкой - это шаблон, предназначенный для уменьшения накладных расходов получения блокировки. Сначала проверяется условие блокировки без синхронизации. И только если условие выполняется, поток попытается получить блокировку. Таким образом, блокировка будет выполнена, только если она действительно была необходима.
Код, реализующий данный шаблон, может быть написан недостаточно аккуратно. Особенно неприятно, что такой код может давать сбой очень-очень редко, что затрудняет выявление проблемы в коде. Так что даже если вам кажется, что программа работает корректно и код написано верно, следует внимательно отнестись к соответствующему предупреждению анализатора.
В случае выявления подозрительного кода, PVS-Studio выдаёт предупреждение V3054 [CWE-609] Potentially unsafe double-checked locking. Use volatile variable(s) or synchronization primitives to avoid this.
Рассмотрим на реальном примере, как выглядит подобная ошибка. Следующий фрагмент кода взят из проекта RunUO. Про проверку этого проекта мы недавно писали в этой статье.
private Packet m_RemovePacket;
....
private object _rpl = new object();
public Packet RemovePacket
{
get
{
if (m_RemovePacket == null)
{
lock (_rpl)
{
if (m_RemovePacket == null)
{
m_RemovePacket = new RemoveItem(this);
m_RemovePacket.SetStatic();
}
}
}
return m_RemovePacket;
}
}Анализатор PVS-Studio выдаёт предупреждение: V3054 Potentially unsafe double-checked locking. Use volatile variable(s) or synchronization primitives to avoid this. Item.cs 1624
Как видно из приведённого выше кода, блокировка с двойной проверкой была применена для реализации порождающего шаблона - Одиночки. Когда мы пытаемся получить экземпляр класса Packet, обращаясь к свойству RemovePacket,геттер осуществляет проверку поля m_RemovePacket на равенство нулю. Если проверка проходит, то мы попадаем в тело оператора lock, где и происходит инициализация поля m_RemovePacket. Интересная ситуация возникает в тот момент, когда главный поток уже инициализировал переменную m_RemovePacket через конструктор, но ещё не вызвал метод SetStatic(). Теоретически другой поток может обратиться к свойству RemovePacket именно в тот самый неудобный момент. Проверка m_RemovePacket на равенство нулю уже не пройдёт, и вызывающий поток получит ссылку на неполностью готовый к использованию объект. Для решения этой проблемы в теле оператора lock можно создавать промежуточную переменную класса Packet, инициализировать её через вызов конструктора и метод SetStatic(), а уже после присваивать её переменной m_RemovePacket. В этом случае тело оператора lock может выглядеть следующим образом:
lock (_rpl)
{
if (m_RemovePacket == null)
{
Packet instance = new RemoveItem(this);
instance.SetStatic();
m_RemovePacket = instance;
}
}Кажется, что проблема исправлена, и код будет работать как ожидается. Но это не так.
Остался ещё один момент: анализатор не просто так предлагает использовать ключевое слово volatile. В релизной версии программы компилятор может выполнить оптимизацию и переупорядочить строки вызова метода SetStatic() и присваивание переменной instance полю m_RemovePacket (с точки зрения компилятора семантика программы не будет нарушена). И мы опять возвращаемся к тому же, с чего и начинали - возможности получения недоинициализированной переменной m_RemovePacket. Нельзя точно сказать, когда может произойти это переупорядочивание, и случится ли оно вообще: на это может повлиять версия CLR, архитектура используемого процессора и т.п. Обезопаситься от подобного сценария всё же стоит, и одним из решений (однако не самым производительным) будет использование ключевого слова volatile. Переменная, которая будет объявлена с модификатором volatile, не будет подвержена перестановкам при проведении оптимизации компилятором. Окончательно исправленный код может выглядеть следующим образом:
private volatile Packet m_RemovePacket;
....
private object _rpl = new object();
public Packet RemovePacket
{
get
{
if (m_RemovePacket == null)
{
lock (_rpl)
{
if (m_RemovePacket == null)
{
Packet instance = new RemoveItem(this);
instance.SetStatic();
m_RemovePacket = instance;
}
}
}
return m_RemovePacket;
}
}Иногда использование volatile поля может быть нежелательно из-за накладных расходов на обращение к такому полю. Не будем подробно останавливаться сейчас на этой теме, отметив просто, что в рассматриваемом примере атомарная запись поля нужна лишь один раз (при первом обращении к свойству), однако объявление такого поля, как volatile, приведёт к тому, что каждое его чтение и запись компилятор будет делать атомарно, что может оказаться неоптимальным с точки зрения производительности.
Поэтому, другим подходом к исправлению данного предупреждения анализатора, позволяющим избежать дополнительных накладных затрат от объявления volatile поля, будет использование типа Lazy<T> для backing поля m_RemovePacket вместо блокировки с двойной проверкой. В этом случае тело геттера может быть заменено методом-инициализатором, который будет передаваться в конструктор экземпляра Lazy<T>:
private Lazy<Packet> m_RemovePacket = new Lazy<Packet>(() =>
{
Packet instance = new RemoveItem(this);
instance.SetStatic();
return instance;
}, LazyThreadSafetyMode.ExecutionAndPublication);
....
public Packet RemovePacket
{
get
{
return m_RemovePacket.Value;
}
}Метод-инициализатор будет вызван один раз, при первом обращении к экземпляру типа Lazy, причём потоковая безопасность в случае одновременного обращения к свойству из нескольких потоков будет гарантироваться самим типом Lazy<T> (режим потокобезопасности управляется вторым параметром конструктора Lazy).