Рекурсивный вызов ConcurrentHashMap.computeIfAbsent() никогда не завершается. Баг или фича?

Некоторое время назад я написал в блоге о Функциональный способ Java 8 для рекурсивного вычисления чисел Фибоначчи с ConcurrentHashMap кешем и новым полезным методом computeIfAbsent():

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

public class Test {
    static Map<Integer, Integer> cache = new ConcurrentHashMap<>();

    public static void main(String[] args) {
        System.out.println(
            "f(" + 8 + ") = " + fibonacci(8));
    }

    static int fibonacci(int i) {
        if (i == 0)
            return i;

        if (i == 1)
            return 1;

        return cache.computeIfAbsent(i, (key) -> {
            System.out.println(
                "Slow calculation of " + key);

            return fibonacci(i - 2) + fibonacci(i - 1);
        });
    }
}

Я выбрал ConcurrentHashMap, потому что думал сделать этот пример еще более сложным, введя параллелизм (чего в итоге не сделал).

Теперь давайте увеличим число с 8 до 25 и посмотрим, что произойдет:

        System.out.println(
            "f(" + 25 + ") = " + fibonacci(25));

Программа никогда не останавливается. Внутри метода есть цикл, который работает вечно:

for (Node<K,V>[] tab = table;;) {
    // ...
}

Я использую:

C:\Users\Lukas>java -version
java version "1.8.0_40-ea"
Java(TM) SE Runtime Environment (build 1.8.0_40-ea-b23)
Java HotSpot(TM) 64-Bit Server VM (build 25.40-b25, mixed mode)

Маттиас, читатель этот пост в блоге также подтвердил проблему (он действительно нашел ее).

Это странно. Я бы ожидал любого из следующих двух:

  • Оно работает
  • Выдает ConcurrentModificationException

Но просто никогда не останавливаться? Это кажется опасным. Это ошибка? Или я неправильно понял какой-то контракт?


person Lukas Eder    schedule 03.03.2015    source источник


Ответы (3)


Это исправлено в JDK-8062841.

В предложении 2011 года я обнаружил эту проблему во время проверки кода. Был обновлен JavaDoc и добавлено временное исправление. Он был удален при дальнейшей переработке из-за проблем с производительностью.

В обсуждении 2014 года мы рассмотрели способы более эффективного обнаружения и отказа. Обратите внимание, что часть обсуждения была переведена в офлайн-режим и отправлена ​​в частную электронную почту для рассмотрения низкоуровневых изменений. Хотя не каждый случай может быть охвачен, общие случаи не будут заблокированы. Эти исправления находятся в репозитории Дуга, но не включены в выпуск JDK.

person Ben Manes    schedule 04.03.2015
comment
Очень интересно, спасибо, что поделились! Мой отчет также был принят как JDK-8074374. - person Lukas Eder; 04.03.2015
comment
@ Бен, извините за оффтоп, но я действительно удивлен тем, насколько низкий у вас рейтинг SO, несмотря на столь значительный вклад в базу знаний, касающуюся такой сложной вещи, как параллелизм без блокировки (и почти без блокировки). - person Alex Salauyou; 03.03.2016
comment
@SashaSalauyou Спасибо. Я часто даю быстрый ответ в комментариях вместо того, чтобы тратить время на более длинный и полный ответ. Однако баллы за комментарии не повышают репутацию. Хотя, вероятно, у меня меньше среднего интереса к погоне за репутацией. - person Ben Manes; 04.03.2016
comment
@Ben не совсем исправлено: бум - person Scott; 04.10.2018
comment
@ScottMcKinney, хороший улов! Похоже, что то же утверждение можно применить к transfer, и это был пропущенный случай. Можешь написать [email protected], чтобы привлечь внимание Дуга? - person Ben Manes; 04.10.2018
comment
@Бен, конечно, без проблем. Я обнаружил это после интеграции кофеина в мой кеш-сервер, где загрузчик кеша может быть рекурсивным. Моя первая попытка исправить это состояла в том, чтобы установить начальную емкость кеша на размер кеша. Это устраняет большинство проблем, включая тест, на который я ссылался выше, однако я боюсь, что помимо этого есть еще больше пропущенных случаев. Я постараюсь изолировать их и предоставить узкие тесты. - person Scott; 04.10.2018
comment
@ScottMcKinney Некрасивый, но возможный обходной путь в Caffeine — использовать AsyncLoadingCache, чтобы вычисления выполнялись вне операции карты (на будущее), необязательно с представлением synchronous(). - person Ben Manes; 04.10.2018
comment
@Бен, спасибо за предложение. Я прибегал к использованию Cache вместо LoadingCache, где мой фасадный класс обертывает Cache и CacheLoader, его метод get() делегирует Cache#getIfPresent() / put(), вызывая CacheLoader при необходимости. пожал плечами - person Scott; 04.10.2018

Это, конечно, "функция". ConcurrentHashMap.computeIfAbsent() Javadoc гласит:

Если указанный ключ еще не связан со значением, пытается вычислить его значение с помощью заданной функции отображения и вводит его в эту карту, если только оно не равно нулю. Весь вызов метода выполняется атомарно, поэтому функция применяется не более одного раза для каждой клавиши. Некоторые попытки обновления этой карты другими потоками могут быть заблокированы во время выполнения вычислений, поэтому вычисления должны быть короткими и простыми и не должны пытаться обновлять какие-либо другие сопоставления этой карты .

Формулировка "не должен" является четким соглашением, которое мой алгоритм нарушил, хотя и не по тем же причинам параллелизма.

Что еще интересно, так это то, что нет ConcurrentModificationException. Вместо этого программа просто никогда не останавливается, что, на мой взгляд, все еще является довольно опасной ошибкой (т.е. -can-possible-go-wrong-does/" rel="noreferrer">бесконечные циклы. или: все, что может пойти не так, делает).

Примечание:

HashMap.computeIfAbsent() или Map.computeIfAbsent() Javadoc не запрещает такие рекурсивные вычисления, что, конечно, смешно, поскольку тип кеша Map<Integer, Integer>, а не ConcurrentHashMap<Integer, Integer>. Для подтипов очень опасно радикально переопределять контракты супертипа (Set против SortedSet — это приветствие). Таким образом, выполнение такой рекурсии также должно быть запрещено в супертипах.

person Lukas Eder    schedule 03.03.2015
comment
Хорошая находка. Я бы предложил отчет об ошибке/RFE против JDK. - person Axel; 03.03.2015
comment
Готово, давайте посмотрим, будет ли это принято... Я обновлю ссылку, если это так. - person Lukas Eder; 03.03.2015
comment
Мне кажется вероятным, что этот тип рекурсивной модификации других сопоставлений не разрешен в ConcurrentHashMap из-за другой важной части его контракта: Вызов всего метода выполняется атомарно, поэтому функция применяется не более одного раза для каждого ключа. . Вполне вероятно, что ваша программа, нарушая соглашение о запрете рекурсивных модификаций, пытается получить блокировку, которую она уже имеет, и взаимоблокируется сама с собой, а не работает в бесконечном цикле. - person murgatroid99; 04.03.2015
comment
Из JavaDoc: IllegalStateException - if the computation detectably attempts a recursive update to this map that would otherwise never complete - person Ben Manes; 04.03.2015
comment
@ murgatroid99: Это довольно неявно. Вызов метода по-прежнему может выполняться атомарно и обновлять несколько ключей. Хотя это, вероятно, не было бы хорошей идеей в параллельном контексте из-за потенциальной взаимоблокировки. - person Lukas Eder; 04.03.2015
comment
@Axel: отчет был принят как JDK-8074374. Вероятно, это дубликат JDK-8062841, как задокументировано Бен Мейнс - person Lukas Eder; 04.03.2015
comment
@Lukas: мне кажется, что если вызов должен оставаться атомарным и по-прежнему иметь возможность обновлять несколько ключей, то, вероятно, все сегменты карты должны быть заблокированы - person user62058; 05.03.2015
comment
@ user62058: На мой взгляд, это должно быть хорошо. Конечно, это может привести к взаимоблокировке, что так же плохо, как и незавершающий цикл. - person Lukas Eder; 05.03.2015
comment
@Lukas, вы предлагаете использовать HashMap, является ли метод put в HashMap потокобезопасным? так гарантированно ли он не испортит состояние карты при одновременном вызове несколькими потоками? с коррумпированным я имею в виду, что у вас есть потерянное обновление или исключение при установке. Это нормально, если два потока вычисляют одно и то же значение, и только одна операция размещения этих потоков действительно обновляет карту (например, выигрывает последний) - person leozilla; 06.10.2015
comment
@leozilla: см. исходный вопрос. В этом случае алгоритм является однопоточным. - person Lukas Eder; 06.10.2015
comment
@LukasEder даже с HashMap это не нормально. bugs.openjdk.java.net/browse/JDK-8172951 - person Piotr Findeisen; 22.05.2017
comment
@LukasEder, это хорошо, но я думаю, что ваше последнее замечание все еще можно понять как ... но для HashMap это работает. Давайте перефразируем: хотя Map javadocs явно не запрещает это, функция не должна каким-либо образом изменять карту. - person Piotr Findeisen; 26.05.2017
comment
@PiotrFindeisen: Вы хотите, чтобы я добавил немного интерробанга после последнего предложения, выделенного жирным шрифтом? :) - person Lukas Eder; 26.05.2017

Это очень похоже на баг. Потому что, если вы создадите свой кеш емкостью 32, ваша программа будет работать до 49. И интересно, что параметр sizeCtl =32 + (32 >>> 1) + 1) =49! Может быть причина в изменении размера?

person Павел Бивойно    schedule 11.03.2015
comment
У меня нет времени копаться в исходниках, но я думаю, что вы правы. Мы постоянно сталкиваемся с этим в производстве. Как только мы увеличили начальную емкость CHM до очень высокого значения, это исчезло. Пока мы не рефакторим наш код, мы будем делать это... - person Eugene; 09.02.2019