Questo codice è un thread sicuro?

Question

Desidero elaborare un flusso di richieste client. Ogni richiesta ha il suo tipo speciale. Per prima cosa ho bisogno di inizializzare alcuni dati per quel tipo, dopodiché posso iniziare ad elaborare le richieste. Quando il tipo di client arriva per la prima volta, ho appena inizializzato i dati corrispondenti. Successivamente, tutte le seguenti richieste di quel tipo vengono elaborate utilizzando tali dati.

Ho bisogno di farlo in un modo sicuro per thread.

Ecco un codice che ho scritto. È thread-safe?

public class Test { 

    private static Map<Integer, Object> clientTypesInitiated = new ConcurrentHashMap<Integer, Object>(); 

    /* to process client request we need to 
    create corresponding client type data. 
    on the first signal we create that data, 
    on the second - we process the request*/ 

    void onClientRequestReceived(int clientTypeIndex) { 
     if (clientTypesInitiated.put(clientTypeIndex, "") == null) { 
      //new client type index arrived, this type was never processed 
      //process data for that client type and put it into the map of types 
      Object clientTypeData = createClientTypeData(clientTypeIndex); 
      clientTypesInitiated.put(clientTypeIndex, clientTypeData); 
     } else { 
      //already existing index - we already have results and we can use them 
      processClientUsingClientTypeData(clientTypesInitiated.get(clientTypeIndex)); 
     } 
    } 

    Object createClientTypeData(int clientIndex) {return new Object();} 

    void processClientUsingClientTypeData(Object clientTypeData) {} 
} 

Da un lato, ConcurrentHashMap non può produrre map.put (A, B) == nulli due volte per lo stesso A. Da altra parte, l'operazione di assegnazione e comparisson non è thread-safe.

Quindi questo codice è ok? In caso contrario, come posso risolverlo?

AGGIORNAMENTO: Ho accettato la risposta di Martin Serrano perché il suo codice è thread-safe e non è soggetto a un doppio problema di inizializzazione. Ma vorrei sottolineare che non ho trovato alcun problema con la mia versione, pubblicato come risposta sotto, e la mia versione non richiede la sincronizzazione.

Answer 1

Questo codice non è thread-safe perché

//already existing index - we already have results and we can use them 
processClientUsingClientTypeData(clientTypesInitiated.get(clientTypeIndex)); 

ha la possibilità di ottenere il "valore" si inserisce temporaneamente nel controllo put.

Questo codice potrebbe essere fatto threadsafe thusly:

public class Test { 

    private static Map<Integer, Object> clientTypesInitiated = new ConcurrentHashMap<Integer, Object>(); 

    /* to process client request we need to 
     create corresponding client type data. 
     on the first signal we create that data, 
     on the second - we process the request*/ 

void onClientRequestReceived(int clientTypeIndex) { 
    Object clientTypeData = clientTypesInitiated.get(clientTypeIndex); 
    if (clientTypeData == null) { 
     synchronized (clientTypesInitiated) { 
      clientTypeData = clientTypesInitiated.get(clientTypeIndex); 
      if (clientTypeData == null) { 
       //new client type index arrived, this type was never processed 
       //process data for that client type and put it into the map of types 
       clientTypeData = createClientTypeData(clientTypeIndex); 
       clientTypesInitiated.put(clientTypeIndex, clientTypeData); 
      } 
     } 
    } 
    processClientUsingClientTypeData(clientTypeData); 
} 

Object createClientTypeData(int clientIndex) {return new Object();} 

void processClientUsingClientTypeData(Object clientTypeData) {} 

}

Answer 2

No, non penso che sia ancora sicuro per i thread.

È necessario disporre l'operazione di inserimento in un blocco sincronizzato.

Secondo javadoc for ConcurrentHashMap

operazioni di recupero (compresi get) generalmente non ostruire, così possono sovrapporsi con le operazioni di aggiornamento (compresi mettere e togliere).

Answer 3

No non lo è, se avete bisogno di tutto il metodo da thread-safe dichiarano come sincronizzato, o utilizzare un blocco sincronizzato a inserire il codice critico.

Answer 4

dovresti usare putIfAbsent qui, la semantica di questa operazione è simile a CAS, ed è certamente atomica. E dal momento che è atomico - allora non hai problemi con incarichi interni e confronti.

Il codice corrente non è thread-safe.

Answer 5

Il codice corrente non è thread-safe poiché può verificarsi un cambio di contesto tra l'if e il put successivo. è necessario creare clientTypesInitiated come tipo ConcurrentHashMap e non sulla mappa dell'interfaccia. Quindi utilizzare il metodo putIfAbsent

Answer 6

Penso che il codice non funzioni come previsto. Guarda:

void onClientRequestReceived(int clientTypeIndex) { 
    // the line below ensures the existing data is lost even if was not null 
    if (clientTypesInitiated.put(clientTypeIndex, "") == null) { 
     Object clientTypeData = createClientTypeData(clientTypeIndex); 
     clientTypesInitiated.put(clientTypeIndex, clientTypeData); 
    } else { 
     processClientUsingClientTypeData(clientTypesInitiated.get(clientTypeIndex)); 
    } 
} 

Immagino che tu preferisca una chiamata al metodo get.

Io suggerirei un tale approccio:

void onClientRequestReceived(int clientTypeIndex) { 
    Object clientTypeData; 
    synchronized (clientTypesInitiated) { 
     if ((clientTypeData = clientTypesInitiated.get(clientTypeIndex)) == null) { 
      clientTypeData = createClientTypeData(clientTypeIndex); 
      clientTypesInitiated.put(clientTypeIndex, clientTypeData); 
     } 
    } 
    processClientUsingClientTypeData(clientTypeData); 
} 

Answer 7

putIfAbsent funziona grande, ma a volte richiederà inizializzazione ripetuto per diverse volte. Il blocco sincronizzato con blocco a doppio controllo probabilmente può risolvere questo problema.

Ma qui è un'altra opzione sulla base di AtomicIntegerArrays:

public class Test { 

    private static Map<Integer, Object> clientTypesInitiated = new ConcurrentHashMap<Integer, Object>(); 

    private static final int CLIENT_TYPES_NUMBER = 10; 
    private static AtomicIntegerArray clientTypesInitStarted = new AtomicIntegerArray(CLIENT_TYPES_NUMBER); 
    private static AtomicIntegerArray clientTypesInitFinished = new AtomicIntegerArray(CLIENT_TYPES_NUMBER); 

    void onClientRequestReceived(int clientTypeIndex) { 

     int isInitStarted = clientTypesInitStarted.getAndIncrement(clientTypeIndex); 
     int isInitFinished = clientTypesInitFinished.get(clientTypeIndex); 

     if (isInitStarted == 0) { 
      //new client type index arrived, this type was never processed 
      //process data for that client type and put it into the map of types 
      Object clientTypeData = createClientTypeData(clientTypeIndex); 
      clientTypesInitiated.put(clientTypeIndex, clientTypeData); 
      clientTypesInitFinished.getAndIncrement(clientTypeIndex); 
     } else if (isInitFinished > 0) { 
      //already existing index - we already have results and we can use them 
      processClientUsingClientTypeData(clientTypesInitiated.get(clientTypeIndex)); 
     } 
    } 

    Object createClientTypeData(int clientIndex) { 
     return new Object(); 
    } 

    void processClientUsingClientTypeData(Object clientTypeData) { 
    } 
} 

Tutte le opinioni?

Answer 8

ConcurrentHashMap è stato creato per non utilizzare il blocco sincronizzato, sebbene utilizzarlo come blocco per il blocco sincronizzato sia ok, ma non efficiente. Se è necessario utilizzare ConcurrentHashMap, l'utilizzo corretto è il seguente;

private static Map<Integer, Object> clientTypesInitiated = new ConcurrentHashMap<Integer, Object>(); 

void onClientRequestReceived(int clientTypeIndex) { 
    Object clientTypeData = clientTypesInitiated.get(clientTypeIndex); 
    if (clientTypeData == null) { 
     Object newClientTypeData = createClientTypeData(clientTypeIndex); 
     clientTypeData = clientTypesInitiated.putIfAbsent(clientTypeIndex, newClientTypeData); 
     // if clientTypeData is null, then put successful, otherwise, use the old one 
     if (clientTypeData == null) { 
      clientTypeData = newClientTypeData; 
     } 
    } 
    processClientUsingClientTypeData(clientTypeData); 
} 

Per essere onesti, il codice di cui sopra potrebbe creare clientTypeData più volte (a causa della condizione di competizione), anche se non usa synchronized blocco. Pertanto, dovresti misurare quanto è costoso creare clientTypeData e se è così costoso che oscura non usando il blocco synchronized, non hai nemmeno bisogno di usare ConcurrentHashMap. Utilizzare il blocco normale HashMap e synchronized.