ArrayIndexOutOfBoundsException w cache

ArrayIndexOutOfBoundsException w cache
LitwinWileński
  • Rejestracja:prawie 3 lata
  • Ostatnio:6 dni
  • Postów:740
0

Co kilkadziesiąt sekund 1 wątek woła cachedSimpleService.update() i równocześnie cały czas miliardy klientów uderza przez http do controlera, który woła cachedSimpleService.getXmlData(${page}). Co kilka dni aplikacja się wykrzacza, wchodzi w zły stan i trzeba ją zrestartować. Wylatuje ArrayIndexOutOfBoundsException na każdy cachedSimpleService.getXmlData(${page}).

Weak, "eventual consistancy" jest wystarczające, klienty nie muszą widzieć tych samych danych od razu ani aktualnych, ważne by w końcu za którymś zapytaniem się odświeżyły dlatego nic tu nie gmerałem z blokami synchronized itp.

Widzicie jakiś błąd w tym kodzie?

Kopiuj
class CachedSimpleService implements MyService {

    private final SimpleService myService;
    private Map<Integer, String> xmlDataPaged;

    public CachedSimpleService(MyService myService) {
        this.myService = myService;
        this.dataPaged = new ConcurrentHashMap<>();
    }

    @Override
    public String getXmlData(int page) {
        String xml = xmlDataPaged.get(page);
        if (xml == null) {
            xml = myService.getXmlData(page);
            xmlDataPaged.put(page, xml);
        }
        return xml;
    }

    @Override
    public void update() {
        this.myService.update();
        invalidateCache();
    }

    private void invalidateCache() {
        this.xmlDataPaged = new ConcurrentHashMap<>();
    }
}
Kopiuj

class SimpleService implements MyService {

    private final DataHolder dataHolder;
    private final PageableMapper pageableMapper;
    private final XmlParser xmlParser;

    @Override
    public String getXmlData(int page) {
        var data = dataHolder.getData();
        var singlePageFromData = pageableMapper.from(data, page);
        return xmlParser.parse(singlePageFromData);              // ===> tu leci java.lang.ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 47
    }

    @Override
    public void update() {
        this.dataHolder.update();
    }
}
Kopiuj
class DataHolder {

    private final DataRepo dataRepo;

    private Data data;

    public Data getData() {
        if (data == null) {
            throw new DataNotInitializedException("Data not prepared yet. Please try again later");
        }
        return data;
    }

    public void update() {
        this.data = getData();
    }

    private Data getData() {
      // ...
    }
}


zrobiłbym tak, ale nie wiem czy to jest przyczyną tych błędów:

Kopiuj

class CachedSimpleService implements MyService {

    private final SimpleService myService;
    private volatile Map<Integer, String> xmlDataPaged;

    public CachedSimpleService(MyService myService) {
        this.myService = myService;
        this.dataPaged = new ConcurrentHashMap<>();
    }

    @Override
    public String getXmlData(int page) {
        String xml = xmlDataPaged.get(page);
        if (xml == null) {z
            synchronized(xmlDataPaged) {                // ===> changed
                if(xml == null {                        // ===> changed
                    xml = myService.getXmlData(page);
                    xmlDataPaged.put(page, xml);
                }
            }
        }
        return xml;
    }

    @Override
    public void update() {
        this.myService.update();
        invalidateCache();
    }

    private void invalidateCache() {
        this.xmlDataPaged = new ConcurrentHashMap<>();
    }
}

Kopiuj
class DataHolder {

    private final DataRepo dataRepo;

    private volatile Data data;

    public synchronized Data getData() { // ===> changed
        if (data == null) {
            throw new DataNotInitializedException("Data not prepared yet. Please try again later");
        }
        return data;
    }

    public synchronized void update() { // ===> changed
        this.data = getData();
    }

    private Data getData() {
      // ...
    }
}

edytowany 9x, ostatnio: LitwinWileński
YA
  • Rejestracja:prawie 10 lat
  • Ostatnio:8 dni
  • Postów:2370
1
  1. return xmlParser.parse(singlePageFromData); jakie będzie zachowanie tego parsera, gdy raz na milion wywołań dostanie niepoprawny dokument XML?
    ) Przy invalidateCache mam wątpliwości, czy zmiana będzie atomowa, czy atomowa że wybuchnie na grubo ;-)
Kopiuj
  private void invalidateCache() {
        this.xmlDataPaged = new ConcurrentHashMap<>();
    }

Dlaczego by tego ConcurrentHashMap nie opakować w AtomicReference ? Wówczas podmiana byłaby atomowa, a tak to kto to może wiedzieć.

crejk
  • Rejestracja:ponad 6 lat
  • Ostatnio:około 5 godzin
  • Postów:46
0
Kopiuj
    @Override
    public String getXmlData(int page) {
        String xml = xmlDataPaged.get(page);
        if (xml == null) {z
            synchronized(xmlDataPaged) {
                if(xml == null {
                    xml = myService.getXmlData(page);
                    xmlDataPaged.put(page, xml);
                }
            }
        }
        return xml;
    }

Użyj po prostu computeIfAbsent

LitwinWileński
  • Rejestracja:prawie 3 lata
  • Ostatnio:6 dni
  • Postów:740
0
crejk napisał(a):
Kopiuj
    @Override
    public String getXmlData(int page) {
        String xml = xmlDataPaged.get(page);
        if (xml == null) {z
            synchronized(xmlDataPaged) {
                if(xml == null {
                    xml = myService.getXmlData(page);
                    xmlDataPaged.put(page, xml);
                }
            }
        }
        return xml;
    }

Użyj po prostu computeIfAbsent

haha, też się na to naciąłem. Przy computeIfAbsent mapa prędzej czy później będzie strzelać ConcurrentModificationException.

https://stackoverflow.com/questions/54824656/since-java-9-hashmap-computeifabsent-throws-concurrentmodificationexception-on

edytowany 2x, ostatnio: LitwinWileński
crejk
Nie wiem co ten wątek, który podesłałeś ma wspólnego z computeIfAbsent.
LitwinWileński
  • Rejestracja:prawie 3 lata
  • Ostatnio:6 dni
  • Postów:740
0
yarel napisał(a):
  1. return xmlParser.parse(singlePageFromData); jakie będzie zachowanie tego parsera, gdy raz na milion wywołań dostanie niepoprawny dokument XML?

właśnie, uzywam parsera z pakietu jakarta.xml może inicjować go za każdym użyciem:

Kopiuj
        try {
            return init(MyDtoXml.class).parse(dto);
        } catch (JAXBException e) {
            throw new DataParsingException(e.getMessage());
        }

    init(Class<?> clazz) throws JAXBException {
        var marshaller = JAXBContext.newInstance(clazz).createMarshaller();
        marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
        return new JakartaXmlParser(marshaller)
    }

bo miałem go jako Beana springowego czyli singleton.

YA
  • Rejestracja:prawie 10 lat
  • Ostatnio:8 dni
  • Postów:2370
0

JAXB wieki temu używałem, więc nie powiem Ci jak go teraz używać. Sprawdź sobie dokumentację czy ten parser jest thread safe, czy przypadkiem marshaller/unmarshaller nie utrzymują wewnętrznego stanu (to mogłoby być problemem w połączeniu z singletonem).

LitwinWileński
  • Rejestracja:prawie 3 lata
  • Ostatnio:6 dni
  • Postów:740
0

https://javaee.github.io/jaxb-v2/doc/user-guide/ch03.html#other-miscellaneous-topics-performance-and-thread-safety

the Marshaller, Unmarshaller, and Validator classes are not thread safe. (...) each time you need to unmarshal/marshal/validate a document. Just create a new Unmarshaller/Marshaller/Validator

jarekr000000
  • Rejestracja:ponad 8 lat
  • Ostatnio:około 4 godziny
  • Lokalizacja:U krasnoludów - pod górą
  • Postów:4707
3
  1. computeIfAbsent oczywiście można użyć i polecam (i nic nie zepsuje - wątki/problemy z so dotyczą czegoś zupełnie innego, nie mającego nic wspólnego z twoim kodem).
  2. Problem jest tu:
Kopiuj
private void invalidateCache() {
        this.xmlDataPaged = new ConcurrentHashMap<>();
    }

A) pole powinno być final
B) a w tej metodzie powinieneś robić
this.xmlDataPaged.clear()


jeden i pół terabajta powinno wystarczyć każdemu
edytowany 2x, ostatnio: jarekr000000
99xmarcin
  • Rejestracja:około 5 lat
  • Ostatnio:6 miesięcy
  • Postów:2420
0

W takich wypadkach jak ktoś nie umie w wątki a się pcha, to od razu blokuję PR i karzę użyć jednej z uznanych bibliotek:

Ludzie którzy nawet się znają na wielowątkowości mogą takie bugi do kodu wprowadzić że 100 thread dumpów nie pomoże rozwiązać problemu.


Holy sh*t, with every month serenityos.org gets better & better...
LitwinWileński
Ludzie w zespole muszą mieć ciężko z mądralinskim
99xmarcin
Mają bardzo ciężko na review, ale potem doceniają jak ich PagerDuty o 2 AM nie budzi...
LitwinWileński
A co ma do rzeczy zarozumialstwo z jakością kodu
99xmarcin
@LitwinWileński: nie pisze personalnie do ciebie więc nie masz się co obrażać, niemniej moja rada jest dobra i merytoryczna, przemyśl na spokojnie i tyle. Trzeba mieć dystans między kodem a programistą, ujowy kod każdemu się zdarza ale nie każdy kto napisał ujowy fragment kodu jest ujowym programistą...
AU
  • Rejestracja:ponad rok
  • Ostatnio:12 miesięcy
  • Postów:175
0

Nie możesz stacktrace wyświetlić? jakie były informacje w ramce przed wyjątkiem?
Takie gdb pozwala dowolną ilość ramek przeskoczyć do góry i na dół do wyjątku, możesz do dowolnej funkcji jaka była wcześniej wywołana i dalej leży na stosie.

Chodź oczywiście rozumiem, że to losowo wywala błąd, to nie jest tak łatwo się na niego przyczaić.
Dumpy też są fajne, ale jak jesteś dynamicznie podłączony debuggerem to możesz sobie swobodnie się przemieszczać po programie jak umarł.

LitwinWileński
  • Rejestracja:prawie 3 lata
  • Ostatnio:6 dni
  • Postów:740
0

https://javaee.github.io/jaxb-v2/doc/user-guide/ch03.html#other-miscellaneous-topics-performance-and-thread-safety

the Marshaller, Unmarshaller, and Validator classes are not thread safe. (...) each time you need to unmarshal/marshal/validate a document. Just create a new Unmarshaller/Marshaller/Validator

to chyba rozwiązało problem.

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.