Рекурсивното извикване 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
@Ben, съжалявам за offtop, но наистина съм изненадан колко нисък 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 Грозно, но възможно заобиколно решение в кофеина е да се използва AsyncLoadingCache, така че изчисленията да се извършват извън операцията на картата (върху бъдещето), по избор с изгледа synchronous(). - person Ben Manes; 04.10.2018
comment
@Ben благодаря за предложението. Прибягнах до използването на Cache вместо LoadingCache, където моят фасаден клас обвива Cache и CacheLoader, неговият метод get() делегира на Cache#getIfPresent() / put(), извиквайки CacheLoader, когато е необходимо. свиване на рамене - person Scott; 04.10.2018

Това разбира се е "функция". ConcurrentHashMap.computeIfAbsent() Javadoc гласи:

Ако указаният ключ все още не е свързан със стойност, се опитва да изчисли стойността му с помощта на дадената функция за преобразуване и го въвежда в тази карта, освен ако не е нула. Цялото извикване на метода се извършва атомарно, така че функцията се прилага най-много веднъж на ключ. Някои опити за актуализиране на тази карта от други нишки може да бъдат блокирани, докато изчислението е в ход, така че изчислението трябва да бъде кратко и просто и не трябва да се опитва да актуализира други съпоставяния на тази карта .

Формулировката „не трябва“ е ясен договор, който моят алгоритъм наруши, макар и не поради същите причини за едновременност.

Все още е интересно, че няма ConcurrentModificationException. Вместо това програмата просто никога не спира - което все още е доста опасен бъг според мен (т.е. безкрайни цикли. или: всичко, което може да се обърка, го прави).

Забележка:

Функцията 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 безопасен ли е за нишки? така че гарантирано ли е да не се повреди състоянието на картата, когато се извиква едновременно от множество нишки? с повреден имам предвид, че имате загубена актуализация или изключение при поставяне. Добре е, ако две нишки изчисляват една и съща стойност и само една операция put от тези нишки наистина актуализира картата (например последната печели) - 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: Искаш ли да добавя малко interrobang след последното изречение с удебелен шрифт? :) - 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