Czy to jest optymalny kod

@Service
public class AuthService {

    // ...

    private AccessToken accessToken;

    public synchronized String getAccessToken() {

        if (accessToken != null && !accessToken.isExpired()) {
            return accessToken.getAccessToken();
        }

        /*
          Tutaj request do pobrania tokena ...
        */
        
        accessToken = getTokenFrom(responseBody);
        return accessToken.getAccessToken();
    }
}

Załóżmy, że token może być ważny przez 5min/1h/albo dłużej.
Czy jego odczyt współbieżny da się zrobić lepiej/optymalniej?

Oczywiście że da się lepiej. synchronized to w zasadzie najwolniejsze dostępne rozwiązanie.

1. Weź tam użyj jakiegoś AtomicReference
2. Najlepiej to ten token pobieraj sobie w innym wątku co te twoje 5min/1h tak żeby nigdy nie był expired kiedy ktoś wysyła request. W ten sposób nigdy nie będziesz miał dużego latency kiedy request musi czekać na token. Ten inny wątek niezależnie pobiera token i ustawia wartość w atomic reference

Poza tym masz tam buga w kodzie - TOC-TOA jakby nie patrzeć. Sprawdzasz czy token nie jest expired i dopiero potem cośtam wykonujesz i zwracasz. Co jeśli token wygaśnie pomiędzy tym ifem a wywołaniem accessToken.getAccessToken()?

Tak jak ci napisał @Shalom Oddziel wewnętrzne pobieranie tokena z cache od odświeżania go. Czyli zrób sobie osobny wątek "gdzieś", który będzie zapewniał, że token zawsze będzie ważny. Wewnątrz tokena musisz mieć datę końca, więc jak pobierzesz nowy, to wiesz, że o godzinie expTime - 5 minut należy go odświeżyć. W ten sposób unikniesz sytuacji, że wykonując jakąś akcję wymagającą uwierzytelnienia musisz czekać aż odświeży się token. nie będziesz też musiał synchronizować metody getAccessToken().

@lookacode1: nie prowadźmy dyskusji w komentarzach...

Hmm nie do końca rozumiem. Przykładowo jak pobrałem wcześniej token, który ma ważność 5min jednak ustawiłem mu -30s czyli w .isExpired() powinno zwrócić true gdy minie 4min 30s. W przypadku gdy nie ma race condition np. 4min 50s .isExpired() zwróci true więc pobieram nowy token i jest ok. W przypadku gdy jest race condition np. 4min 29.9999s .isExpired() zwraca false token niby wygasa ale mam jeszcze 30s zapasu więc user może z niego korzystać

Ok, jeśli masz tam te 30s to faktycznie nie powinieneś wpaść w race. Niemniej to mocno nieintuicyjne że isExpired zwraca true kiedy wcale nie jest expired ;)

Poza tym nie rozumiem jak request ma NIGDY nie trafić na expired token. Kiedyś w końcu musi trafić bo czas leci do przodu chyba, że ten oddzielny wątek, o którym mówisz ma działać w pętli co czas ważności i go odświeżać z automatu (?)

Oczywiście że dokładnie to ma robić. Ma działać w tle i pobierać nowy token zanim stary wygaśnie i podmieniać referencje. Dzięki temu handler requestów zawsze ma dostęp do aktualnego tokena i nigdy nie blokuje.

AtomicReference ok ale te rozwiązanie wyżej trzeba by było przerobić bo co jeśli 2 wątki odczytają, że .isExpired() = true i równolegle zaczną pobierać token oczywiście gdy nie ma synchronized?

Jw, zrób osobny wątek. Jeśli juz musisz to robić w trakcie requestu, to co za różnica? Nic się nie stanie jak pobierzesz 2 tokeny przecież.

Mam wrażenie ze patrzysz na to z jakiejś dziwnej strony. Pytałeś o to czy jest optymalnie, ale z jakiego punktu widzenia? Z punktu widzenia ile tokenów requestujesz? Czy z punktu widzenia jak szybko i ile requestów ten serwis da radę obsłużyć na raz? Bo z tym synchronized obsługujesz 1 request na raz. Jak wrzucisz tam samo atomic reference to obsługujesz INF requestów na raz, ale istnieje ryzyko że pobierzesz więcej tokenów niż potrzeba. Jak zrobisz osobny wątek na pobieranie tokenu to obsługujesz INF requestów + pobierasz token tylko kiedy wygaśnie.

@Shalom:

Czy z punktu widzenia jak szybko i ile requestów ten serwis da radę obsłużyć na raz?

Bardziej w tę stronę, czyli uznałem za mało optymalne rozwiązanie to, że token może być odczytywany przez jeden wątek na raz.

Doszedłem też do wniosku, że nie zawsze zastąpimy blok synchronized przez AtomicInteger/AtomicReference itp.
czasami trzeba np. przyblokować odczyt gdy zapis jest w trakcie. Postaram się ogarnąć to rozwiązanie z tym nowym wątkiem
jak znajdę czas bo mam deadline.

czasami trzeba np. przyblokować odczyt gdy zapis jest w trakcie

Bo? Eventual consistency się kłania. Może nie boli nas specjalnie że odczytamy starą wartość mimo że update przyszedł chwile przed odczytem, ale jeszcze go nie przeprocesowaliśmy. Pisze sporo aplikacji rozproszonych które obsługują względnie duży ruch i nie pamiętam nawet kiedy ostatni raz widziałem synchronized.

Bardziej w tę stronę

Jeśli chcesz mieć high-throughput / low-latency to musisz skupić sie na tym, żeby critical path przez które idzie request nie miało żadnych pauz, żadnego I/O, requestów sieciowych itd. Jak na krytycznej ścieżce masz odczyt z bazy danych to latency automatycznie skacze przynajmniej 10 razy w górę. Jeśli masz tam jakiś restowy request do innego serwisu to latency skacze 100 razy.
Jeszcze pół biedy jak masz tam jakieś magiczne asynci, reactive czy coś takiego, to przynajmniej throughput może być nadal dość wysoki, bo wątki nie będą się blokować tylko obsługiwać inne requesty póki czekaja na jakieś I/O, ale to komplikuje trochę kod.

lookacode1 napisał(a):

Doszedłem też do wniosku, że nie zawsze zastąpimy blok synchronized przez AtomicInteger/AtomicReference itp.
czasami trzeba np. przyblokować odczyt gdy zapis jest w trakcie. Postaram się ogarnąć to rozwiązanie z tym nowym wątkiem

Tak od siebie dodam, że od jakiegoś czasu Atomici mają metody:

• getPlain i setPlain
• getOpaque i setOpaque
• getAcquire i setRelease
• i w końcu get i set, które są odpowiednikiem getVolatile i setVolatile

W skrócie - wybierając jeden z tych trybów działania możesz spokojnie sobie zapewnić blokadę odczytu gdy zapis jest w trakcie. Co więcej, get i set właśnie z defaultu zapewniają, że odczyt zmiennej będzie zawsze blokowany przez zapis.

@Shalom:

Eventual consistency się kłania.

Hmm wydaje mi się, że eventual consistency nie zawsze się sprawdza bo co mi po tokenie, który jest już nieaktualny/nieważny
w związku z tym jest bezużyteczny.

Jeśli chcesz mieć high-throughput / low-latency to musisz skupić sie na tym, żeby critical path przez które idzie request nie miało żadnych pauz, żadnego I/O ...

Zwykle to jest raczej normalne, że aplikacje komunikują się z bazą danych albo z innymi serwisami przez sieć więc chyba nie da się tego
całkowicie uniknąć ewentualnie zminimalizować / przyspieszyć ?

nie zawsze się sprawdza bo co mi po tokenie, który jest już nieaktualny/nieważny
w związku z tym jest bezużyteczny.

@lookacode1 wiadomo że nie da się zawsze na czymś takim polegać, czasem koniecznie potrzebujesz synchronicznych operacji, ale w praktyce dużo rzadziej niż ludzie ich używają. Niemniej zauważ ze u ciebie mógłbyś pobierać nowy token zanim stary wygaśnie i zawsze mieć w cache aktualny token i jednocześnie nigdy nie musieć blokować requestu użytkownika.

Zwykle to jest raczej normalne, że aplikacje komunikują się z bazą danych albo z innymi serwisami przez sieć więc chyba nie da się tego całkowicie uniknąć ewentualnie zminimalizować / przyspieszyć ?

Jasne, ale nie muszą tego robić synchronicznie w trakcie obsługi requestu od użytkownika.
Np. użytkownicy mogą cośtam dodawać do naszej bazy danych. Mozemy każdy taki request obsługiwać synchronicznie i blokować odpowiedź do użytkownika dopóki nie wykonamy zleconej operacji. To generuje nam bottleneck na bazie i jak nagle dostaniemy np. 100k requestów w ciągu sekundy, to wszystko stanie i użytkownicy nie będą mogli korzystać z aplikacji.
Alternatywnie mozemy te wszystkie inserty zbierać do kolejki, użytkownikowi odpowiadać od razu, a w tle asynchronicznie pobierać z kolejki i wrzucać do bazy. W takiej sytuacji jakieś nagłe piki w requestach nie robią nam dużej różnicy, ot na chwile kolejka urośnie ale rozładuje się po chwili.

Podobnie np. mamy serwis który korzysta z informacji dostarczanych przez inne serwisy. Możemy synchronicznie za każdym razem pobierać te dane, a możemy je np. cachować i pobierać nową wersje tylko raz na jakiś czas, znacznie obniżając latency kolejnych requestów (ale pierwszy nadal jest wolny). A możemy iść o krok dalej i pobierać te dane asychronicznie w tle i zupełnie wyeliminować długie pauzy.

To oczywiście zakłada, że sensownie ustawimy cache expiration czy timer który triggeruje nasze asynchronicznie pobieranie - jeden cache np. może trzymać dane godzinę bo wiemy że te dane praktycznie się nie zmieniają, a inny musi robić refresh co 5 sekund.

Wiadomo że nie zawsze da sie coś takiego osiągnąć, ale w bardzo dużej liczbie przypadków taki model spokojnie sobie radzi w praktyce.

@Shalom:

Niemniej zauważ ze u ciebie mógłbyś pobierać nowy token zanim stary wygaśnie i zawsze mieć w cache aktualny token i jednocześnie nigdy nie musieć blokować requestu użytkownika.

Zdałem sobie sprawę, że to ja w rq ustalam czas życia tokena i inny mikroserwis mi taki wygeneruje o takim czasie życia bo to jest wewnętrzny token
więc równie dobrze mógłbym wygenerować go na tydzień,miesiąc? Wtedy taki dodatkowy wątek, który niby pilnuje, że token jest zawsze ważny
wydaję się bez sensu bo przez większość czasu by spał więc na razie zostawiłem tj. jest bo terminy gonią ale pewnie zamienię po prostu
ten synchronized na AtomicReference bez tego dodatkowego wątku a jeśli jednocześnie od czasu do czasu 2 rq pójdą po nowy token to w sumie nic się nie stanie.

Alternatywnie mozemy te wszystkie inserty zbierać do kolejki, użytkownikowi odpowiadać od razu, a w tle asynchronicznie pobierać z kolejki i wrzucać do bazy.

A co z transakcyjnością?

A możemy iść o krok dalej i pobierać te dane asychronicznie w tle i zupełnie wyeliminować długie pauzy.

A to nie jest tak, że to prowadzi do tego, że tworzycie lokalnie w pamięci kopię bazy jakiejś zewnętrznej usługi?
To równie dobrze aplikacja zamiast za każdym razem synchronicznie strzelać do swojej bazy z zapytaniem
może mieć lokalnie cache jakieś tabeli i w tle co jakiś czas aktualizować ten cache tj. mówisz.

A co z transakcyjnością?

Co z nią? Kolejność requestów jest zachowana w kolejce. Jedyna różnica to eventual consistency ;) tzn ktoś moze odczytać "starą" wartość jeśli jeszcze nie przetworzyliśmy updatu. Trzeba sobie zadać pytanie czy robi nam to specjalnie różnicę czy nie. Coś za coś.

A to nie jest tak, że to prowadzi do tego, że tworzycie lokalnie w pamięci kopię bazy jakiejś zewnętrznej usługi?

W pewnym sensie tak właśnie jest, chociaż cache zwykle trzyma tylko rzeczy które niedawno były używane a nie wszystko. Niemniej może być też tak ze trzymamy w pamięci wszystko, a ta "baza danych" to jest tylko po to, zeby dane były utrwalone i restart aplikacji niczego nie zgubił. Jeśli zależy ci na latency to nie da się specjalnie inaczej. I/O dyskowe jest wolne, I/O sieciowe jeszcze wolniejsze. Żeby w ogóle myśleć o szybkich odpowiedziach to musisz lecieć tylko z pamięci bez blokowania się na I/O.

@Shalom:

Co z nią? Kolejność requestów jest zachowana w kolejce

Bardziej chodziło mi o to co jeżeli dla usera zwrócisz 200 a później wyciągasz jego żądanie z kolejki przetworzysz i poleci rollback :)

Żeby w ogóle myśleć o szybkich odpowiedziach to musisz lecieć tylko z pamięci bez blokowania się na I/O

A jeżeli np. używany jest jakiś cache rozproszony typu redis, hazelcast czyli de facto I/O sieciowe to też warto robić cache tego cache'a żeby nie wchodzić na IO
tylko z pamięci?

Transakcyjność nie jest za darmo. Jeżeli nie jest potrzebna, to nie należy jej używać. W przypadku kiedy potrzebujesz auth token, nie jest ona potrzebna, bo interesuje cię, tylko to, żeby użyć wciąż ważnego tokena, a nie najnowszego.

lookacode1 napisał(a):

Bardziej chodziło mi o to co jeżeli dla usera zwrócisz 200 a później wyciągasz jego żądanie z kolejki przetworzysz i poleci rollback :)

Nie zwracasz 200, tylko 202, a w nim uchwyt do sprawdzenia, czy udało się zrealizować pierwotne żądanie. Albo możesz zadbać, żeby żądanie zostało zawsze zrealizowane. Przykładowo, pierwszy sposób może być wykorzystywany w przypadku przelewów bankowych - wysyłasz komendę wykonania przelewu z A do B, serwer potwierdza otrzymanie zlecenia, następnie otwiera transakcję, wykonuje operacje zapisania zlecenia, pomniejszenia A o 100zł, powiększenia B o 100zł, aktualizuje status zlecenia i zamyka transakcję, jeżeli na koncie A zabrakło środków, albo konto B zostało zamknięte przed nadejściem przelewu użytkownik może sobie później zobaczyć powody dla których zlecenie się nie wykonało Druga opcja to obsługa transakcji kartą płatniczą - nie ważne co by się nie działo, konto zawsze zostanie obciążone (nawet jeżeli saldo spadnie poniżej 0)

Żeby w ogóle myśleć o szybkich odpowiedziach to musisz lecieć tylko z pamięci bez blokowania się na I/O

Zależy jak bardzo potrzebujesz wydajności i na co możesz sobie pozwolić. Narzut na komunikację oczywiście będzie, ale jeżeli np. masz 10 instancji jakiegoś serwisu, które muszą się komunikować pomiędzy sobą, to wiele nie zrobisz, bo czasami po prostu potrzebujesz jakiejś współdzielonej wartości.

Nie zwracasz 200, tylko 202, a w nim uchwyt do sprawdzenia, czy udało się zrealizować pierwotne żądanie.

Ale to chyba nie zadziała przy system2system gdzie np. w zależności od (nie)powodzenia robimy różne rzeczy synchronicznie?
Bo np. na froncie to js'em można co jakiś czas sprawdzać jaki jest status transakcji tym uchwytem i potem pokazać użytkownikowi,
że sukces itp.

A jeżeli np. używany jest jakiś cache rozproszony typu redis, hazelcast czyli de facto I/O sieciowe to też warto robić cache tego cache'a żeby nie wchodzić na IO tylko z pamięci?

To zależy co piszesz i jak bardzo zależy ci na latency. Ja nie twierdzę że zawsze należy tak robić. Jak ktoś klepie CRUDa to pewnie nie ma to znaczenia, ale jak piszesz jakiś system tradingowy który ma obsługiwać setki tysięcy requestów na sekundę i odpowiadać w kilka milisekund, to wtedy nie tyle warto, co nawet trzeba ;)

Zależy jak bardzo potrzebujesz wydajności i na co możesz sobie pozwolić. Narzut na komunikację oczywiście będzie, ale jeżeli np. masz 10 instancji jakiegoś serwisu, które muszą się komunikować pomiędzy sobą, to wiele nie zrobisz, bo czasami po prostu potrzebujesz jakiejś współdzielonej wartości.

Jw, oczywiście że dobiera sie rozwiązania do problemu i jak nie potrzebujesz odpowiadać w 5 milisekund, tylko możesz sobie pozwolić na 500, to wtedy można sie bawić w synchroniczne requesty do innych serwisów.

to wiele nie zrobisz, bo czasami po prostu potrzebujesz jakiejś współdzielonej wartości.

Zapewniam że można wiele zrobić ;)

1. Można wysyłać eventy do wszystkich instancji, tak żeby każda z nich miała dokładnie taki sam stan swojego cache w pamięci. Czyli np. przychodzi update od usera, jest obsługiwany przez serwis zajmujący się "zapisem", który go przetwarza a następnie emituje event, że taka operacja nastąpiła. Dzięki temu serwisy "czytające" nie potrzebują synchronicznie pobierać niczego w trakcie obsługi requestu.
2. Alternatywnie serwisy czytające, mogą co jakiś czas robić sobie odczyt i aktualizować swoje wewnętrzne cache, podobnie jak w tym przykładzie z tokenem tutaj.

Załóżmy np. ze mamy taki problem jak OP, tylko instancji mamy 100 i chcemy żeby wszystkie używały tego samego tokena. Moglibyśmy w takim razie zamiast osobnego wątku mieć w ogóle osobny serwis który zajmuje się pobieraniem tego tokena co jakiś czas i może emitować event do innych instancji kiedy pojawia się nowy token, albo każda instancja aplikacji co zadany czas pobiera z tego serwisu aktualny token.

Można by zamiast ciężkiego serwisu użyć jakiegoś Serverless jak AWS Lambda, bo mamy mały kawałek kodu który chcemy odpalić tylko raz na jakiś czas. W opcji 2 można by wtedy taki token wrzucic do jakiegoś Redisa, z którego co jakiś czas pozostałe serwisy czytałyby sobie aktualną wartość.

Ale to chyba nie zadziała przy system2system

Czemu nie? Jeśli potrzebujesz zsynchronizować akcje, to nic nie stoi na przeszkodzie zeby serwis który wysyłał request zablokował się aż nie sprawdzi powodzenia. Czyli wysłałeś request, dostajesz jakiś callback i jeśli jest dla ciebie ważne żeby nie "iść dalej" póki nie masz pewności że wszystko się powiodło, to czekasz na tym callbacku.
Trik polega na tym ze zawsze możesz zsynchronizować kod, ale jak masz kod synchroniczny to niewiele już można zrobić.

@lookacode1:

A podeślesz jakiegoś linka gdzie jest opisane, że właśnie tak to działa?

Najlepsze to są chyba javadoci :)
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/util/concurrent/atomic/AtomicReference.html
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/invoke/VarHandle.html

Np.

getAcquire - Returns the value of a variable, and ensures that subsequent loads and stores are not reordered before this access.
setRelease - Sets the value of a variable to the newValue, and ensures that prior loads and stores are not reordered after this access.

Przy czym widzę, że tutaj trochę dyskusja zeszła na rozmowę o systemach rozproszonych - to dotyczy tylko sytuacji, w której kilka wątków na jednej maszynie próbuje dostać się do zmiennej.

1. W przypadku gdy ustawianie wartości nie może być operacją atomiczną. Np. masz metodę refreshValue, która pobiera dane z bazy danych - i chcesz się upewnić, że od momentu startu refreshValue aż do jej zakończenia żaden inny wątek nie sięgnie po zmienną - użyj locka.
2. Wyżej wymieniona metoda sprawdzi się też przy systemach rozproszonych - ale najlepiej opakować to w jakiś uchwyt asynchroniczny.

@Shalom:

albo każda instancja aplikacji co zadany czas pobiera z tego serwisu aktualny token

A to nie jest z punktu widzenia bezpieczeństwa słabe?
Np. mamy A1...A100 instancji i mamy X, z którego te instancje pobierają token do Y.
To teraz jaki jest sens zabezpieczenia serwisu Y tokenem skoro od X można pobrać go za free?
Albo np. wystarczy, że gdzieś na jakiś topic się zasubskrybuje i mam tokeny za free.
Pytanie czy wgl potrzebny jest token może wystarczy 2waySSL? A może mówimy o tym, że Y
to jest podsystem zewnętrzny, który nie należy do naszej infrastruktury wtedy to ma sens.