Modi per migliorare il codice utilizzando solo le classi fornite da JDK (6)? (Concorrenza, la sicurezza dei thread)

Question

(nota preliminare: forse questo è più adatto per codereview?)

EDITAnswer to self; Credo che questa risposta copra tutti i miei bisogni/problemi, ma naturalmente i commenti sono ben accetti. Domanda originale qui sotto per riferimento.

Ciao,

Di interesse qui è il metodo .getSources(). Questo metodo è pensato per restituire un elenco di sorgenti di messaggi per un dato Locale.

Le due strutture dati centrali a questo metodo sono sources e failedLookups, vedere il codice per i commenti.

Questa particolare attuazione del .getSources() può sempre e solo restituire o un elenco vuoto o un elenco singolo elemento, a seconda del metodo tryAndLookup() cui prototipo è:

protected abstract MessageSource tryAndLookup(final Locale locale) 
    throws IOException; 

Al momento, la logica del codice è il segue:

• se l'origine messaggio per quella locale è già stata cercata, viene restituita;
• da questo punto in poi non è stata eseguita alcuna ricerca; non è tuttavia noto se ciò significhi che un precedente tentativo di ricerca è stato eseguito: controllare il set di ricerche fallite, se la localizzazione da cercare è lì, è un errore noto, restituire la lista vuota;
• ora, la situazione nota è che questa ricerca locale non è mai stata eseguita: eseguitela; a seconda del risultato del metodo tryAndLookup, registrare un esito positivo o negativo.

Ora, perché vado così a lungo: non ho il controllo su tryAndLookup(); potrebbe essere necessario un tempo eccessivo prima di restituire una fonte valida o in mancanza. Di conseguenza, sono riluttante a utilizzare un blocco grossolano o il blocco synchronized.

/** 
* Set of locales known to have failed lookup. 
* 
* <p>When a locale is in this set, it will not attempt to be reloaded.</p> 
*/ 
private final Set<Locale> lookupFailures 
    = new CopyOnWriteArraySet<Locale>(); 

/** 
* Set of message sources successfully looked up 
* 
* <p>When a source is in there, it is there permanently for now.</p> 
*/ 
private final ConcurrentMap<Locale, MessageSource> sources 
    = new ConcurrentHashMap<Locale, MessageSource>(); 

@Override 
protected final List<MessageSource> getSources(final Locale locale) 
{ 
    MessageSource source = sources.get(locale); 

    /* 
    * If found, return it 
    */ 
    if (source != null) 
     return Arrays.asList(source); 

    /* 
    * If it is a registered failure, return the empty list 
    */ 
    if (lookupFailures.contains(locale)) 
     return Collections.emptyList(); 

    /* 
    * OK, try and look it up. On success, register it in the sources map. 
    * On failure, record the failure an return the empty list. 
    */ 
    try { 
     source = tryAndLookup(locale); 
     sources.putIfAbsent(locale, source); 
     // EDIT: fix for bug pinpointed by JBNizet 
     // was: 
     // return Arrays.asList(source); 
     // now is: 
     return Arrays.asList(sources.get(locale)); 
    } catch (IOException ignored) { 
     lookupFailures.add(locale); 
     return Collections.emptyList(); 
    } 
} 

La mia domanda qui è triplice:

• ho volutamente limitarmi alle classi disponibili solo per il JDK; Ho scelto ConcurrentHashMap come implementazione ConcurrentMap e CopyOnWriteArraySet come implementazione thread-safe Set; dalla javadoc, questi sono i migliori che ho trovato. Ma sono stato ingannato da qualche parte?
• I think questo codice è alla fine thread safe; alcuni casi angolari possono portare a ricerche effettuate più volte, ad esempio, ma è per questo che faccio .putIfAbsent(); in questo momento ho sempre usato e mi sono fidato dello LoadingCache di Guava per scopi di cache, e questa è la mia prima incursione fuori da questo territorio; questo codice è effettivamente sicuro?
• questo codice ha un difetto fatale: più di un thread può essere in esecuzione tryAndLookup() allo stesso tempo ... Quali soluzioni esistono per avere questo metodo eseguito solo una volta per ricerca?

Answer 1

risposta ad auto ...

L'algoritmo è stato completamente rinnovato. E 'sulla base della JDK di FutureTask, dal momento che ha due proprietà molto curato:

• suo metodo .run() è asincrona;
• è stato, sia in caso di esito positivo che in caso di errore, ovvero restituisce il risultato già calcolato o l'eccezione generata (inserita in un ExecutionException) su .get().

Questo ha abbastanza un'implicazione sulle strutture dati utilizzate:

• il preesistente lookupFailures set, che ha registrato fallimenti, è scomparso;
• la mappa esistente, che era un ConcurrentMap, è ora sostituito da una pianura Map, custodito da un ReentrantLock; i suoi valori sono ora FutureTask<MessageSource> anziché MessageSource.

Questo ha anche abbastanza un'implicazione l'algoritmo, che è molto più semplice:

• bloccare la carta;
• ricerca l'attività per la locale; se non esisteva in precedenza, crearlo, e .run() esso;
• sblocca la mappa;
• .get() il risultato: restituisce un singolo elenco di elementi in caso di successo, un elenco vuoto in caso di errore.

codice completo, con i commenti:

@ThreadSafe 
public abstract class CachedI18NMessageBundle 
    extends I18NMessageBundle 
{ 
    /** 
    * Map pairing locales with {@link FutureTask} instances returning message 
    * sources 
    * 
    * <p>There will only ever be one task associated with one locale; we 
    * therefore choose to make it a normal map, guarded by a {@link 
    * ReentrantLock}.</p> 
    * 
    * <p>The tasks' {@link FutureTask#run()} method will be executed the first 
    * time this object is initialized.</p> 
    */ 
    @GuardedBy("lock") 
    private final Map<Locale, FutureTask<MessageSource>> lookups 
     = new HashMap<Locale, FutureTask<MessageSource>>(); 

    /** 
    * Lock used to guarantee exclusive access to the {@link #lookups} map 
    */ 
    private final Lock lock = new ReentrantLock(); 

    @Override 
    protected final List<MessageSource> getSources(final Locale locale) 
    { 
     FutureTask<MessageSource> task; 

     /* 
     * Grab an exclusive lock to the lookups map. The lock is held only for 
     * the time necessary to grab the FutureTask or create it (and run it) 
     * if it didn't exist previously. 
     * 
     * We can do this, since FutureTask's .run() is asynchronous. 
     */ 
     lock.lock(); 
     try { 
      /* 
      * Try and see whether there is already a FutureTask associated with 
      * this locale. 
      */ 
      task = lookups.get(locale); 
      if (task == null) { 
       /* 
       * If not, create it and run it. 
       */ 
       task = lookupTask(locale); 
       lookups.put(locale, task); 
       task.run(); 
      } 
     } finally { 
      lock.unlock(); 
     } 

     /* 
     * Try and get the result for this locale; on any failure event (either 
     * an IOException thrown by tryAndLookup() or a thread interrupt), 
     * return an empty list. 
     */ 
     try { 
      return Arrays.asList(task.get()); 
     } catch (ExecutionException ignored) { 
      return Collections.emptyList(); 
     } catch (InterruptedException ignored) { 
      return Collections.emptyList(); 
     } 
    } 

    protected abstract MessageSource tryAndLookup(final Locale locale) 
     throws IOException; 

    @Override 
    public final Builder modify() 
    { 
     throw new IllegalStateException("cached bundles cannot be modified"); 
    } 

    /** 
    * Wraps an invocation of {@link #tryAndLookup(Locale)} into a {@link 
    * FutureTask} 
    * 
    * @param locale the locale to pass as an argument to {@link 
    * #tryAndLookup(Locale)} 
    * @return a {@link FutureTask} 
    */ 
    private FutureTask<MessageSource> lookupTask(final Locale locale) 
    { 
     final Callable<MessageSource> callable = new Callable<MessageSource>() 
     { 
      @Override 
      public MessageSource call() 
       throws IOException 
      { 
       return tryAndLookup(locale); 
      } 
     }; 

     return new FutureTask<MessageSource>(callable); 
    } 
} 

ho potuto anche testare la "single attività creata", in caso di successo di ricerca così come il fallimento, in quanto tryAndLookup() è astratto, dunque "spyable" utilizzando Mockito. Fonte della classe di test completa here (metodi onlyOneTaskIsCreatedPer*LocaleLookup()).

Answer 2

In alternativa alla CopyOnWriteArraySet, si potrebbe usare un ConcurrentHashMap con valori incoerenti (ad esempio sempre utilizzare Boolean.TRUE come valore - vi interessa soltanto i tasti), oppure è possibile utilizzare un ConcurrentSkipListSet che è essenzialmente un concomitante TreeSet che utilizza un skiplist al posto di un albero binario bilanciato.

Supponendo che tryAndLookup sia veloce e non abbia effetti collaterali, non importa se occasionalmente lo si esegue più di una volta, in quanto tale il codice "alla fine thread-safe" è thread-safe in la sensazione che non produrrà alcun comportamento anomalo. (Se è lento, potrebbe essere più efficiente utilizzare i blocchi per assicurarti di eseguirlo il più possibile, ma in questo caso il tuo codice sarebbe comunque libero da comportamenti anomali. una gara di dati se si esegue due volte sullo stesso locale.)

Modifica Perché tryAndLookup possono avere effetti collaterali, è possibile sincronizzare il locale, oppure è possibile modificare il metodo come segue

private final MessageSource nullSource = new MessageSource(); // Used in place of null in the ConcurrentHashMap, which does not accept null keys or values 

protected final List<MessageSource> getSources(final Locale locale) { 
... 

    try { 
     if(sources.putIfAbsent(locale, nullSource) == null) { 
      source = tryAndLookup(locale); 
      sources.replace(locale, nullSource, source); 
      return Arrays.asList(sources.get(locale)); 
     } else { 
      // Another thread is calling tryAndLookup, so wait for it to finish 
      while(sources.get(locale) == nullSource && !lookupFailures.contains(locale)) 
       Thread.sleep(500); 
      } 
      if(sources.get(locale) != nullSource) { 
       return Arrays.asList(sources.get(locale)); 
      } else { 
       return Collections.emptyList(); 
      } 
     } 
    } catch (IOException ignored) { 
     lookupFailures.add(locale); 
     return Collections.emptyList(); 
    } 
} 

Answer 3

È possibile eseguire i primi due passaggi in un blocco sincronizzato utilizzando tipi di dati più semplici. Se in questi passaggi è necessario eseguire lo tryAndLookup(), memorizzare un Future per il risultato in sospeso in un elenco separato di ricerche in sospeso prima di lasciare il blocco sincronizzato.

Quindi al di fuori del blocco sincronizzato, eseguire la ricerca effettiva. I thread che trovano che hanno bisogno dello stesso risultato troveranno il futuro e possono get() il suo risultato al di fuori del blocco sincronizzato.