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
– Boris Treukhov, Commented Nov 20, 2012 at 9: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
– Peter Lawrey, Commented Nov 20, 2012 at 9:25
I'd suggest to check that hashMap.get(string) != null before cloning — Evgeniy Dorofeev
– Evgeniy Dorofeev, Commented Nov 20, 2012 at 9:30
@BorisTreukhov, subclasses will not be able to break thread-safety. The instantiated object is a ConcurrentHashMap. — Isaac
– Isaac, Commented Nov 20, 2012 at 9:34
@Nerrve see stackoverflow.com/questions/12177081/… So alas the code in the question seems to be not quite threadsafe. — Boris Treukhov
– Boris Treukhov, Commented Nov 20, 2012 at 9:36

Community · Accepted Answer · 2017-05-23 10:34:11Z

5

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.

edited May 23, 2017 at 10:34

CommunityBot

11 silver badge

answered Nov 20, 2012 at 9:20

rolve

10.3k4 gold badges58 silver badges78 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Boris Treukhov Over a year ago

final is not only about replacing the reference but also about memory visibility see M. Topolnik answer : stackoverflow.com/questions/12177081/…

CaptainHastings Over a year ago

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 ...

rolve Over a year ago

@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.

Isaac · Accepted Answer · 2012-11-20 09:21:22Z

0

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

answered Nov 20, 2012 at 9:21

Isaac

16.8k5 gold badges63 silver badges83 bronze badges

Comments

jabal · Accepted Answer · 2012-11-20 09:25:40Z

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, 2012 at 9:25

jabal

12.5k13 gold badges55 silver badges103 bronze badges

Comments

wonhee · Accepted Answer · 2015-11-11 22:02:23Z

0

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.

answered Nov 11, 2015 at 22:02

wonhee

1,6611 gold badge15 silver badges24 bronze badges

Collectives™ on Stack Overflow

Java byte array multi-thread

4 Answers 4

3 Comments

Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

3 Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related