CODEWITHSUNDEEP
javamultithreading
blue-sky
blue-sky

Reputation: 53806

Can race condition occur in below code

Could a race codition occur when invoking method addId from multiple threads ?

private static Map<String , Long> table;

static {
    table = new ConcurrentHashMap<String  , Long>();
}

public static void addId(String key, Long value){

    if(table.containsKey(key)){
        table.remove(key);
    }

    table.put(key, value);
}

Upvotes: 1

Views: 169

Answers (4)

Atreys
Atreys

Reputation: 3761

If you wanted to guarantee something is done with the old value when it is replaced, then the race could result in not handling all the overwritten keys.

if(table.containsKey(key)){
    Long oldValue = table.remove(key);
}
table.put(key, value);
if (oldValue != null)
    importantOperation(oldValue);

Edit:

It looks like put also returns the old value if there was one, so the check is still not necessary (as others have specified). The check would be necessary in the following awkward situation, which would have racing involved:

if(table.containsKey(key)){
    Long oldValue = table.remove(key);
    value = secretlyAdjustInput(oldValue, value)
}
table.put(key, value);

Upvotes: 0

Kiril
Kiril

Reputation: 40345

Yes.

(It seems that my answer has to be 30 characters or more)

The addId call can gave a race condition: one thread can try to put a key and another can remove the a key at the same time (if it's the same key, then it could be a problem). Within that method there are a couple more cases in which you can get a race condition (with various consequences).

This may further be complicated if you have other methods that do other things with the table, i.e. read from it, and it will make your race condition much worse than "just" an overwritten value.

Ultimately, what are you asking? Are you wondering how to avoid the race condition? What does the rest of your map class do?

Upvotes: 2

Paul Cager
Paul Cager

Reputation: 1920

Yes, a race condition could occur.

   if(table.containsKey(key)){
        table.remove(key);
    }

    table.put(key, value);

Another thread could modify the table between your containsKey, the remove and the put. There's no need to call remove() before doing the put(), though - remove the containsKey / remove and it will be thread safe.

Upvotes: 2

aioobe
aioobe

Reputation: 420971

Nothing prevents another thread from putting some value between the containsKey/remove and the put, so checking if the table already contains a key before putting isn't really "safe" (or rather, doesn't really "make sense").

Why don't you just do 

public static void addId(String key, Long value){
    table.put(key, value);
}

put will override any previous entry anyway.

If you don't want multiple threads to execute the method concurrently, declare the method as synchronized.

Upvotes: 8

Related Questions