Почему эта Java-программа завершает работу, несмотря на то, что, очевидно, она не должна (и не должна)?

205

Чувствительная операция в моей лаборатории сегодня прошла совсем не так. Привод на электронном микроскопе перешел ее границы, и после череды событий я потерял оборудование на 12 миллионов долларов. Я сузил более 40K строк в неисправном модуле так:

import java.util.*;

class A {
    static Point currentPos = new Point(1,2);
    static class Point {
        int x;
        int y;
        Point(int x, int y) {
            this.x = x;
            this.y = y;
        }
    }
    public static void main(String[] args) {
        new Thread() {
            void f(Point p) {
                synchronized(this) {}
                if (p.x+1 != p.y) {
                    System.out.println(p.x+" "+p.y);
                    System.exit(1);
                }
            }
            @Override
            public void run() {
                while (currentPos == null);
                while (true)
                    f(currentPos);
            }
        }.start();
        while (true)
            currentPos = new Point(currentPos.x+1, currentPos.y+1);
    }
}

Некоторые примеры вывода я получаю:

$ java A
145281 145282
$ java A
141373 141374
$ java A
49251 49252
$ java A
47007 47008
$ java A
47427 47428
$ java A
154800 154801
$ java A
34822 34823
$ java A
127271 127272
$ java A
63650 63651

Поскольку здесь нет никакой арифметики с плавающей запятой, и мы все знаем, что целые числа со знаком хорошо работают при переполнении в Java, я думаю, что в этом коде нет ничего плохого. Однако, несмотря на вывод, указывающий, что программа не достигла условия выхода, она достигла условия выхода (она была достигнута и не достигнута?). Зачем?


Я заметил, что это не происходит в некоторых средах. Я на OpenJDK 6 на 64-битной Linux.

Собака
источник
41
12 миллионов оборудования? Мне действительно любопытно, как это могло случиться ... почему вы используете пустой блок синхронизации: synchronized (this) {}?
Мартин В.
84
Это даже не безопасно для потоков.
Мэтт Болл
8
Интересно отметить: добавление finalквалификатора (который не влияет на производимый байт-код) к полям xи y«решает» ошибку. Хотя это не влияет на байт-код, поля помечаются им, что заставляет меня думать, что это побочный эффект оптимизации JVM.
Нив Штейнгартен
9
@ Евгений: это не должно заканчиваться. Вопрос «почему это заканчивается?». Создается A, Point pкоторое удовлетворяет p.x+1 == p.y, затем ссылка передается потоку опроса. В конце концов, поток опроса решает выйти, потому что считает, что условие не выполняется для одного из Points, которые он получает, но затем вывод консоли показывает, что он должен быть выполнен. Отсутствие volatileздесь просто означает, что поток опроса может застрять, но это явно не проблема.
Эрма К. Писарро
21
@JohnNicholas: Реальный код (который, очевидно, не тот) имел 100% тестовое покрытие и тысячи тестов, многие из которых тестировали вещи в тысячах различных порядков и перестановок ... Тестирование не может волшебным образом найти каждый крайний случай, вызванный недетерминированным JIT / кэш / планировщик. Реальная проблема заключается в том, что разработчик, который написал этот код, не знал, что конструирование не происходит до использования объекта. Обратите внимание, как удаление пустого synchronizedделает ошибку не происходит? Это потому, что мне приходилось писать код случайным образом, пока я не нашел код, который воспроизводил бы это поведение детерминистически.
собака

Ответы:

140

Очевидно, что запись в currentPos не происходит - до ее прочтения, но я не понимаю, как это может быть проблемой.

currentPos = new Point(currentPos.x+1, currentPos.y+1);делает несколько вещей, в том числе записывает значения по умолчанию в xand y(0) и затем записывает их начальные значения в конструктор. Поскольку ваш объект не публикуется безопасно, эти 4 операции записи могут быть свободно переупорядочены компилятором / JVM.

Таким образом, с точки зрения потока чтения, это законное выполнение для чтения xс новым значением, но yсо значением по умолчанию, например, 0. К тому времени, когда вы достигнете printlnоператора (который, кстати, синхронизирован и, следовательно, влияет на операции чтения), переменные имеют свои начальные значения, и программа выводит ожидаемые значения.

Пометка currentPosas volatileобеспечит безопасную публикацию, поскольку ваш объект является практически неизменным - если в вашем реальном случае объект мутирует после создания, volatileгарантий не будет достаточно, и вы снова увидите несогласованный объект.

В качестве альтернативы вы можете сделать Pointнеизменяемым, что также обеспечит безопасную публикацию даже без использования volatile. Чтобы добиться неизменности, вам просто нужно пометить xи yокончательно.

В качестве дополнительного примечания и, как уже упоминалось, synchronized(this) {}JVM может рассматривать его как запрет на выполнение операций (насколько я понимаю, вы включили его для воспроизведения поведения).

assylias
источник
4
Я не уверен, но разве x и y final не будут иметь одинаковый эффект, избегая барьера памяти?
Майкл Беклинг
3
Более простая конструкция - это неизменяемый точечный объект, который тестирует инварианты при построении. Таким образом, вы никогда не рискуете опубликовать опасную конфигурацию.
Рон
@BuddyCasino Да, действительно - я добавил это. Если честно, я не помню всю дискуссию 3 месяца назад (использование final было предложено в комментариях, поэтому не уверен, почему я не включил его в качестве опции).
assylias
2
Сама неизменность не гарантирует безопасную публикацию (если бы x a y были частными, но подвергались только получателям, такая же проблема публикации все равно существовала бы). окончательный или изменчивый гарантирует это. Я бы предпочел окончательный, а не волатильный.
Стив Куо
@SteveKuo Неизменяемость требует финала - без финала лучшее, что вы можете получить, - это эффективная неизменность, которая не имеет той же семантики.
assylias
29

Поскольку currentPosизменяется вне потока, он должен быть помечен как volatile:

static volatile Point currentPos = new Point(1,2);

Без volatile поток не может считывать обновления в currentPos, которые производятся в основном потоке. Таким образом, новые значения продолжают записываться для currentPos, но поток продолжает использовать предыдущие кэшированные версии из соображений производительности. Поскольку только один поток модифицирует currentPos, вы можете обойтись без блокировок, которые улучшат производительность.

Результаты будут сильно отличаться, если вы прочитаете значения только один раз в потоке для использования в сравнении и последующем их отображении. Когда я делаю следующее xвсегда отображается как 1и yменяется между 0и некоторым большим целым числом. Я думаю, что его поведение в данный момент несколько неопределенно без volatileключевого слова, и вполне возможно, что JIT-компиляция кода способствует тому, чтобы он действовал так. Также, если я закомментирую пустой synchronized(this) {}блок, тогда код также работает, и я подозреваю, что это происходит потому, что блокировка вызывает достаточную задержку, currentPosи ее поля перечитываются, а не используются из кэша.

int x = p.x + 1;
int y = p.y;

if (x != y) {
    System.out.println(x+" "+y);
    System.exit(1);
}
Эд Плезе
источник
2
Да, и я мог бы просто поставить замок вокруг всего. В чем ваша точка зрения?
собака
Я добавил некоторые дополнительные пояснения по использованию volatile.
Эд Плезе
19

У вас есть обычная память, ссылка «currentpos» и объект Point и его поля позади него, совместно используемые двумя потоками без синхронизации. Таким образом, не существует определенного порядка между записями, которые происходят с этой памятью в главном потоке, и чтениями в созданном потоке (назовите его T).

Основной поток выполняет следующие записи (игнорирование начальной установки точки приведет к тому, что px и py будут иметь значения по умолчанию):

  • в px
  • пи
  • в настоящее время

Поскольку в этих записях нет ничего особенного с точки зрения синхронизации / барьеров, среда выполнения свободна, чтобы позволить потоку T видеть, что они происходят в любом порядке (основной поток, конечно, всегда видит записи и чтения, упорядоченные в соответствии с порядком программы), и происходит в любой момент между чтениями в Т.

Итак, T делает:

  1. читает currentpos в p
  2. читать px и py (в любом порядке)
  3. сравните и возьмите ветку
  4. читать px и py (в любом порядке) и вызывать System.out.println

Учитывая отсутствие порядка упорядочения между записями в main и чтениями в T, есть несколько способов, которыми это может привести к вашему результату, так как T может увидеть запись main в currentpos перед записью в currentpos.y или currentpos.x:

  1. Сначала он читает currentpos.x до того, как произошла запись x - получает 0, затем читает currentpos.y до того, как запись y произошла - получает 0. Сравнение evals до true. Записи становятся видимыми для T. System.out.println называется.
  2. Сначала он читает currentpos.x, после того как произошла запись x, затем читает currentpos.y до того, как произошла запись y - получает 0. Сравнение evals до true. Пишет становится видимым для T ... и т. Д.
  3. Сначала он читает currentpos.y, до того, как произошла запись y (0), затем читает currentpos.x после записи x и принимает значение true. и т.п.

и так далее ... Здесь есть несколько гонок данных.

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

currentPos = new Point(currentPos.x+1, currentPos.y+1);

Java не дает такой гарантии (это было бы ужасно для производительности). Необходимо добавить еще кое-что, если вашей программе требуется гарантированный порядок записей относительно операций чтения в других потоках. Другие предложили сделать поля x, y окончательными или, в качестве альтернативы, сделать текущие переменными.

  • Если вы сделаете поля x, y окончательными, то Java гарантирует, что записи их значений будут видны до того, как конструктор вернется во всех потоках. Таким образом, поскольку присваивание currentpos выполняется после конструктора, поток T гарантированно увидит записи в правильном порядке.
  • Если вы сделаете currentpos изменчивым, то Java гарантирует, что это точка синхронизации, которая будет упорядочена по всем точкам синхронизации. Так как в основном записи в x и y должны происходить до записи в currentpos, то любое чтение currentpos в другом потоке должно также видеть записи x, y, которые произошли ранее.

Преимущество использования final в том, что оно делает поля неизменяемыми и, таким образом, позволяет кэшировать значения. Использование volatile приводит к синхронизации при каждой записи и чтении currentpos, что может снизить производительность.

См. Главу 17 Спецификации языка Java для подробностей: http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html.

(Первоначальный ответ предполагал более слабую модель памяти, так как я не был уверен, что JLS гарантировала, что энергозависимость была достаточной. Ответ отредактирован, чтобы отразить комментарий от ассилий, указав, что модель Java более сильная - происходит до того, как транзитивна - и такой изменчивости на currentpos также достаточно ).

paulj
источник
2
Это лучшее объяснение на мой взгляд. Большое спасибо!
Skyde
1
@skyde, но неправильно в семантике volatile. volatile гарантирует, что при чтении volatile-переменной будут видны последние доступные записи volatile-переменной, а также любая предшествующая запись . В этом случае, если currentPosзадание является изменчивым, назначение обеспечивает безопасную публикацию currentPosобъекта, а также его элементов, даже если они сами не являются изменчивыми.
assylias
Ну, я говорил, что не могу для себя увидеть, как именно JLS гарантирует, что volatile формирует барьер с другими, обычными операциями чтения и записи. Технически я не могу ошибаться;). Когда речь идет о моделях памяти, разумно предположить, что порядок не гарантирован, и ошибаться (вы все еще в безопасности), чем наоборот, и ошибаться и быть небезопасным. Здорово, если volatile дает такую ​​гарантию. Можете ли вы объяснить, как это предусмотрено в 17-й главе JLS?
Пол
2
Короче говоря, у Point currentPos = new Point(x, y)вас есть 3 записи: (w1) this.x = x, (w2) this.y = yи (w3) currentPos = the new point. Порядок программы гарантирует, что hb (w1, w3) и hb (w2, w3). Позже в программе вы прочитали (r1) currentPos. Если currentPosне является изменчивым, то между r1 и w1, w2, w3 нет hb, поэтому r1 может наблюдать любой (или ни один) из них. С volatile вы вводите hb (w3, r1). И отношение hb является транзитивным, поэтому вы также вводите hb (w1, r1) и hb (w2, r1). Это кратко изложено в разделе «Параллелизм Java на практике» (3.5.3. Идиомы безопасной публикации).
assylias
2
Ах, если hb транзитивен таким образом, то это достаточно сильный «барьер», да. Я должен сказать, что нелегко определить, что 17.4.5 JLS определяет hb, чтобы иметь это свойство. Это, конечно, не в списке свойств, приведенных в начале 17.4.5. Транзитивное закрытие упоминается только после некоторых пояснительных записок! В любом случае, приятно знать, спасибо за ответ! :). Примечание: я обновлю свой ответ, чтобы отразить комментарий ассилий.
Пол
-2

Вы можете использовать объект для синхронизации записи и чтения. В противном случае, как уже говорили другие, запись в currentPos произойдет в середине двух операций чтения p.x + 1 и py

new Thread() {
    void f(Point p) {
        if (p.x+1 != p.y) {
            System.out.println(p.x+" "+p.y);
            System.exit(1);
        }
    }
    @Override
    public void run() {
        while (currentPos == null);
        while (true)
            f(currentPos);
    }
}.start();
Object sem = new Object();
while (true) {
    synchronized(sem) {
        currentPos = new Point(currentPos.x+1, currentPos.y+1);
    }
}
Джермано Фронза
источник
На самом деле это делает работу. В моей первой попытке я поместил чтение в синхронизированный блок, но позже я понял, что в этом нет необходимости.
Джермано Фронза
1
-1 JVM может доказать, что semоно не является общим, и обработать синхронизированный оператор как недопустимый ... Тот факт, что он решает проблему, является чистой удачей.
assylias
4
Я ненавижу многопоточное программирование, слишком много вещей работают из-за удачи.
Джонатан Аллен
-3

Вы получаете доступ к currentPos дважды и не гарантируете, что он не обновляется между этими двумя доступами.

Например:

  1. х = 10, у = 11
  2. рабочий поток оценивает px как 10
  3. основной поток выполняет обновление, теперь x = 11 и y = 12
  4. рабочий поток оценивает py как 12
  5. рабочий поток замечает, что 10 + 1! = 12, поэтому печатает и завершает работу.

По сути, вы сравниваете две разные точки.

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

Добавить

boolean IsValid() { return x+1 == y; }

метод к вашему классу очков. Это обеспечит использование только одного значения currentPos при проверке x + 1 == y.

user2686913
источник
currentPos читается только один раз, его значение копируется в p. p читается дважды, но он всегда будет указывать на одно и то же место.
Джонатан Аллен