Java: Mám neprázdný Set, ale nemůžu z něj dostat položky

[Javista]

Mám v javě

Kód: [Vybrat]

package cz.exambuilder.entity.domain.calendar;
(...)
public class Calendar {
    protected static final Set<Image> imagesQueue = new LinkedHashSet<>();
    (...)
}


Je potřeba pohlídat že je důsledně zajištěno že do imagesQueue nikdy nebude přistupovat více threadů zároveň! Vzhledem k tomu že jde o statickou proměnnou tak se tato chyba může stát velmi jednoduše. Doporučuji používat nástroje pro statickou analýzu kódu (např. FindBugs), ty takovéhle základní věci odhalují.

Samozřejmě že synchronizace přes instanci Calendar není dostačující pokud existuje více instancí Calendar - představte si co se stane když přes dvě instance Calendar zároveň přistupujete k té samé (protože statické) proměnné imagesQueue - to je jasný recept na rozbití vnitřního stavu LinkedHashSet!

K imagesQueue se nikdo jiný než Calendar nedostane, je privátní a nemá getter ani setter. Takže by mělo stačit synchronizovat příslušné metody v Calendar.

Ani omylem! I kdyby zrovna teď platilo že víc instanci Calendar v aplikaci nevzniká, do budoucna se to může změnit a pak se to rozbije. Přístup ke statické instanci LinkedHashSet musí být chráněný jedním jediným mutexem a to klíčové slovo synchronized u nestatické metody nezajišťuje. Klasické řešení by bylo použít synchronized(imagesQueue) nebo rovnou v imagesQueue mít Collections.synchronizedSet(new LinkedHashSet()). Případně s ohledem na to že se proměnná jmenuje Queue použít úplně jinou třídu.

Chyba byla asi v tom, že jsem pouze přidal klíčové slovo synchronized k těm metodám - s vědomím toho, že Calendar je singleton, což ale asi neplatí, protože tam může být ve skutečnosti více ekvivalentních instancí (v terminologii frameworku se tomu ale říká singleton). Nechal jsem se tím označením zřejmě svést (ale musím to ještě ověřit).

Používat frameworky bez jejich znalosti je cesta do pekel.

[Reklama]

[Javista]

Mám v javě

Kód: [Vybrat]

package cz.exambuilder.entity.domain.calendar;
(...)
public class Calendar {
    protected static final Set<Image> imagesQueue = new LinkedHashSet<>();
    (...)
}


Je potřeba pohlídat že je důsledně zajištěno že do imagesQueue nikdy nebude přistupovat více threadů zároveň! Vzhledem k tomu že jde o statickou proměnnou tak se tato chyba může stát velmi jednoduše. Doporučuji používat nástroje pro statickou analýzu kódu (např. FindBugs), ty takovéhle základní věci odhalují.

Samozřejmě že synchronizace přes instanci Calendar není dostačující pokud existuje více instancí Calendar - představte si co se stane když přes dvě instance Calendar zároveň přistupujete k té samé (protože statické) proměnné imagesQueue - to je jasný recept na rozbití vnitřního stavu LinkedHashSet!

K imagesQueue se nikdo jiný než Calendar nedostane, je privátní a nemá getter ani setter.

Ještě si prosím nastudujte co přesně znamená klíčové slovo protected - rozhodně není vhodné ve spojení s ním používat výraz privátní, to je velmi zavádějící!

[Kit]

Ještě si prosím nastudujte co přesně znamená klíčové slovo protected - rozhodně není vhodné ve spojení s ním používat výraz privátní, to je velmi zavádějící!

Všechny atributy objektu mají být private, jinak si vývojář koleduje o průšvih.

[Filip Jirsák]

K imagesQueue se nikdo jiný než Calendar nedostane, je privátní a nemá getter ani setter. Takže by mělo stačit synchronizovat příslušné metody v Calendar. Chyba byla asi v tom, že jsem pouze přidal klíčové slovo synchronized k těm metodám - s vědomím toho, že Calendar je singleton, což ale asi neplatí, protože tam může být ve skutečnosti více ekvivalentních instancí (v terminologii frameworku se tomu ale říká singleton). Nechal jsem se tím označením zřejmě svést (ale musím to ještě ověřit).

To použití je špatné tak jako tak. Pokud ten objekt má být singleton na zodpovědnost uživatele („není důvod mít víc instancí, ale když vzniknou, nemělo by to ničemu vadit“), má mít ten set jako instanční proměnnou. Pokud to má být tvrdý singleton (nesmí existovat víc kopií), musíte v kódu zajistit, aby to opravdu nemohlo nastat. Navíc takovéhle použití statického fieldu je bad practice, nic tím nezískáte a akorát si zavíráte dveře – nebudete moci ten objekt pořádně testovat, bude špatně spolupracovat s frameworky atd.

[Kit]

... s vědomím toho, že Calendar je singleton, ...

Proč je Calendar singleton? Copak není možné mít více kalendářů? Vidím to na nesmyslně použitý návrhový vzor.

[Reklama]

[Ondrej Nemecek]

Navíc takovéhle použití statického fieldu je bad practice, nic tím nezískáte a akorát si zavíráte dveře – nebudete moci ten objekt pořádně testovat, bude špatně spolupracovat s frameworky atd.

Získám tím sdílený stav.

[Ondrej Nemecek]

Mám v javě

Kód: [Vybrat]

package cz.exambuilder.entity.domain.calendar;
(...)
public class Calendar {
    protected static final Set<Image> imagesQueue = new LinkedHashSet<>();
    (...)
}


Je potřeba pohlídat že je důsledně zajištěno že do imagesQueue nikdy nebude přistupovat více threadů zároveň! Vzhledem k tomu že jde o statickou proměnnou tak se tato chyba může stát velmi jednoduše. Doporučuji používat nástroje pro statickou analýzu kódu (např. FindBugs), ty takovéhle základní věci odhalují.

Samozřejmě že synchronizace přes instanci Calendar není dostačující pokud existuje více instancí Calendar - představte si co se stane když přes dvě instance Calendar zároveň přistupujete k té samé (protože statické) proměnné imagesQueue - to je jasný recept na rozbití vnitřního stavu LinkedHashSet!

K imagesQueue se nikdo jiný než Calendar nedostane, je privátní a nemá getter ani setter.

Ještě si prosím nastudujte co přesně znamená klíčové slovo protected - rozhodně není vhodné ve spojení s ním používat výraz privátní, to je velmi zavádějící!

Změnil jsem to na private.

[Ondrej Nemecek]

... s vědomím toho, že Calendar je singleton, ...

Proč je Calendar singleton? Copak není možné mít více kalendářů? Vidím to na nesmyslně použitý návrhový vzor.

Právě že to není návrhový vzor, ale způsob, jak se frameworku řekne, že tenhle objekt tam je právě jednou a že si uživatel nemá udělat kopii. Popletení těch významů je příčinou mého problému.

[Ondrej Nemecek]

K imagesQueue se nikdo jiný než Calendar nedostane, je privátní a nemá getter ani setter. Takže by mělo stačit synchronizovat příslušné metody v Calendar. Chyba byla asi v tom, že jsem pouze přidal klíčové slovo synchronized k těm metodám - s vědomím toho, že Calendar je singleton, což ale asi neplatí, protože tam může být ve skutečnosti více ekvivalentních instancí (v terminologii frameworku se tomu ale říká singleton). Nechal jsem se tím označením zřejmě svést (ale musím to ještě ověřit).

Takže je to skutečně tak, že těch instancí Calendar je tam spousta (ale jsou identické - equals vrací true). To je asi ponaučení, co si odnáším a pak taky Javistovo shrnutí:

K imagesQueue se nikdo jiný než Calendar nedostane, je privátní a nemá getter ani setter. Takže by mělo stačit synchronizovat příslušné metody v Calendar.

Ani omylem! I kdyby zrovna teď platilo že víc instanci Calendar v aplikaci nevzniká, do budoucna se to může změnit a pak se to rozbije. Přístup ke statické instanci LinkedHashSet musí být chráněný jedním jediným mutexem a to klíčové slovo synchronized u nestatické metody nezajišťuje. Klasické řešení by bylo použít synchronized(imagesQueue) nebo rovnou v imagesQueue mít Collections.synchronizedSet(new LinkedHashSet()). Případně s ohledem na to že se proměnná jmenuje Queue použít úplně jinou třídu.

Tímto všem děkuji za angažmá a považuju to za vyřešené :-)

[Zdenek Henek]

Takže nevím, zda je root ideální server pro tento dotaz, ale zkusím to 

Mám v javě

Kód: [Vybrat]

package cz.exambuilder.entity.domain.calendar;
(...)
public class Calendar {
    protected static final Set<Image> imagesQueue = new LinkedHashSet<>();
    (...)
}

a ten set imagesQueue je neprázdný:

Kód: [Vybrat]

imagesQueue.isEmpty(); // dává false
imagesQueue.size(); // dává 1

Problém je, že položky toho setu nemůžu získat:

Kód: [Vybrat]

imagesQueue.iterator().hasNext(); // dává false
imagesQueue.iterator().next(); //  hodí java.util.NoSuchElementException.

Podstatné, že k tomuto stavu dojde až po cca měsíčním běhu aplikace, zprvu to funguje normálně.

Čichal bych tady nějaký problém v implementaci equals(), hashCode() nebo compareTo(), ale na nic jsem nepřišel, používám výchozí implementace z frameworku. Možná vstoupí do hry nějaký JIT nebo přesun instancí do jiné oblasti paměti a změní se hashcode nebo něco podobného? Na co se mám zaměřit? Uvažoval jsem ještě nad tím, zda se to do tohoto stavu nemůže dostat špatně ošetřeným konkurenčním přístupem, ale nedokážu si to představit.

Pozitivní je, že mám k dispozici testovací prostředí, kde to ještě funguje i prostředí, kde to už nefunguje - do obou se můžu se připojit debuggerem nebo z vývojové konzole umožňující spouštět interaktivně kód.

Na co se mám zaměřit? Kdy k něčemu takovému může v principu vůbec dojít?

Používám java version "1.8.0_161", apache-tomcat-8.5.5, Debian 4.8.4-1 a Brightspot ve verzi v3.2.7178-2110f8

Aktuální testovací kód:

Kód: [Vybrat]

import cz.exambuilder.entity.domain.calendar.Calendar;
import java.util.stream.*;
import java.lang.reflect.*;
import org.slf4j.*;

public class Code {
    public static Object main() throws Throwable {

        Logger log = LoggerFactory.getLogger(Code.class);

        // tady získám tu privátní instanci imagesQueue, abych se na ní mohl podívat:

        Calendar calendar = Query.from(Calendar.class).first();
        Field field = Calendar.class.getDeclaredField("imagesQueue");
        field.setAccessible(true);
        Set<Image> imagesQueue = (Set<Image>) field.get(calendar);

        // tady zkouším, co imagesQueue obsahuje:

        log.info("*** imagesQueue.isEmpty(): " + imagesQueue.isEmpty()); // false
        log.info("*** imagesQueue.size(): " + imagesQueue.size()); // 1

        log.info("*** imagesQueue.stream(): " + imagesQueue.stream().map(i -> String.format("%s (%s)", i.getTitle(), i.getId())).collect(Collectors.toList())); // prázdná kolekce
        log.info("*** imagesQueue.iterator().hasNext(): " + imagesQueue.iterator().hasNext()); // false

        try {
           log.info("*** imagesQueue.iterator().next(): " + imagesQueue.iterator().next()); // NoSuchElementException
        } catch(Exception e){
            log.info("*** exception: " + e);
        }
       
        return "SEE LOGS";

    }
}

Dík za nápady!

Nepouzivejte plain Set jak static promenou.

Zabalte to do nejake tridy a pak bud vse synchrnonizujte, nebo at  veskere operace v metode nemeni tu mnozinu.

e.g.

class MySet{
.....
  MySet(Set<Image> a){
    this.a = new HashSet(a);
  }

  ... metody pro praci s tou mnozinou...
  tu mnozinu nepoustejte ven z tohoto objektu

  pokud budete predavat objekty z te mnoziny ven, musite zajistit ze budou nemenne, nebo opet obalte obalovou tridou a synchronizujte. Vice v Concurrency in Practice strana 39, Publication and escape.
}

Jako bonus toto Vam pomuze udrzovat kod, ktery nejak manipuluje s tou mnozinou na jednom miste.

Jetse me tak napada, ze byste mohl spis pouzivat neco jako https://github.com/ben-manes/caffeine at Vam tam ty obrazky nevytvari memory leak a po case je vyhazovat. Jetsli teda mate moznost si ty obrazky opet vytvorit, kdyz uz nebudou v te mnozine.

[Filip Jirsák]

Získám tím sdílený stav.

Získáte tím stav sdílený mezi jedním singletonem. To je trošku nesmysl, ne? A je to přesně to, co vás vypeklo – že jste jeden singleton měl z poloviny udělaný jedním způsobem a z poloviny druhým.

[Kit]

Nepouzivejte plain Set jak static promenou.

Zabalte to do nejake tridy a pak bud vse synchrnonizujte, nebo at  veskere operace v metode nemeni tu mnozinu.

Ano, přesně takhle se to má dělat.

[Ondrej Nemecek]

Získám tím sdílený stav.

Získáte tím stav sdílený mezi jedním singletonem. To je trošku nesmysl, ne? A je to přesně to, co vás vypeklo – že jste jeden singleton měl z poloviny udělaný jedním způsobem a z poloviny druhým.

Ano, proto by skutečně dávalo smysl tu funkčnost vytáhnout bokem jak uvádí Zdenek Henek Tam bych si mohl i sám řešit, zda to je tvrdý singleton nebo ne - aniž by na to měl vliv framework (tj. bylo by to vyčlenění kompetence). Co mi ještě není úplně jasné je, čemu se vystavuju, pokud bych vracel ven měnitelné položky toho setu (neobalil je jak uvádí Zdenek Henek)? Pokud se podívám, jak framework implementuje hashCode, compareTo a equals, neměl bych v kolekcích ani při modifikaci narazit na problém. Je to tak? Resp. se to asi dočtu v té kapitole Publication and escape v Java Concurrency in Practice.

[Zdenek Henek]

Získám tím sdílený stav.

Získáte tím stav sdílený mezi jedním singletonem. To je trošku nesmysl, ne? A je to přesně to, co vás vypeklo – že jste jeden singleton měl z poloviny udělaný jedním způsobem a z poloviny druhým.

Ano, proto by skutečně dávalo smysl tu funkčnost vytáhnout bokem jak uvádí Zdenek Henek Tam bych si mohl i sám řešit, zda to je tvrdý singleton nebo ne - aniž by na to měl vliv framework (tj. bylo by to vyčlenění kompetence). Co mi ještě není úplně jasné je, čemu se vystavuju, pokud bych vracel ven měnitelné položky toho setu (neobalil je jak uvádí Zdenek Henek)? Pokud se podívám, jak framework implementuje hashCode, compareTo a equals, neměl bych v kolekcích ani při modifikaci narazit na problém. Je to tak? Resp. se to asi dočtu v té kapitole Publication and escape v Java Concurrency in Practice.

Pokud mate synchronizovane oprace nad Set<...> tak to nestaci pokud neco co je v te Set pustite mimo synchronized sekci a pak to tam menite. Musite zajistit, ze se to bud nebude menit - nejlepsi, nebo ze veskere zmeny budou opet synchronizovane.

Jestli muzu doporucit, poridte si tu knihu Java Concurrency in practice. Nebojte se, ze je z roku 2006, jedine, co tam nenajdete jsou nove veci implementovane pozdeji, ale zakladni princip Vasi otazky tam je moc dobre popsany.

Pochybuju ze objectk Image je immutable, pokud je, tak nemusite nic delat. Immutable, znamena, ze bez reflexe se s tim neda nic delat, ne ze si slibite, ze to nebudete menit, protoze na to zapomenete. Jeste muzete pred kazdou modifikaci udelat clon toho objektu a po provedeni zmen vyhodit ze Set objektu ten stary a dat tam novy ... , ale to je cesta do pekel. Pak je clone na kazdem rohu.

Nejjednodussi pravidlo je, ze cokoli je sdilene je immutable a ma to builder. Pokud to nejde, tak wrapper a synchronizace.

[Filip Jirsák]

Co mi ještě není úplně jasné je, čemu se vystavuju, pokud bych vracel ven měnitelné položky toho setu (neobalil je jak uvádí Zdenek Henek)?

To není úplně přesné. Java collections framework má nějaké požadavky na položky, které se do kolekcí vkládají – na hashCode, equals a compare. Respektive dokonce každá implementace kolekcí může mít ty požadavky trochu jiné. Podstatné je splnění těch požadavků. Lapidárně řečeno, objekt se může měnit, jak chce, ale dokud bude mít stejný hashCode a bude správně odpovídat na equals, je to v pořádku.

Pokud se podívám, jak framework implementuje hashCode, compareTo a equals, neměl bych v kolekcích ani při modifikaci narazit na problém. Je to tak? Resp. se to asi dočtu v té kapitole Publication and escape v Java Concurrency in Practice.

Nejprve si musíte určit, co vlastně znamená, že se jedná o ten samý objekt. Ale pokud vám z toho vyjde, že jeden objekt (konkrétní místo v paměti) může v průběhu své životnosti představovat několik různých objektů (hashCode a equals zahrnují položky, které se mohou změnit), nebude vám to s většinou kolekcí fungovat.

Tohle ale s vícevláknovým přístupem nesouvisí. Vám ten vícevláknový přístup nejspíš rozbil vnitřní implementaci HashMapy použitou v tom LinkedHashSetu. To by modifikovatelné prvky kolekce nezpůsobily, ty by mohly způsobit nanejvýš to, že byste do kolekce nějaký prvek vložil, ale on by tam později nebyl, nebo byste do kolekce vložil prvek, ale později byste tam našel jiný.