А что если окажется, что простые знания на самом деле более нюансированные, а старые знакомые, такие как Double-checked locking, являются неоднозначными? Именно на такие мысли наталкивает изучение кода реальных проектов. Результаты этого исследования мы и рассмотрим в этой статье.

Предисловие

Не так давно я — в порядке обычной деятельности по проверке открытых проектов с помощью PVS-Studio — проверял только вышедшую тогда 24-ю версию известного проекта DBeaver. Надо признать, меня приятно удивило качество его кода — недаром они используют инструменты статического анализа. Но я продолжил исследование и нашёл несколько подозрительных мест, которые меня зацепили. Зацепили настолько, что каждому я решил посвятить по статье, так что приветствую в первой части цикла.

Потоконебезопасное программирование

О чём речь?

Параллельное программирование таит в себе кучу граблей. И, разумеется, не только в Java — тот же злой (или добрый :) ) брат-близнец C# позволяет легко оступиться при использовании событий в параллельных сценариях. Известно, что зачастую именно асинхронные и многопоточные ошибки сложнее всего отлавливать.

Далее мы посмотрим на три типа таких ошибок. Первой из них является неатомарное изменение volatile переменных.

Ещё раз, что такое volatile?

Из документации мы можем узнать, что ключевое слово volatile:

1. Предотвращает использование кэша для разных потоков, тем самым гарантируя, что изменения в переменной будут сразу видны всем потокам;

2. Устанавливает happens-before отношение между переменной и чтениями/записью рядом, тем самым запрещая производить оптимизации, основанные на изменении порядка операций.

Продемонстрировать работу этого ключевого слова может прозаичный пример с потоком, ожидающим изменения переменной:

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 и железа.

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

Гонка

Несмотря на то, что у нас обеспечена видимость переменной, этого, конечно, не хватит для корректной работы в параллельных потоках:

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. Исправлений может быть два, одно проще другого:

private static final AtomicInteger counter = new AtomicInteger(0);
private static void increment() {
    Thread.sleep(1);
    counter.incrementAndGet();
}

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--;               // <=
            ....
        }
    }
}

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

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

this.initializedCount = 0;

for (DBNDatabaseNode node : nodes) {
    node.initializeNode(null, initializeFinisher);
    ....
}
while (initializedCount != nodes.size()) {
    Thread.sleep(50);
    if (monitor.isCanceled()) {
        throw new InterruptedException();
    }
    ....
}

public boolean initializeNode(
    DBRProgressMonitor monitor,
    DBRProgressListener onFinish
) throws DBException {
    if (onFinish != null) {
        onFinish.onTaskFinished(Status.OK_STATUS);
    }
    return true;
}

Внезапный поворот

До этого мы смотрели на простые примеры с примитивными типами. А достаточно ли сделать поле ссылочного типа 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.

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, но я бы сделал шаг назад и посмотрел на код. А в чём, собственно, проблема? Порядок действий тут такой:

1. Проверка resource на null. Эту точку может пройти одновременно несколько потоков;

2. Вход в блок синхронизации. Попасть туда может только один поток:

   • первый поток, сделавший это, во второй проверке удостоверится, что resource ещё не создан, после чего создаст его сам;

3. Другие потоки, проскочившие первую проверку, поочерёдно попадают в блок синхронизации:

   • там они видят, что resource уже создан, и выходят оттуда;

4. Все довольны и счастливы.

Казалось бы, зачем нам volatile, если мы используем синхронизацию для контроля доступа к полю? И правда, видимость значения для потоков тут не является камнем преткновения, но у нас есть две проблемы:

• операция создания объектов не является атомарной (с примитивными типами, за исключением 64-х битных, проблем не будет);

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

То есть последствия таких оптимизаций могут оказаться непредсказуемыми. Например, компилятор может заинлайнить конструктор и как угодно поменять порядок инициализации полей. Как результат, объект может быть отмечен как созданный, другой поток это увидит и начнёт его использовать, и найдёт там ещё неинициализированные поля. Именно от этой ситуации спасает вышеописанный принцип 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.

То есть перед нами паттерн, преследующий своей целью оптимизацию. В чём тут может быть проблема? Сделаю намёк: а вы помните мою ремарку про динозавров? :)

Этот паттерн был рождён в очень далёкие времена, когда 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(....);
}

private void loadRunsIfNeeded() {
    if (runs == null) {
        synchronized (this) {
            if (runs == null) {
                runs = new ArrayList<>(loadRunStatistics());
            }
        }
    }
}

@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, и к выходу планируются дополнительные статьи. После их публикации я добавлю ссылки сюда, а пока предлагаю подписаться на мой Х-твиттер, чтобы не пропустить их выход.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Konstantin Volohovsky. Volatile, DCL, and synchronization pitfalls in Java.