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, Feb 01 '20 at 13:03

score 3 · Accepted Answer · edited Feb 02 '20 at 14:11

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

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.. :-) — user2780757, Feb 01 '20 at 14:09
@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. — Andy Turner, Feb 01 '20 at 14:59

Java non-final static Map Operation in Static Block

1 Answers1