Sincronizzazione su oggetti String in Java

StackOverflow https://stackoverflow.com/questions/133988

Domanda

Ho una webapp su cui sto eseguendo alcuni test di carico / prestazioni, in particolare su una funzionalità in cui prevediamo che alcune centinaia di utenti accedano alla stessa pagina e premano l'aggiornamento ogni 10 secondi circa su questa pagina. Un'area di miglioramento che abbiamo scoperto di poter realizzare con questa funzione è stata la memorizzazione nella cache delle risposte dal servizio Web per un certo periodo di tempo, poiché i dati non cambiano.

Dopo aver implementato questa cache di base, in alcuni ulteriori test ho scoperto che non avevo considerato come thread simultanei potessero accedere alla cache contemporaneamente. Ho scoperto che nel giro di ~ 100 ms, circa 50 thread stavano cercando di recuperare l'oggetto dalla cache, scoprendo che era scaduto, colpendo il servizio web per recuperare i dati e quindi rimettendo l'oggetto nella cache.

Il codice originale era simile al seguente: 

private SomeData[] getSomeDataByEmail(WebServiceInterface service, String email) {

  final String key = "Data-" + email;
  SomeData[] data = (SomeData[]) StaticCache.get(key);

  if (data == null) {
      data = service.getSomeDataForEmail(email);

      StaticCache.set(key, data, CACHE_TIME);
  }
  else {
      logger.debug("getSomeDataForEmail: using cached object");
  }

  return data;
}

Quindi, per essere sicuro che solo un thread chiamasse il servizio web quando l'oggetto in chiave è scaduto, ho pensato che avrei dovuto sincronizzare l'operazione get / set della cache, e mi sembrava di usare il la chiave di cache sarebbe un buon candidato per la sincronizzazione di un oggetto (in questo modo, le chiamate a questo metodo per email b@b.com non sarebbero bloccate dalle chiamate di metodo a a@a.com).

Ho aggiornato il metodo in questo modo: 

private SomeData[] getSomeDataByEmail(WebServiceInterface service, String email) {


  SomeData[] data = null;
  final String key = "Data-" + email;

  synchronized(key) {      
    data =(SomeData[]) StaticCache.get(key);

    if (data == null) {
        data = service.getSomeDataForEmail(email);
        StaticCache.set(key, data, CACHE_TIME);
    }
    else {
      logger.debug("getSomeDataForEmail: using cached object");
    }
  }

  return data;
}

Ho anche aggiunto linee di registrazione per cose come "prima del blocco di sincronizzazione", "all'interno del blocco di sincronizzazione", "in procinto di lasciare il blocco di sincronizzazione" e "dopo il blocco di sincronizzazione", in modo da poter determinare se ero effettivamente sincronizzare l'operazione get / set.

Tuttavia non sembra che abbia funzionato. I miei log di test hanno output come:

  

(l'output del registro è 'nome thread' 'nome logger' 'messaggio')
  http-80-Processor253 jsp.view-page - getSomeDataForEmail: sta per entrare nel blocco di sincronizzazione
  http-80-Processor253 jsp.view-page - getSomeDataForEmail: blocco di sincronizzazione interno
  http-80-Processor253 cache.StaticCache - get: oggetto nella chiave [SomeData-test@test.com] è scaduto
  http-80-Processor253 cache.StaticCache - get: key [SomeData-test@test.com] valore di ritorno [null]
  http-80-Processor263 jsp.view-page - getSomeDataForEmail: sta per entrare nel blocco di sincronizzazione
  http-80-Processor263 jsp.view-page - getSomeDataForEmail: blocco di sincronizzazione interno
  http-80-Processor263 cache.StaticCache - get: oggetto nella chiave [SomeData-test@test.com] è scaduto
  http-80-Processor263 cache.StaticCache - get: key [SomeData-test@test.com] valore di ritorno [null]
  http-80-Processor131 jsp.view-page - getSomeDataForEmail: sta per entrare nel blocco di sincronizzazione
  http-80-Processor131 jsp.view-page - getSomeDataForEmail: blocco di sincronizzazione interno
  http-80-Processor131 cache.StaticCache - get: oggetto nella chiave [SomeData-test@test.com] è scaduto
  http-80-Processor131 cache.StaticCache - get: key [SomeData-test@test.com] valore di ritorno [null]
  http-80-Processor104 jsp.view-page - getSomeDataForEmail: blocco di sincronizzazione interno
  http-80-Processor104 cache.StaticCache - get: oggetto nella chiave [SomeData-test@test.com] è scaduto
  http-80-Processor104 cache.StaticCache - get: key [SomeData-test@test.com] valore di ritorno [null]
  http-80-Processor252 jsp.view-page - getSomeDataForEmail: sta per entrare nel blocco di sincronizzazione
  http-80-Processor283 jsp.view-page - getSomeDataForEmail: sta per entrare nel blocco di sincronizzazione
  http-80-Processor2 jsp.view-page - getSomeDataForEmail: sta per entrare nel blocco di sincronizzazione
  http-80-Processor2 jsp.view-page - getSomeDataForEmail: blocco di sincronizzazione interno

Volevo vedere solo un thread alla volta che entrava / usciva dal blocco di sincronizzazione attorno alle operazioni get / set.

C'è un problema nella sincronizzazione degli oggetti String? Ho pensato che la chiave di cache sarebbe stata una buona scelta in quanto unica per l'operazione, e anche se la chiave String finale è dichiarata all'interno del metodo, stavo pensando che ogni thread avrebbe ottenuto un riferimento a lo stesso oggetto e quindi la sincronizzazione su questo singolo oggetto.

Cosa sto facendo di sbagliato qui?

Aggiorna : dopo aver esaminato ulteriormente i log, sembrano metodi con la stessa logica di sincronizzazione in cui la chiave è sempre la stessa, come 

final String key = "blah";
...
synchronized(key) { ...

non presentano lo stesso problema di concorrenza: solo un thread alla volta sta entrando nel blocco.

Aggiornamento 2 : grazie a tutti per l'aiuto! Ho accettato la prima risposta su intern () ing Strings, che ha risolto il mio problema iniziale - in cui più thread stavano inserendo blocchi sincronizzati dove pensavo che non avrebbero dovuto, perché la chiave ha avuto lo stesso valore.

Come altri hanno sottolineato, l'uso di intern () a tale scopo e la sincronizzazione su quelle stringhe risulta davvero una cattiva idea - quando si eseguono i test JMeter contro la webapp per simulare l'atteso carico, ho visto la dimensione dell'heap usato crescere fino a quasi 1 GB in poco meno di 20 minuti.

Attualmente sto usando la semplice soluzione di sincronizzare l'intero metodo, ma davvero mi piacciono gli esempi di codice forniti da martinprobst e MBCook, ma dal momento che ho circa 7 simili getData ( ) attualmente in questa classe (dal momento che necessita di circa 7 diversi dati da un servizio web), non volevo aggiungere una logica quasi duplicata su come ottenere e rilasciare blocchi per ciascun metodo. Ma queste sono sicuramente informazioni molto, molto preziose per un utilizzo futuro. Penso che queste siano in definitiva le risposte corrette sul modo migliore per eseguire un'operazione come questa thread-safe, e darei più voti a queste risposte se potessi!

È stato utile?

Soluzione

Senza mettere completamente il mio cervello in marcia, da una rapida scansione di ciò che dici sembra che tu debba internare () le tue stringhe: 

final String firstkey = "Data-" + email;
final String key = firstkey.intern();

Due stringhe con lo stesso valore non sono necessariamente lo stesso oggetto.

Si noti che ciò potrebbe introdurre un nuovo punto di contesa, poiché nel profondo della VM, intern () potrebbe dover acquisire un blocco. Non ho idea di che aspetto abbiano le VM moderne in quest'area, ma si spera che siano diabolicamente ottimizzate.

Suppongo che tu sappia che StaticCache deve ancora essere sicuro per i thread. Ma la contesa dovrebbe essere minuscola rispetto a quella che avresti se ti stessi bloccando nella cache piuttosto che solo la chiave durante la chiamata getSomeDataForEmail.

Risposta all'aggiornamento della domanda :

Penso che sia perché una stringa letterale produce sempre lo stesso oggetto. Dave Costa sottolinea in un commento che è persino meglio di così: un letterale produce sempre la rappresentazione canonica. Quindi tutti i letterali String con lo stesso valore in qualsiasi punto del programma produrrebbero lo stesso oggetto.

Modifica

Altri hanno sottolineato che la sincronizzazione su stringhe interne è in realtà una pessima idea - in parte perché la creazione di stringhe di intern è autorizzata a farli esistere per sempre, e in parte perché se più di un bit di il codice in qualsiasi punto del programma si sincronizza su stringhe interne, si hanno dipendenze tra quei bit di codice e la prevenzione di deadlock o altri bug potrebbe essere impossibile.

Strategie per evitarlo memorizzando un oggetto lock per stringa chiave vengono sviluppate in altre risposte mentre digito.

Ecco un'alternativa: usa ancora un blocco singolare, ma sappiamo che avremo comunque bisogno di uno di quelli per la cache e stavi parlando di 50 thread, non di 5000, quindi potrebbe non essere fatale. Suppongo anche che il collo di bottiglia delle prestazioni qui sia un rallentamento dell'I / O di blocco in DoSlowThing () che trarrà quindi enorme vantaggio dal non essere serializzato. Se questo non è il collo di bottiglia, allora:

  • Se la CPU è occupata, questo approccio potrebbe non essere sufficiente e sarà necessario un altro approccio.
  • Se la CPU non è occupata e l'accesso al server non è un collo di bottiglia, questo approccio è eccessivo e potresti anche dimenticare sia questo che il blocco per chiave, mettere un grande sincronizzato (StaticCache) attorno all'intera operazione e fallo nel modo più semplice.

Ovviamente questo approccio deve essere testato per la scalabilità prima dell'uso - non garantisco nulla.

Questo codice NON richiede che StaticCache sia sincronizzato o sicuro per i thread. Questo deve essere rivisitato se qualsiasi altro codice (ad esempio la pulizia programmata di vecchi dati) tocca mai la cache.

IN_PROGRESS è un valore fittizio, non esattamente pulito, ma il codice è semplice e consente di risparmiare con due hashtabili. Non gestisce InterruptedException perché non so cosa vuole fare la tua app in quel caso. Inoltre, se DoSlowThing () fallisce costantemente per una determinata chiave, questo codice così com'è non è esattamente elegante, poiché ogni thread attraverso lo riproverà. Dal momento che non so quali siano i criteri di fallimento e se siano suscettibili di essere temporanei o permanenti, non lo gestisco neanche, mi assicuro solo che i thread non si blocchino per sempre. In pratica potresti voler inserire un valore di dati nella cache che indica 'non disponibile', forse con un motivo e un timeout per quando riprovare. 

// do not attempt double-check locking here. I mean it.
synchronized(StaticObject) {
    data = StaticCache.get(key);
    while (data == IN_PROGRESS) {
        // another thread is getting the data
        StaticObject.wait();
        data = StaticCache.get(key);
    }
    if (data == null) {
        // we must get the data
        StaticCache.put(key, IN_PROGRESS, TIME_MAX_VALUE);
    }
}
if (data == null) {
    // we must get the data
    try {
        data = server.DoSlowThing(key);
    } finally {
        synchronized(StaticObject) {
            // WARNING: failure here is fatal, and must be allowed to terminate
            // the app or else waiters will be left forever. Choose a suitable
            // collection type in which replacing the value for a key is guaranteed.
            StaticCache.put(key, data, CURRENT_TIME);
            StaticObject.notifyAll();
        }
    }
}

Ogni volta che qualcosa viene aggiunto alla cache, tutti i thread si riattivano e controllano la cache (indipendentemente dalla chiave che stanno cercando), quindi è possibile ottenere prestazioni migliori con algoritmi meno controversi. Tuttavia, gran parte di tale lavoro avrà luogo durante il tuo copioso tempo di inattività della CPU in blocco sull'I / O, quindi potrebbe non essere un problema.

Questo codice potrebbe essere condiviso per l'uso con più cache, se si definiscono astrazioni adatte per la cache e il relativo blocco associato, i dati che restituisce, il manichino IN_PROGRESS e l'operazione lenta da eseguire. Inserire l'intera cosa in un metodo nella cache potrebbe non essere una cattiva idea.

Altri suggerimenti

La sincronizzazione su una stringa internata potrebbe non essere affatto una buona idea: internandola, la stringa si trasforma in un oggetto globale e se si sincronizza sulle stesse stringhe internate in parti diverse dell'applicazione, è possibile ottenere problemi di sincronizzazione davvero strani e sostanzialmente indistruttibili come deadlock. Potrebbe sembrare improbabile, ma quando succede sei davvero fregato. Come regola generale, sincronizza sempre e solo su un oggetto locale in cui sei assolutamente sicuro che nessun codice esterno al tuo modulo potrebbe bloccarlo.

Nel tuo caso, puoi utilizzare una hashtable sincronizzata per archiviare oggetti di blocco per le tue chiavi.

per esempio:. 

Object data = StaticCache.get(key, ...);
if (data == null) {
  Object lock = lockTable.get(key);
  if (lock == null) {
    // we're the only one looking for this
    lock = new Object();
    synchronized(lock) {
      lockTable.put(key, lock);
      // get stuff
      lockTable.remove(key);
    }
  } else {
    synchronized(lock) {
      // just to wait for the updater
    }
    data = StaticCache.get(key);
  }
} else {
  // use from cache
}

Questo codice ha una condizione di competizione, in cui due thread potrebbero mettere un oggetto nella tabella di blocco uno dopo l'altro. Questo non dovrebbe comunque essere un problema, perché in questo caso hai solo un altro thread che chiama il servizio web e aggiorna la cache, il che non dovrebbe essere un problema.

Se stai invalidando la cache dopo un po 'di tempo, dovresti verificare se i dati sono nuovamente nulli dopo averli recuperati dalla cache, nel lock! = null case.

In alternativa, e molto più semplice, puoi rendere sincronizzato l'intero metodo di ricerca della cache (" getSomeDataByEmail "). Ciò significa che tutti i thread devono sincronizzarsi quando accedono alla cache, il che potrebbe essere un problema di prestazioni. Ma come sempre, prova prima questa semplice soluzione e vedi se è davvero un problema! In molti casi non dovrebbe essere così, poiché probabilmente impiegherai molto più tempo a elaborare il risultato che a sincronizzare.

Le stringhe sono non buone candidate per la sincronizzazione. Se devi eseguire la sincronizzazione su un ID stringa, puoi farlo utilizzando la stringa per creare un mutex (vedi " sincronizzazione su un ID "). La validità del costo di tale algoritmo dipende dal fatto che invocare il tuo servizio comporti un I / O significativo.

Inoltre:

  • Spero che i metodi StaticCache.get () e set () siano sicuri per il thread.
  • String.intern () ha un costo (uno che varia tra le implementazioni di VM) e dovrebbe essere usato con cura.

Altri hanno suggerito di internare le stringhe e funzionerà.

Il problema è che Java deve mantenere le stringhe internate. Mi è stato detto che lo fa anche se non hai un riferimento perché il valore deve essere lo stesso la prossima volta che qualcuno usa quella stringa. Ciò significa che internare tutte le stringhe potrebbe iniziare a consumare memoria, che con il carico che stai descrivendo potrebbe essere un grosso problema.

Ho visto due soluzioni a questo:

Potresti sincronizzarti su un altro oggetto

Invece dell'email, crea un oggetto che contiene l'e-mail (ad esempio l'oggetto Utente) che contiene il valore dell'email come variabile. Se hai già un altro oggetto che rappresenta la persona (supponiamo che tu abbia già estratto qualcosa dal DB in base alla sua e-mail) potresti usarlo. Implementando il metodo equals e il metodo hashcode puoi assicurarti che Java consideri gli oggetti uguali quando fai un cache.contains () statico per scoprire se i dati sono già nella cache (dovrai sincronizzarti nella cache ).

In realtà, potresti tenere una seconda mappa per gli oggetti da bloccare. Qualcosa del genere: 

Map<String, Object> emailLocks = new HashMap<String, Object>();

Object lock = null;

synchronized (emailLocks) {
    lock = emailLocks.get(emailAddress);

    if (lock == null) {
        lock = new Object();
        emailLocks.put(emailAddress, lock);
    }
}

synchronized (lock) {
    // See if this email is in the cache
    // If so, serve that
    // If not, generate the data

    // Since each of this person's threads synchronizes on this, they won't run
    // over eachother. Since this lock is only for this person, it won't effect
    // other people. The other synchronized block (on emailLocks) is small enough
    // it shouldn't cause a performance problem.
}

Ciò impedirà 15 recuperi sullo stesso indirizzo e-mail in uno. Avrai bisogno di qualcosa per impedire che troppe voci finiscano nella mappa emailLocks. Usando LRUMap di Apache Commons sarebbe fallo.

Questo richiederà alcune modifiche, ma potrebbe risolvere il tuo problema.

Utilizza una chiave diversa

Se sei disposto a tollerare possibili errori (non so quanto sia importante) potresti usare l'hashcode della String come chiave. gli ints non devono essere internati.

Riepilogo

Spero che questo aiuti. Il threading è divertente, vero? Puoi anche utilizzare la sessione per impostare un valore che significa " Sto già lavorando per trovare questo " e controlla che per vedere se il secondo (terzo, nth) thread deve tentare di creare o semplicemente attendere che il risultato venga visualizzato nella cache. Immagino di avere tre suggerimenti.

Puoi utilizzare le utility di concorrenza 1.5 per fornire una cache progettata per consentire l'accesso simultaneo multiplo e un singolo punto di aggiunta (ovvero solo un thread che esegue il costoso oggetto "creazione"): 

 private ConcurrentMap<String, Future<SomeData[]> cache;
 private SomeData[] getSomeDataByEmail(final WebServiceInterface service, final String email) throws Exception {

  final String key = "Data-" + email;
  Callable<SomeData[]> call = new Callable<SomeData[]>() {
      public SomeData[] call() {
          return service.getSomeDataForEmail(email);
      }
  }
  FutureTask<SomeData[]> ft; ;
  Future<SomeData[]> f = cache.putIfAbsent(key, ft= new FutureTask<SomeData[]>(call)); //atomic
  if (f == null) { //this means that the cache had no mapping for the key
      f = ft;
      ft.run();
  }
  return f.get(); //wait on the result being available if it is being calculated in another thread
}

Ovviamente, questo non gestisce le eccezioni come si vorrebbe, e la cache non ha lo sfratto incorporato. Forse potresti usarlo come base per cambiare la tua classe StaticCache,

Utilizza un framework di cache decente come ehcache .

L'implementazione di una buona cache non è facile come alcuni credono.

Per quanto riguarda il commento che String.intern () è una fonte di perdite di memoria, questo in realtà non è vero. Le stringhe internate sono raccolte, potrebbe richiedere più tempo perché su alcune JVM (SUN) sono memorizzate nello spazio Perm che viene toccato solo dai GC completi.

Ecco una breve soluzione sicura per Java 8 che utilizza una mappa di oggetti di blocco dedicati per la sincronizzazione: 

private static final Map<String, Object> keyLocks = new ConcurrentHashMap<>();

private SomeData[] getSomeDataByEmail(WebServiceInterface service, String email) {
    final String key = "Data-" + email;
    synchronized (keyLocks.computeIfAbsent(key, k -> new Object())) {
        SomeData[] data = StaticCache.get(key);
        if (data == null) {
            data = service.getSomeDataForEmail(email);
            StaticCache.set(key, data);
        }
    }
    return data;
}

Ha uno svantaggio che chiavi e oggetti di blocco manterrebbero per sempre nella mappa.

Questo può essere aggirato in questo modo: 

private SomeData[] getSomeDataByEmail(WebServiceInterface service, String email) {
    final String key = "Data-" + email;
    synchronized (keyLocks.computeIfAbsent(key, k -> new Object())) {
        try {
            SomeData[] data = StaticCache.get(key);
            if (data == null) {
                data = service.getSomeDataForEmail(email);
                StaticCache.set(key, data);
            }
        } finally {
            keyLocks.remove(key); // vulnerable to race-conditions
        }
    }
    return data;
}

Ma poi le chiavi più diffuse verranno costantemente reinserite nella mappa con gli oggetti bloccati riallocati.

Aggiorna : e questo lascia la possibilità di condizioni di gara quando due thread entrerebbero contemporaneamente nella sezione sincronizzata per la stessa chiave ma con blocchi diversi.

Quindi potrebbe essere più sicuro ed efficiente utilizzare scadenza Guava Cache : 

private static final LoadingCache<String, Object> keyLocks = CacheBuilder.newBuilder()
        .expireAfterAccess(10, TimeUnit.MINUTES) // max lock time ever expected
        .build(CacheLoader.from(Object::new));

private SomeData[] getSomeDataByEmail(WebServiceInterface service, String email) {
    final String key = "Data-" + email;
    synchronized (keyLocks.getUnchecked(key)) {
        SomeData[] data = StaticCache.get(key);
        if (data == null) {
            data = service.getSomeDataForEmail(email);
            StaticCache.set(key, data);
        }
    }
    return data;
}

Si noti che qui si presume che StaticCache sia thread-safe e non soffrirebbe di letture e scritture simultanee per chiavi diverse.

Il tuo problema principale non è solo che potrebbero esserci più istanze di String con lo stesso valore. Il problema principale è che è necessario disporre di un solo monitor su cui eseguire la sincronizzazione per accedere all'oggetto StaticCache. Altrimenti più thread potrebbero finire per modificare contemporaneamente StaticCache (anche se con chiavi diverse), che molto probabilmente non supporta modifiche simultanee.

La chiamata: 

   final String key = "Data-" + email;

crea un nuovo oggetto ogni volta che viene chiamato il metodo. Poiché quell'oggetto è ciò che usi per bloccare e ogni chiamata a questo metodo crea un nuovo oggetto, non stai davvero sincronizzando l'accesso alla mappa in base alla chiave.

Questo spiega ulteriormente la tua modifica. Quando hai una stringa statica, allora funzionerà.

L'uso di intern () risolve il problema, poiché restituisce la stringa da un pool interno mantenuto dalla classe String, ciò garantisce che se due stringhe sono uguali, verrà utilizzata quella nel pool. Vedere

http: / /java.sun.com/j2se/1.4.2/docs/api/java/lang/String.html#intern ()

Questa domanda mi sembra un po 'troppo ampia, e quindi ha suscitato un insieme altrettanto ampio di risposte. Quindi proverò a rispondere alla domanda Sono stato reindirizzato da, purtroppo che uno è stato chiuso come duplicato. 

public class ValueLock<T> {

    private Lock lock = new ReentrantLock();
    private Map<T, Condition> conditions  = new HashMap<T, Condition>();

    public void lock(T t){
        lock.lock();
        try {
            while (conditions.containsKey(t)){
                conditions.get(t).awaitUninterruptibly();
            }
            conditions.put(t, lock.newCondition());
        } finally {
            lock.unlock();
        }
    }

    public void unlock(T t){
        lock.lock();
        try {
            Condition condition = conditions.get(t);
            if (condition == null)
                throw new IllegalStateException();// possibly an attempt to release what wasn't acquired
            conditions.remove(t);
            condition.signalAll();
        } finally {
            lock.unlock();
        }
    }

Dopo l'operazione (esterna) blocco viene acquisito il blocco (interno) per ottenere un accesso esclusivo alla mappa per un breve periodo e se l'oggetto corrispondente è già nella mappa, l'attuale il thread attenderà, altrimenti inserirà la nuova Condizione sulla mappa, rilascerà il blocco (interno) e procederà, e il blocco (esterno) è considerato ottenuto. L'operazione (esterna) unlock , acquisendo prima un blocco (interno), segnalerà Condition e quindi rimuoverà l'oggetto dalla mappa.

La classe non utilizza la versione simultanea di Mappa , poiché ogni accesso ad essa è protetto da un singolo blocco (interno).

Nota, la semantica del metodo lock () di questa classe è diversa da quella di ReentrantLock.lock () , la ripetuta lock () invocazioni senza unlock () associato bloccheranno il thread corrente indefinitamente.

Un esempio di utilizzo che potrebbe essere applicabile alla situazione, l'OP descritto 

    ValueLock<String> lock = new ValueLock<String>();
    // ... share the lock   
    String email = "...";
    try {
        lock.lock(email);
        //... 
    } finally {
        lock.unlock(email);
    }

È piuttosto tardi, ma qui è presentato un sacco di codice errato.

In questo esempio: 

private SomeData[] getSomeDataByEmail(WebServiceInterface service, String email) {


  SomeData[] data = null;
  final String key = "Data-" + email;

  synchronized(key) {      
    data =(SomeData[]) StaticCache.get(key);

    if (data == null) {
        data = service.getSomeDataForEmail(email);
        StaticCache.set(key, data, CACHE_TIME);
    }
    else {
      logger.debug("getSomeDataForEmail: using cached object");
    }
  }

  return data;
}

La sincronizzazione ha un ambito errato. Per una cache statica che supporta un'API get / put, dovrebbe esserci almeno la sincronizzazione attorno alle operazioni di tipo get e getIfAbsentPut, per un accesso sicuro alla cache. L'ambito della sincronizzazione sarà la cache stessa.

Se è necessario apportare aggiornamenti agli elementi di dati stessi, ciò aggiunge un ulteriore livello di sincronizzazione, che dovrebbe essere sui singoli elementi di dati.

SynchronizedMap può essere utilizzato al posto della sincronizzazione esplicita, ma è comunque necessario prestare attenzione. Se vengono utilizzate le API errate (get and put anziché putIfAbsent), le operazioni non avranno la sincronizzazione necessaria, nonostante l'uso della mappa sincronizzata. Notare le complicazioni introdotte dall'uso di putIfAbsent: in entrambi i casi, il valore put deve essere calcolato anche nei casi in cui non è necessario (perché il put non può sapere se il valore put è necessario fino a quando non viene esaminato il contenuto della cache) o richiede un attento uso della delega (diciamo, usando Future, che funziona, ma è in qualche modo incompatibile; vedi sotto), dove il valore put viene ottenuto su richiesta se necessario.

L'uso del futuro è possibile, ma sembra piuttosto imbarazzante e forse un po 'troppo ingegneristico. L'API Future è al centro delle operazioni asincrone, in particolare delle operazioni che potrebbero non essere completate immediatamente. Coinvolgere il futuro molto probabilmente aggiunge uno strato di creazione di thread - extra probabilmente complicazioni inutili.

Il problema principale dell'utilizzo di Future per questo tipo di operazione è che Future si lega intrinsecamente al multi-threading. L'uso di Future quando non è necessario un nuovo thread significa ignorare molti dei macchinari di Future, rendendolo un'API eccessivamente pesante per questo uso.

Perché non rendere semplicemente una pagina html statica che viene servita all'utente e rigenerata ogni x minuti?

Suggerirei anche di eliminare completamente la concatenazione di stringhe se non ne hai bisogno. 

final String key = "Data-" + email;

Ci sono altre cose / tipi di oggetti nella cache che usano l'indirizzo e-mail di cui hai bisogno di quel dato aggiuntivo " Dati & " all'inizio della chiave?

in caso contrario, lo farei solo 

final String key = email;

ed eviti anche tutta quella creazione di stringhe extra.

sincronizzazione in altro modo sull'oggetto stringa: 

String cacheKey = ...;

    Object obj = cache.get(cacheKey)

    if(obj==null){
    synchronized (Integer.valueOf(Math.abs(cacheKey.hashCode()) % 127)){
          obj = cache.get(cacheKey)
         if(obj==null){
             //some cal obtain obj value,and put into cache
        }
    }
}

Nel caso in cui altri abbiano un problema simile, il seguente codice funziona, per quanto ne so: 

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Supplier;

public class KeySynchronizer<T> {

    private Map<T, CounterLock> locks = new ConcurrentHashMap<>();

    public <U> U synchronize(T key, Supplier<U> supplier) {
        CounterLock lock = locks.compute(key, (k, v) -> 
                v == null ? new CounterLock() : v.increment());
        synchronized (lock) {
            try {
                return supplier.get();
            } finally {
                if (lock.decrement() == 0) {
                    // Only removes if key still points to the same value,
                    // to avoid issue described below.
                    locks.remove(key, lock);
                }
            }
        }
    }

    private static final class CounterLock {

        private AtomicInteger remaining = new AtomicInteger(1);

        private CounterLock increment() {
            // Returning a new CounterLock object if remaining = 0 to ensure that
            // the lock is not removed in step 5 of the following execution sequence:
            // 1) Thread 1 obtains a new CounterLock object from locks.compute (after evaluating "v == null" to true)
            // 2) Thread 2 evaluates "v == null" to false in locks.compute
            // 3) Thread 1 calls lock.decrement() which sets remaining = 0
            // 4) Thread 2 calls v.increment() in locks.compute
            // 5) Thread 1 calls locks.remove(key, lock)
            return remaining.getAndIncrement() == 0 ? new CounterLock() : this;
        }

        private int decrement() {
            return remaining.decrementAndGet();
        }
    }
}

Nel caso del PO, sarebbe usato in questo modo: 

private KeySynchronizer<String> keySynchronizer = new KeySynchronizer<>();

private SomeData[] getSomeDataByEmail(WebServiceInterface service, String email) {
    String key = "Data-" + email;
    return keySynchronizer.synchronize(key, () -> {
        SomeData[] existing = (SomeData[]) StaticCache.get(key);
        if (existing == null) {
            SomeData[] data = service.getSomeDataForEmail(email);
            StaticCache.set(key, data, CACHE_TIME);
            return data;
        }
        logger.debug("getSomeDataForEmail: using cached object");
        return existing;
    });
}

Se nulla deve essere restituito dal codice sincronizzato, il metodo di sincronizzazione può essere scritto in questo modo: 

public void synchronize(T key, Runnable runnable) {
    CounterLock lock = locks.compute(key, (k, v) -> 
            v == null ? new CounterLock() : v.increment());
    synchronized (lock) {
        try {
            runnable.run();
        } finally {
            if (lock.decrement() == 0) {
                // Only removes if key still points to the same value,
                // to avoid issue described below.
                locks.remove(key, lock);
            }
        }
    }
}

Ho aggiunto una piccola classe di blocco che può bloccare / sincronizzare qualsiasi chiave, comprese le stringhe.

Vedi l'implementazione per Java 8, Java 6 e un piccolo test.

Java 8: 

public class DynamicKeyLock<T> implements Lock
{
    private final static ConcurrentHashMap<Object, LockAndCounter> locksMap = new ConcurrentHashMap<>();

    private final T key;

    public DynamicKeyLock(T lockKey)
    {
        this.key = lockKey;
    }

    private static class LockAndCounter
    {
        private final Lock lock = new ReentrantLock();
        private final AtomicInteger counter = new AtomicInteger(0);
    }

    private LockAndCounter getLock()
    {
        return locksMap.compute(key, (key, lockAndCounterInner) ->
        {
            if (lockAndCounterInner == null) {
                lockAndCounterInner = new LockAndCounter();
            }
            lockAndCounterInner.counter.incrementAndGet();
            return lockAndCounterInner;
        });
    }

    private void cleanupLock(LockAndCounter lockAndCounterOuter)
    {
        if (lockAndCounterOuter.counter.decrementAndGet() == 0)
        {
            locksMap.compute(key, (key, lockAndCounterInner) ->
            {
                if (lockAndCounterInner == null || lockAndCounterInner.counter.get() == 0) {
                    return null;
                }
                return lockAndCounterInner;
            });
        }
    }

    @Override
    public void lock()
    {
        LockAndCounter lockAndCounter = getLock();

        lockAndCounter.lock.lock();
    }

    @Override
    public void unlock()
    {
        LockAndCounter lockAndCounter = locksMap.get(key);
        lockAndCounter.lock.unlock();

        cleanupLock(lockAndCounter);
    }


    @Override
    public void lockInterruptibly() throws InterruptedException
    {
        LockAndCounter lockAndCounter = getLock();

        try
        {
            lockAndCounter.lock.lockInterruptibly();
        }
        catch (InterruptedException e)
        {
            cleanupLock(lockAndCounter);
            throw e;
        }
    }

    @Override
    public boolean tryLock()
    {
        LockAndCounter lockAndCounter = getLock();

        boolean acquired = lockAndCounter.lock.tryLock();

        if (!acquired)
        {
            cleanupLock(lockAndCounter);
        }

        return acquired;
    }

    @Override
    public boolean tryLock(long time, TimeUnit unit) throws InterruptedException
    {
        LockAndCounter lockAndCounter = getLock();

        boolean acquired;
        try
        {
            acquired = lockAndCounter.lock.tryLock(time, unit);
        }
        catch (InterruptedException e)
        {
            cleanupLock(lockAndCounter);
            throw e;
        }

        if (!acquired)
        {
            cleanupLock(lockAndCounter);
        }

        return acquired;
    }

    @Override
    public Condition newCondition()
    {
        LockAndCounter lockAndCounter = locksMap.get(key);

        return lockAndCounter.lock.newCondition();
    }
}

Java 6:

DynamicKeyLock di classe pubblica implementa Lock     {         private private static ConcurrentHashMap locksMap = new ConcurrentHashMap ();         chiave T finale privata; 

    public DynamicKeyLock(T lockKey) {
        this.key = lockKey;
    }

    private static class LockAndCounter {
        private final Lock lock = new ReentrantLock();
        private final AtomicInteger counter = new AtomicInteger(0);
    }

    private LockAndCounter getLock()
    {
        while (true) // Try to init lock
        {
            LockAndCounter lockAndCounter = locksMap.get(key);

            if (lockAndCounter == null)
            {
                LockAndCounter newLock = new LockAndCounter();
                lockAndCounter = locksMap.putIfAbsent(key, newLock);

                if (lockAndCounter == null)
                {
                    lockAndCounter = newLock;
                }
            }

            lockAndCounter.counter.incrementAndGet();

            synchronized (lockAndCounter)
            {
                LockAndCounter lastLockAndCounter = locksMap.get(key);
                if (lockAndCounter == lastLockAndCounter)
                {
                    return lockAndCounter;
                }
                // else some other thread beat us to it, thus try again.
            }
        }
    }

    private void cleanupLock(LockAndCounter lockAndCounter)
    {
        if (lockAndCounter.counter.decrementAndGet() == 0)
        {
            synchronized (lockAndCounter)
            {
                if (lockAndCounter.counter.get() == 0)
                {
                    locksMap.remove(key);
                }
            }
        }
    }

    @Override
    public void lock()
    {
        LockAndCounter lockAndCounter = getLock();

        lockAndCounter.lock.lock();
    }

    @Override
    public void unlock()
    {
        LockAndCounter lockAndCounter = locksMap.get(key);
        lockAndCounter.lock.unlock();

        cleanupLock(lockAndCounter);
    }


    @Override
    public void lockInterruptibly() throws InterruptedException
    {
        LockAndCounter lockAndCounter = getLock();

        try
        {
            lockAndCounter.lock.lockInterruptibly();
        }
        catch (InterruptedException e)
        {
            cleanupLock(lockAndCounter);
            throw e;
        }
    }

    @Override
    public boolean tryLock()
    {
        LockAndCounter lockAndCounter = getLock();

        boolean acquired = lockAndCounter.lock.tryLock();

        if (!acquired)
        {
            cleanupLock(lockAndCounter);
        }

        return acquired;
    }

    @Override
    public boolean tryLock(long time, TimeUnit unit) throws InterruptedException
    {
        LockAndCounter lockAndCounter = getLock();

        boolean acquired;
        try
        {
            acquired = lockAndCounter.lock.tryLock(time, unit);
        }
        catch (InterruptedException e)
        {
            cleanupLock(lockAndCounter);
            throw e;
        }

        if (!acquired)
        {
            cleanupLock(lockAndCounter);
        }

        return acquired;
    }

    @Override
    public Condition newCondition()
    {
        LockAndCounter lockAndCounter = locksMap.get(key);

        return lockAndCounter.lock.newCondition();
    }
}

Prova: 

public class DynamicKeyLockTest
{
    @Test
    public void testDifferentKeysDontLock() throws InterruptedException
    {
        DynamicKeyLock<Object> lock = new DynamicKeyLock<>(new Object());
        lock.lock();
        AtomicBoolean anotherThreadWasExecuted = new AtomicBoolean(false);
        try
        {
            new Thread(() ->
            {
                DynamicKeyLock<Object> anotherLock = new DynamicKeyLock<>(new Object());
                anotherLock.lock();
                try
                {
                    anotherThreadWasExecuted.set(true);
                }
                finally
                {
                    anotherLock.unlock();
                }
            }).start();
            Thread.sleep(100);
        }
        finally
        {
            Assert.assertTrue(anotherThreadWasExecuted.get());
            lock.unlock();
        }
    }

    @Test
    public void testSameKeysLock() throws InterruptedException
    {
        Object key = new Object();
        DynamicKeyLock<Object> lock = new DynamicKeyLock<>(key);
        lock.lock();
        AtomicBoolean anotherThreadWasExecuted = new AtomicBoolean(false);
        try
        {
            new Thread(() ->
            {
                DynamicKeyLock<Object> anotherLock = new DynamicKeyLock<>(key);
                anotherLock.lock();
                try
                {
                    anotherThreadWasExecuted.set(true);
                }
                finally
                {
                    anotherLock.unlock();
                }
            }).start();
            Thread.sleep(100);
        }
        finally
        {
            Assert.assertFalse(anotherThreadWasExecuted.get());
            lock.unlock();
        }
    }
}

Nel tuo caso potresti usare qualcosa del genere (questo non perde alcuna memoria): 

private Synchronizer<String> synchronizer = new Synchronizer();

private SomeData[] getSomeDataByEmail(WebServiceInterface service, String email) {
    String key = "Data-" + email;

    return synchronizer.synchronizeOn(key, () -> {

        SomeData[] data = (SomeData[]) StaticCache.get(key);
        if (data == null) {
            data = service.getSomeDataForEmail(email);
            StaticCache.set(key, data, CACHE_TIME);
        } else {
          logger.debug("getSomeDataForEmail: using cached object");
        }
        return data;

    });
}

per usarlo basta aggiungere una dipendenza: 

compile 'com.github.matejtymes:javafixes:1.3.0'

Puoi tranquillamente usare String.intern per la sincronizzazione se puoi ragionevolmente garantire che il valore della stringa sia univoco nel tuo sistema. Gli UUIDI sono un buon modo per avvicinarsi a questo. Puoi associare un UUID alla tua chiave di stringa effettiva, tramite una cache, una mappa, o magari anche memorizzare l'UUID come campo sul tuo oggetto entità. 

    @Service   
    public class MySyncService{

      public Map<String, String> lockMap=new HashMap<String, String>();

      public void syncMethod(String email) {

        String lock = lockMap.get(email);
        if(lock==null) {
            lock = UUID.randomUUID().toString();
            lockMap.put(email, lock);
        }   

        synchronized(lock.intern()) {
                //do your sync code here
        }
    }
Autorizzato sotto: CC-BY-SA insieme a attribuzione
Non affiliato a StackOverflow
scroll top