Search code examples
javamultithreadingconcurrencyconcurrenthashmap

Is this code a thread-safe one?


I want to process a flow of client requests. Each request has its special type. First I need to initialize some data for that type, and after this I can start processing the requests. When the client type comes for the first time, I just initialize the corresponding data. After this all the following requests of that type are processed using that data.

I need to do this in a thread-safe manner.

Here is a code I have written. Is it 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) {}
}

From one hand, ConcurrentHashMap cannot produce map.put(A,B) == null two times for the same A. From the other hand, the assignment and comparisson operation is not thread-safe.

So is this code is ok? If not, how can I fix it?

UPDATE: I have accepted the answer of Martin Serrano because his code is thread-safe and it is not prone to double initialization issue. But I would like to note, that I have not found any isssues with my version, posted as an answer below, and my version does not require synchronization.


Solution

  • This code is not thread safe because

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

    has a chance of getting the "" value you temporarily insert in the put check.

    This code could be made 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) {}
    

    }