CODEWITHSUNDEEP
javamultithreadingconcurrencythread-safetyrace-condition
user926958
user926958

Reputation: 9565

Java byte array multi-thread

If I have multiple threads accessing the getter and setter, is this code going to run into any race condition? I do not mind the getter gets the old data during the set operation, but as long as it does not cause an exception or got null.

ConcurrentHashMap<String, Object> hashMap =
    new ConcurrentHashMap<String, Object> ();

void setByteArray(String string, byte[] byteArray) {
    hashMap.put(string, byteArray.clone());
}

byte[] getByteArray(String string) {
    return ((byte[]) hashMap.get(string)).clone();
}

Upvotes: 6

Views: 3249

Answers (4)

wonhee
wonhee

Reputation: 1661

Your code isn't threadsafe, not because of byte stuff but because of the way you use ConcurrentHashMap.

For adding items in the map, you should use putIfAbsent() over put(). PutIfAbsent() is equivalent to 

   if (!map.containsKey(key)){
      return map.put(key, value);
   }
   else {
       return map.get(key);
   }

(probably with proper synchronized block or keyword)

If you just use put(), there are chance that you'll override existing value with new value in multi-thread environment.

Upvotes: 0

rolve
rolve

Reputation: 10218

This is almost thread-safe (if there is such a thing). The only thing missing is to declare the hashMap field final. This guarantees safe publication of the map.

Other than that, I don't see any problems (regarding thread-safety). ConcurrentHashMap is thread safe, so storing and retrieving the byte arrays should be as well.

Also, since you always copy the byte arrays, they will never be shared among threads, except for the ones that are stored in the Map. The ConcurrentHashMap will safely publish those to all threads and since they are never modified (meaning they're effectively immutable), thread safety is guaranteed.

Finally, based on the comments, here is an improved version regarding some other aspects:

private final ConcurrentHashMap<String, Object> hashMap =
    new ConcurrentHashMap<String, Object> ();

void setByteArray(String string, byte[] byteArray) {
    hashMap.put(string, byteArray.clone());
}

byte[] getByteArray(String string) {
    Object result = hashMap.get(string);
    if(result == null)
        return null;
    else
        return ((byte[]) result).clone();
}

The first thing is the private modifier for hashMap, so subclasses can not store any other objects, for example shared byte arrays.

The second thing is the null check in the getter. You might want to replace return null; by throw new IllegalArgumentException(); or something else, based on your requirements.

Upvotes: 5

jabal
jabal

Reputation: 12347

It seems to be ok, as ConcurrentHashMap deals with thread safety, according to the spec http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/ConcurrentHashMap.html

Upvotes: 0

Isaac
Isaac

Reputation: 16736

ConcurrentHashMap is set to deal with it, so my conclusion is that you should be fine for your requirements.

Upvotes: 0

Related Questions