Мы используем куки, чтобы пользоваться сайтом
было удобно.
А что если окажется, что простые знания на самом деле более нюансированные, а старые знакомые, такие как Double-checked locking, являются неоднозначными? Именно на такие мысли наталкивает изучение кода реальных проектов. Результаты этого исследования мы и рассмотрим в этой статье.
Не так давно я — в порядке обычной деятельности по проверке открытых проектов с помощью PVS-Studio — проверял только вышедшую тогда 24-ю версию известного проекта DBeaver. Надо признать, меня приятно удивило качество его кода — недаром они используют инструменты статического анализа. Но я продолжил исследование и нашёл несколько подозрительных мест, которые меня зацепили. Зацепили настолько, что каждому я решил посвятить по статье, так что приветствую в первой части цикла.
Обновляемый список статей:
Параллельное программирование таит в себе кучу граблей. И, разумеется, не только в Java — тот же злой (или добрый :) ) брат-близнец C# позволяет легко оступиться при использовании событий в параллельных сценариях. Известно, что зачастую именно асинхронные и многопоточные ошибки сложнее всего отлавливать.
Далее мы посмотрим на три типа таких ошибок. Первой из них является неатомарное изменение volatile переменных.
Из документации мы можем узнать, что ключевое слово volatile:
Продемонстрировать работу этого ключевого слова может прозаичный пример с потоком, ожидающим изменения переменной:
private static volatile String str;
public static void main() throws InterruptedException {
new Thread(() -> {
while (str == null) { }
System.out.println(str);
}).start();
Thread.sleep(1);
str = "Hello World!";
}Если убрать volatile, то цикл будет бесконечным. По крайней мере, в моём случае. Результат может сильно зависеть от JVM и железа.
Если немного модифицировать пример, добавив связанную переменную, то станет ясен и механизм работы happens-before:
private static String str;
private static volatile boolean flag = false;
public static void main() throws InterruptedException {
new Thread(() -> {
while (!flag) { }
System.out.println(str);
}).start();
Thread.sleep(1);
str = "Hello World!";
flag = true;
}Если бы у нас была только гарантия видимости, то компилятор или процессор могли бы поменять порядок операций присваивания переменным str и flag. И тогда была бы вероятность попасть в System.out.println(str) в моменте, когда str ещё равняется null.
Тем не менее принцип happens-before гарантирует, что запись, находящаяся перед записью в volatile переменную, останется там же.
Несмотря на то, что у нас обеспечена видимость переменной, этого, конечно, не хватит для корректной работы в параллельных потоках:
private static volatile int counter = 0;
private static void increment() {
Thread.sleep(1);
counter++;
}
public static void main() {
var threads = Stream.generate(() -> new Thread(Main::increment))
.limit(100)
.toList();
threads.forEach(Thread::start);
while (threads.stream().anyMatch(Thread::isAlive)) {
Thread.yield();
}
System.out.println(counter);
}Такой код будет стабильно выводить число меньшее, чем 100. Неудивительно, ведь операция инкремента не является атомарной, и один поток может влезть поперёк другого. Это может произойти как раз между операциями взятия старого значения counter и увеличения его на 1. Исправлений может быть два, одно проще другого:
Используем специальные типы данных, предоставляющие готовый интерфейс для работы из нескольких потоков. В представленном случае — AtomicInteger.
private static final AtomicInteger counter = new AtomicInteger(0);
private static void increment() {
Thread.sleep(1);
counter.incrementAndGet();
}Просто добавляем ключевое слово synchronized (либо synchronized блок). Это гарантирует, что только один поток сможет выполнить операции в этом методе.
private synchronized static void increment() {
Thread.sleep(1);
counter++;
}Если вам вдруг показалось, что это базовые знания и совершать ошибку, представленную выше, в реальных проектах никто не будет, то тут вы... почти угадали. В DBeaver я нашёл три места, где работа ведётся с volatile переменными примитивных типов. И, как это часто бывает в реальной жизни, ситуации не такие однозначные. Где-то опасную ситуацию можно притянуть, но по большей части ничего страшного произойти не может. Тем не менее вот эти три места:
public class MultiPageWizardDialog extends .... {
....
private volatile int runningOperations = 0;
....
@Override
public void run(....) {
....
try {
runningOperations++; // <=
ModalContext.run(
runnable,
true,
monitorPart,
getShell().getDisplay()
);
} finally {
runningOperations--; // <=
....
}
}
}Предупреждения PVS-Studio:
Два предупреждения по цене одного, и по совместительству единственное место, где есть сколько-то осязаемый риск. Казалось бы, имеем почти что эталонный случай идентичный тому, что был в искусственном примере выше. И всё же, если присмотреться к названию класса и метода, где всё происходит (MultiPageWizardDialog и run соответственно), то станет понятно, что мы имеем дело с UI, и настоящее состояние гонки здесь если и возможно вообще, то очень маловероятно.
Хотя если всё же представить, что метод run запустили бы одновременно из нескольких потоков, то значение runningOperations запросто могло бы оказаться некорректным.
private volatile int drawCount = 0;
....
private void showProgress() {
if (loadStartTime == 0) {
return;
}
if (progressOverlay == null) {
....
painListener = e -> {
....
Image image = DBeaverIcons.getImage(
PROGRESS_IMAGES[drawCount % PROGRESS_IMAGES.length]
);
....
};
....
}
drawCount++;
....
}Предупреждение PVS-Studio:
V6074 Non-atomic modification of volatile variable. Inspect 'drawCount'. ProgressLoaderVisualizer.java(192)
С виду самое некритичное место. drawCount используется как селектор картинки прогресса, обеспечивая циклическую прокрутку индекса. Во-первых, кажется, что цена ошибки в таком месте не самая высокая. Во-вторых, сам факт доступа к отрисовке из нескольких параллельных потоков звучит сомнительно. Если последнее предположение верно, то модификатор volatile здесь не нужен.
Анализатор выдаёт предупреждение на следующий код:
private volatile int initializedCount = 0;
....
public CompareObjectsExecutor(CompareObjectsSettings settings)
{
....
initializeFinisher = new DBRProgressListener() {
@Override
public void onTaskFinished(IStatus status)
{
if (!status.isOK()) {
initializeError = status;
} else {
initializedCount++;
}
}
};
....
}Предупреждение PVS-Studio
V6074 Non-atomic modification of volatile variable. Inspect 'initializedCount'. CompareObjectsExecutor.java(130)
И это место больше других похоже на опасное. Но пока нам не хватает контекста, надо посмотреть на место использования initializeFinisher и initializecCount:
this.initializedCount = 0;
for (DBNDatabaseNode node : nodes) {
node.initializeNode(null, initializeFinisher);
....
}
while (initializedCount != nodes.size()) {
Thread.sleep(50);
if (monitor.isCanceled()) {
throw new InterruptedException();
}
....
}Итого:
Казалось бы, эталонный случай: если два потока одновременно инкрементируют счётчик, то вся логика сломается. Так ведь? Нет, в node.initializeNode нет никаких потоков. Там вообще ничего нет :)
public boolean initializeNode(
DBRProgressMonitor monitor,
DBRProgressListener onFinish
) throws DBException {
if (onFinish != null) {
onFinish.onTaskFinished(Status.OK_STATUS);
}
return true;
}В общем, несмотря на то что DBRProgressListener действительно используется в параллельных сценариях, это не тот случай, и volatile для initializedCount кажется попросту избыточным, как и цикл с проверкой количества инициализаций.
До этого мы смотрели на простые примеры с примитивными типами. А достаточно ли сделать поле ссылочного типа volatile, чтобы гарантировать аналогичную работу? Отнюдь. Подробнее можно узнать здесь. Немного модифицировав прошлый пример, достаточно легко добиться того, чтобы у нас снова возник бесконечный цикл:
private static class Proxy {
public String str;
}
private static volatile Proxy proxy = new Proxy();
public static void main() throws InterruptedException {
var proxy = Main.proxy;
new Thread(() -> {
while (proxy.str == null) { }
System.out.println(proxy.str);
}).start();
Thread.sleep(1);
proxy.str = "Hello World!";
}Одно из возможных исправлений — отметить поле str как volatile, и тогда всё заработает как прежде. Что же до самого поля proxy, то в этом случае как-либо отмечать его смысла нет.
private static class Proxy {
public volatile String str;
}
private static Proxy proxy = new Proxy();
public static void main() throws InterruptedException {
var proxy = TaskRunner.proxy;
new Thread(() -> {
while (proxy.str == null) { }
System.out.println(proxy.str);
}).start();
Thread.sleep(1);
proxy.str = "Hello World!";
}Так что, нет никакого смысла делать ссылочное поле volatile? Вот тут мы, наконец, и подходим к паттерну Double-checked locking.
Этот паттерн используется как производительный способ реализовать отложенную инициализацию в многопоточной среде. Его суть максимально проста: перед "тяжёлой" проверкой в блоке синхронизации делается проверка обычная, и только если запрашиваемый ресурс ещё не был создан, то поток управления пойдёт дальше. Реализация и правда кажется незамысловатой:
public class HolderThreadSafe {
private volatile ResourceClass resource;
public ResourceClass getResource() {
if (resource == null) {
synchronized (this) {
if (resource == null) {
resource = new ResourceClass();
}
}
}
return resource;
}
}Паттерн чаще всего используется для создания одиночек (синглтонов) и отложенной инициализации тяжёлых объектов. Но всё ли тут так просто?
С этим паттерном связана одна забавная особенность. Он не работает. Во всяком случае, если ваш возраст примерно равен возрасту динозавров, и вы используете Java меньшей версии, чем пятая. Но не переключайтесь. На самом деле, это ключевая точка нашего повествования :)
Итак, что такого изменилось в JDK 5? В нём добавили то самое ключевое слово volatile. Тут можно сразу меня раскусить и сказать, что в примере выше resource отмечен как volatile, но я бы сделал шаг назад и посмотрел на код. А в чём, собственно, проблема? Порядок действий тут такой:
Казалось бы, зачем нам volatile, если мы используем синхронизацию для контроля доступа к полю? И правда, видимость значения для потоков тут не является камнем преткновения, но у нас есть две проблемы:
То есть последствия таких оптимизаций могут оказаться непредсказуемыми. Например, компилятор может заинлайнить конструктор и как угодно поменять порядок инициализации полей. Как результат, объект может быть отмечен как созданный, другой поток это увидит и начнёт его использовать, и найдёт там ещё неинициализированные поля. Именно от этой ситуации спасает вышеописанный принцип happens-before. И да, именно благодаря volatile переменным double-check locking всё-таки работает, начиная с пятой версии Java.
Что же побудило меня так глубоко закопаться в теорию? Очередное предупреждение анализатора на следующий метод проекта DBeaver:
private List<DBTTaskRun> runs;
....
private void loadRunsIfNeeded() {
if (runs == null) {
synchronized (this) {
if (runs == null) {
runs = new ArrayList<>(loadRunStatistics());
}
}
}
}V6082 Unsafe double-checked locking. The field should be declared as volatile. TaskImpl.java(59), TaskImpl.java(317)
Забавно, что в сборнике паттернов, на который я уже ссылался, указан вот такой минус:
Complex implementation can lead to mistakes, such as incorrect publishing of objects due to memory visibility issues.
Сложная имплементация этого паттерна может привести к ошибкам. Например, некорректная публикация объектов из-за проблем с видимостью.
Закономерно, что именно это и произошло в представленном случае :)
Возвращаясь к вопросу, заданному мною в теоретическом разделе:
Так что, нет никакого смысла делать ссылочное поле volatile?
Тогда я слукавил и намеренно опустил очевидную вещь. Когда мы меняем поле объекта, мы никак не трогаем переменную, содержащую ссылку на объект. Но вот при перезаписи этой переменной вся указанная в том разделе информация снова становится актуальной. Поэтому, хотя и с указанными ранее ограничениями (volatile поле не гарантирует безопасной публикации членов объекта, на который оно содержит ссылку), сценарии использования существуют, и это один из них.
Кажется, что исправление в этом случае самоочевидно: просто сделать список runs volatile переменной. Проблему это действительно исправит, но я предлагаю ещё раз вспомнить предназначение этого паттерна:
The Double-Checked Locking pattern aims to reduce the overhead of acquiring a lock by first testing the locking criterion (the 'lock hint') without actually acquiring the lock. Only if the locking criterion check indicates that locking is necessary does the actual locking logic proceed.
Double-checked locking преследует цель уменьшить накладные расходы получения статуса блокировки. Это происходит посредством первоочередной проверки критерия блокировки без получения блокировки как таковой. И только если проверка критерия блокировки говорит о том, что она необходима, логика блокировки задействуется.
То есть перед нами паттерн, преследующий своей целью оптимизацию. В чём тут может быть проблема? Сделаю намёк: а вы помните мою ремарку про динозавров? :)
Этот паттерн был рождён в очень далёкие времена, когда synchronized методы были очень медленными. На сегодня и производительность JVM, и железа как такового шагнула далеко вперёд, из-за чего использовать эту (микро)оптимизацию становится накладно банально из-за риска вот таких ошибок. В простом случае синглтонов для потокобезопасности достаточно инициализировать их как поле класса:
class Holder {
static SomeSingleton singleton = new SomeSingleton();
}А в остальных случаях достаточно просто использовать синхронизированные методы, если только всё остальное ещё не оптимизировано.
Кстати, раз я уже упоминал C# до этого, сделаю это ещё раз. Забавно, но там тоже существует точно такая же проблема, о чём мы уже писали.
Использовать синхронизированные методы? А что, если разработчики уже это делают? Тогда поле volatile было бы просто избыточным. Оно используется в 20 местах, давайте посмотрим на случайные три:
void addNewRun(@NotNull DBTTaskRun taskRun) {
synchronized (this) {
loadRunsIfNeeded();
runs.add(taskRun);
while (runs.size() > MAX_RUNS_IN_STATS) {
runs.remove(0);
}
flushRunStatistics(runs);
}
TaskRegistry.getInstance().notifyTaskListeners(....);
}Так, мы сразу попали на четыре использования, но тут есть synchronized блок...
private void loadRunsIfNeeded() {
if (runs == null) {
synchronized (this) {
if (runs == null) {
runs = new ArrayList<>(loadRunStatistics());
}
}
}
}Ещё три по цене одного, и снова synchronized. Неужели я всё это время просто вам морочил голову?
@Override
public void cleanRunStatistics() {
Path statsFolder = getTaskStatsFolder(false);
if (Files.exists(statsFolder)) {
....
}
if (runs != null) {
runs.clear();
}
flushRunStatistics(List.of());
TaskRegistry.getInstance().notifyTaskListeners(....);
}Есть! Если проследовать по использованиям этого метода, то выше нигде синхронизации нет. Получается, мы нашли то, что искали. К слову, место это не единственное.
К чему было нужно это представление, если я сразу мог показать место, где нет никакой синхронизации? Просто это ещё одна самостоятельная потенциальная уязвимость. Её присутствие может спровоцировать состояние гонки само по себе, и это помимо того, что она делает последствия ошибки с double-check locking возможными.
В общем, чтобы исправить это место, потребуется приложить несколько больше усилий, ведь пока изучалась одна ошибка, нашлась другая.
Надеюсь, вам было интересно погрузиться со мной в удивительный мир ловушек параллельного программирования. Многие утверждают, что если вам приходилось использовать volatile переменные или паттерн double-checked locking, то вы уже делали что-то не так. Либо вы эксперт и точно знали, что делали :)
Тем не менее думаю, эта ремарка неплохо резюмирует содержание всей статьи: кроличья нора и правда глубока. Во всяком случае достаточно глубока для того, чтобы быть вдвойне внимательным, когда имеешь дело с предметом статьи.
Если хочется поискать эти или другие ошибки в своём проекте, то вы можете бесплатно попробовать PVS-Studio, перейдя по этой ссылке.
А ещё я напомню, что это не все интересные вещи, которые были найдены в DBeaver. Ссылки на другие статьи можно найти в начале, а чтобы не пропустить выход новых статей, предлагаю подписаться на мой Х-твиттер.
0
0
0
27
0
37
0
110
0
Попробуйте PVS-Studio бесплатно
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0