Java byte array multi-thread

Question

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();
}

Yes, this code is almost thread-safe, but don't forget to declare hashmap private and final.So if someone creates the subclass, he wouldn't be able to break encapsulation and thread safety. — Boris Treukhov, Nov 20 '12 at 09:25
cloning `byte[]` can be very expensive if done often. I assume you are using multiple threads to improve performance so I would consider a strategy which avoids the need to clone byte[]s — Peter Lawrey, Nov 20 '12 at 09:25
I'd suggest to check that hashMap.get(string) != null before cloning — Evgeniy Dorofeev, Nov 20 '12 at 09:30
@BorisTreukhov, subclasses will not be able to break thread-safety. The instantiated object is a `ConcurrentHashMap`. — Isaac, Nov 20 '12 at 09:34
@Isaac the will break thread safety if the programmer will add something to the Map and forget to make the defensive copy with clone. So you should hide the reference to the hashmap from the subclasses and avoid its leaking. — Boris Treukhov, Nov 20 '12 at 09:35
@Nerrve see http://stackoverflow.com/questions/12177081/safe-publication-of-java-util-concurrent-collections So alas the code in the question seems to be not quite threadsafe. — Boris Treukhov, Nov 20 '12 at 09:36
@EvgeniyDorofeev, check-then-act is pointless with concurenthashmap, you can only rely on the atomic operations provided by the concurrent class itself. p.s. Ah gotcha, sorry, you're right - дошло короче ))) — Boris Treukhov, Nov 20 '12 at 09:41

score 5 · Accepted Answer · edited May 23 '17 at 10:34

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.

final is not only about replacing the reference but also about memory visibility see M. Topolnik answer : http://stackoverflow.com/questions/12177081/safe-publication-of-java-util-concurrent-collections/12177310#12177310 — Boris Treukhov, Nov 20 '12 at 09:52
I am curious, how is getByteArray( .. ) thread safe? It is performing a "check then act" compound action which is not thread-safe. The entire action needs to be "locked" or the method can simply return "hashMap.get( )" and the calling thread can then perform the check and clone part ... — CaptainHastings, Apr 25 '13 at 15:14
@CaptainHastings "Check then act" is only wrong if the property that is checked could change before the act. This is not possible here because `result` is a local variable and the property is nullness. — rolve, Jul 20 '13 at 06:27

score 0 · Answer 2 · answered Nov 20 '12 at 09:21

0

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

answered Nov 20 '12 at 09:21

Isaac

16,458
5
57
81

score 0 · Answer 3 · answered Nov 20 '12 at 09:25

0

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

answered Nov 20 '12 at 09:25

jabal

11,987
12
51
99

score 0 · Answer 4 · answered Nov 11 '15 at 22:02

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.

Java byte array multi-thread

4 Answers4