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.
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) {}
}