Java non-final static Map Operation in Static Block

Question

I am trying to understand if below code is thread-safe. I have gone through many many questions in SO but can't seem to find a definite answer.

class Car {
   public static Map<String, String> features = new HashMap<>();
   static {
       features.put("color", "red");
       features.put("foo", "bar");
   }
   public Comparable<?> getValue(String id) {
       if(!features.containsKey(id)) {
          features.put(id, id);
       }
       String res = features.get(id);
       // some business logic and return stmt.
   }
}

We recently encountered unexpected behaviour in our application, wherein, the value returned by getValue("color") was null. I have been unable to reproduce this issue, but it seems to have happened when two threads were being processed at the same time.

The features map is modified only in getValue method if the argument isn't already available in the map.
The issue occurred for an argument which the map is initialised with, in the static block.
Use case example - Car c = new Car(); c.getValue("color");

Any help would be much appreciated. Thanks.

Please post the logic of the method. I believe the problem is there, since the static blocks are initialized at start of application. — Steyrix
– Steyrix, Commented Feb 1, 2020 at 13:03

Arvind Kumar Avinash · Accepted Answer · 2020-02-02 14:11:17Z

3

No, this isn't thread safe.

From the Javadoc of HashMap:

Note that this implementation is not synchronized. If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally.

So, synchronize access to the map:

String res;
synchronized (features) {
  if(!features.containsKey(id)) {
    features.put(id, id);
  }
  res = features.get(id);
}

and make the field final, and synchronize inside the static initializer too.

Or, better, use a ConcurrentHashMap, and the computeIfAbsent method.

String res = concurrentFeatures.computeIfAbsent(id, k -> id);

(HashMap also has a computeIfAbsent method in Java 8+, but you'd need to invoke that in a synchronized block too).

Actually, an even better way to do this in Java 8+ is to use getOrDefault, assuming you don't actually need to keep the previously-unseen key/value pairs:

res = features.getOrDefault(id, id);

This doesn't mutate the map, so you don't have to worry about thread safety here; you just have to make sure that it is initialized safely:

public final static Map<String, String> features;
static {
   Map<String, String> f = new HashMap<>();
   f.put("color", "red");
   f.put("foo", "bar");
   features = f;
}

edited Feb 2, 2020 at 14:11

Arvind Kumar Avinash

81k10 gold badges98 silver badges144 bronze badges

answered Feb 1, 2020 at 13:31

Andy Turner

141k11 gold badges169 silver badges263 bronze badges

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

2 Comments

user2780757 Over a year ago

Thanks - @Eng.Fouad suggested something very similar. Although I have been unable to reproduce this (not surprised there) - I agree this is indeed the issue. Funnily this has been running fine for last 8 years.. :-)

Andy Turner Over a year ago

@user2780757 the pernicious thing about thread safety issues is that the lack of guarantees of correct functioning are not the same as guarantees of incorrect functioning. If it has been "fine" for 8 years, you have either been lucky, or just not spotted the issue.

Collectives™ on Stack Overflow

Java non-final static Map Operation in Static Block

1 Answer 1

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related