Fabbrica di oggetti singleton: questo codice è sicuro per il thread?

Question

Ho un'interfaccia comune per un numero di implementazioni singleton. L'interfaccia definisce il metodo di inizializzazione che può generare un'eccezione controllata.

Ho bisogno di un factory che restituisca le implementazioni singleton memorizzate nella cache su richiesta, e mi chiedo se il seguente approccio sia thread-safe?

Update1: Si prega di non qualsiasi proposta di 3a in parte le biblioteche, in quanto richiede per ottenere l'autorizzazione legale a causa di possibili problemi di licenza :-)

UPDATE2: questo codice sarà probabilmente da utilizzare in Ambiente EJB, quindi è preferibile non generare thread aggiuntivi o usare cose del genere.

interface Singleton 
{ 
    void init() throws SingletonException; 
} 

public class SingletonFactory 
{ 
    private static ConcurrentMap<String, AtomicReference<? extends Singleton>> CACHE = 
     new ConcurrentHashMap<String, AtomicReference<? extends Singleton>>(); 

    public static <T extends Singleton> T getSingletonInstance(Class<T> clazz) 
     throws SingletonException 
    { 
     String key = clazz.getName(); 
     if (CACHE.containsKey(key)) 
     { 
      return readEventually(key); 
     } 

     AtomicReference<T> ref = new AtomicReference<T>(null); 
     if (CACHE.putIfAbsent(key, ref) == null) 
     { 
      try 
      { 
       T instance = clazz.newInstance(); 
       instance.init(); 
       ref.set(instance); // ----- (1) ----- 
       return instance; 
      } 
      catch (Exception e) 
      { 
       throw new SingletonException(e); 
      } 
     } 

     return readEventually(key); 
    } 

    @SuppressWarnings("unchecked") 
    private static <T extends Singleton> T readEventually(String key) 
    { 
     T instance = null; 
     AtomicReference<T> ref = (AtomicReference<T>) CACHE.get(key); 
     do 
     { 
      instance = ref.get(); // ----- (2) ----- 
     } 
     while (instance == null); 
     return instance; 
    } 
} 

Non sono del tutto sicuro di linee (1) e (2). So che l'oggetto di riferimento è dichiarato come campo volatile in AtomicReference, e quindi le modifiche apportate alla riga (1) dovrebbero diventare immediatamente visibili alla riga (2) - ma hanno ancora qualche dubbio ...

Oltre a questo - penso l'uso di ConcurrentHashMap indirizza l'atomicità di inserire una nuova chiave in una cache.

Ragazzi, vedete qualche preoccupazione con questo approccio? Grazie!

PS: so di statica idioma di classe titolare - e io non lo uso a causa di ExceptionInInitializerError (che qualsiasi eccezione generata durante la creazione di un'istanza Singleton è avvolto in) e la successiva NoClassDefFoundError che non sono qualcosa che voglio per la cattura . Invece, vorrei sfruttare il vantaggio dell'eccezione verificata dedicata catturandola e gestendola con garbo piuttosto che analizzare la traccia stack di EIIR o NCDFE.

Answer 1

Considerare l'utilizzo di CacheBuilder di Guava. Per esempio:

private static Cache<Class<? extends Singleton>, Singleton> singletons = CacheBuilder.newBuilder() 
    .build(
     new CacheLoader<Class<? extends Singleton>, Singleton>() { 
     public Singleton load(Class<? extends Singleton> key) throws SingletonException { 
      try { 
      Singleton singleton = key.newInstance(); 
      singleton.init(); 
      return singleton; 
      } 
      catch (SingletonException se) { 
      throw se; 
      } 
      catch (Exception e) { 
      throw new SingletonException(e); 
      } 
     } 
     }); 

public static <T extends Singleton> T getSingletonInstance(Class<T> clazz) { 
    return (T)singletons.get(clazz); 
} 

Nota: questo esempio non è testato e non compilato.

L'implementazione di base di Cache di Guava gestirà tutta la logica della cache e della concorrenza.

Answer 2

Il codice non è generalmente thread-safe perché c'è uno spazio tra il controllo CACHE.containsKey(key) e la chiamata CACHE.putIfAbsent(key, ref). È possibile che due thread chiamino simultaneamente nel metodo (specialmente su sistemi multi-core/processore) ed entrambi eseguono il controllo containsKey(), quindi entrambi tentano di eseguire le operazioni di inserimento e creazione.

Proteggerei quell'esecuzione del metodo getSingletonInstnace() utilizzando un blocco o eseguendo la sincronizzazione su un monitor di qualche tipo.

Answer 3

Avere tutte queste cose concomitanti/atomiche causerebbe problemi maggiori di blocco che mettere semplicemente

synchronized(clazz){} 

blocchi intorno il getter. I riferimenti atomici sono per i riferimenti che sono AGGIORNATI e non si desidera la collisione. Qui hai un singolo scrittore, quindi non ti interessa.

Si potrebbe ottimizzare ulteriormente avendo un HashMap, e solo se v'è una miss, utilizzare il blocco sincronizzato:

public static <T> T get(Class<T> cls){ 
    // No lock try 
    T ref = cache.get(cls); 
    if(ref != null){ 
     return ref; 
    } 
    // Miss, so use create lock 
    synchronized(cls){ // singletons are double created 
     synchronized(cache){ // Prevent table rebuild/transfer contentions -- RARE 
      // Double check create if lock backed up 
      ref = cache.get(cls); 
      if(ref == null){ 
       ref = cls.newInstance(); 
       cache.put(cls,ref); 
      } 
      return ref; 
     } 
    } 
} 

Answer 4

Questo appare come avrebbe funzionato anche se potrei prendere in considerazione una sorta di sonno, se ancora un nanosecondo o qualcosa durante il test per il riferimento da impostare. Il ciclo di test di spin sarà estremamente costoso.

Inoltre, vorrei prendere in considerazione il miglioramento del codice passando il AtomicReference-readEventually() in modo da poter evitare la condizione containsKey() e poi putIfAbsent() gara. Quindi il codice sarebbe:

AtomicReference<T> ref = (AtomicReference<T>) CACHE.get(key); 
if (ref != null) { 
    return readEventually(ref); 
} 

AtomicReference<T> newRef = new AtomicReference<T>(null); 
AtomicReference<T> oldRef = CACHE.putIfAbsent(key, newRef); 
if (oldRef != null) { 
    return readEventually(oldRef); 
} 
... 

Answer 5

Siete andati a un sacco di lavoro per evitare la sincronizzazione, e presumo il motivo per fare questo è per problemi di prestazioni. Hai testato per vedere se questo effettivamente migliora le prestazioni rispetto a una soluzione sincronizzata?

La ragione per cui chiedo è che le classi concorrenti tendono ad essere più lente rispetto a quelle non concorrenti, per non parlare del livello aggiuntivo di reindirizzamento con il riferimento atomico. A seconda del conflitto di thread, una soluzione ingenua sincronizzata potrebbe essere effettivamente più veloce (e più semplice da verificare per la correttezza).

Inoltre, penso che si possa finire con un ciclo infinito quando viene lanciata una SingletonException durante una chiamata a instance.init(). Il motivo è che un thread simultaneo in attesa in readEventually non finirà mai per trovare la sua istanza (poiché è stata generata un'eccezione mentre un altro thread stava inizializzando l'istanza). Forse questo è il comportamento corretto per il tuo caso, o forse vuoi impostare un valore speciale per l'istanza per attivare un'eccezione da gettare nel thread in attesa.

Answer 6

google "Memoizer". in pratica, invece di AtomicReference, utilizzare Future.