Is this the correct and efficient way to make a singleton map thread safe in a static method

Question

I would like to create lock only once when map is initialized. Here is the code i am using.

public static Map<String, String> getOrderStatusInstance() {
    if (orderStatusMap == null) {
        synchronized (Utility.class) {
            orderStatusMap = new HashMap<String, String>();

            orderStatusMap.put("Key1", "Value1");
            orderStatusMap.put("Key2", "Value2");
            orderStatusMap.put("Key3", "Value3");
            orderStatusMap.put("Key4", "Value4");
        }
    }

    return orderStatusMap;
}

You could use a `HashTable`, which is already a thread-safe container. No reason to do the extra work. — Hunter McMillen, May 21 '12 at 17:50
No, it isn't due to the fact that immidiatly after setting "orderStatusMap" varible another thread may use it before it filled. Further more, two thread may pass first check "if (orderStatusMap == null) { ..." before values would be assigned. — Dmitry, May 21 '12 at 17:52
Do you need it to be lazy? If so you might want to indicate this as statically creating the HashMap would certainly be simpler. — Maarten Bodewes, May 21 '12 at 18:11

score 9 · Accepted Answer · answered May 21 '12 at 17:50

No, that's a bad idea - you're returning a mutable, non-thread-safe map. You're also trying to implement double-checked locking, but not doing it properly - and it's harder to get it right than it is to use static initialization.

I would create an immutable map (using Guava, for preference), ideally as part of class initialization:

private static final ImmutableMap<String, String> STATUS_MAP = ImmutableMap.of(
    "Key1", "Value1",
    "Key2", "Value2",
    "Key3", "Value3",
    "Key4", "Value4");

public static ImmutableMap<String, String> getOrderStatusInstance() {
    return STATUS_MAP;
}

(For more than 5 key/value pairs, use ImmutableMap.Builder.)

Do you really need it to be any lazier than that?

Thanks. I haven't use Guava yet, seems very useful! – TastyCode May 22 '12 at 18:39 — TastyCode, May 22 '12 at 18:39

score 2 · Answer 2 · edited May 23 '17 at 12:20

Nearly correct... Think of the case where one thread checks orderStatusMap == null and gets true. Then the scheduler switches to another thread which does the same check. No both threads will execute the synchronized block.

Prevent this by another check for null in the synchronized block:

if (orderStatusMap == null) {
    synchronized (Utility.class) {
        if (orderStatusMap == null) {
            Map<String, String> tmp = new HashMap<String, String>();

            tmp.put("Key1", "Value1");
            tmp.put("Key2", "Value2");
            tmp.put("Key3", "Value3");
            tmp.put("Key4", "Value4");

            orderStatusMap = tmp;
        }
    }
}
return orderStatusMap;

Yes, it's fine to do that twice. The outer check still helps performance, because after the map has been created, the expensive step of entering the synchronized block does not need to be done anymore.

Remember that this is a thread safe method to create the hashmap. It does not make the map thread-safe.

P.S.: If you like this question, you might also like: How to directly initialize a HashMap (in a literal way)?

The `return orderStatusMap;` - missing from your code - may still return a partially filled map. See the comment from Dmitry on npe's answer. — Maarten Bodewes, May 21 '12 at 18:04

score 0 · Answer 3 · answered May 21 '12 at 17:50

0

You should do another check for null inside the synchronized block.

When two threads call your method, they will both find orderStatusMap is null, and one of them will get inside synchronized block, while the other will block. But eventually it will be passed inside the sync block and initialize the map again.

answered May 21 '12 at 17:50

npe

15,395
1
56
55

1

It is not enough. Assign orderStatusMap variable should be the last action. – Dmitry May 21 '12 at 17:57

score 0 · Answer 4 · answered May 21 '12 at 18:06

no it is not correct

also there is no need for the lazy initialization here (which I believe is overused anyway) so just do:

private static final Map<String, String> orderStatusMap;
static{
    orderStatusMap = Collections.synchronizedMap(new HashMap<String, String>());//making it thread safe
    //or use a ConcurrentHashMap

    orderStatusMap.put("Key1", "Value1");
    orderStatusMap.put("Key2", "Value2");
    orderStatusMap.put("Key3", "Value3");
    orderStatusMap.put("Key4", "Value4");
}

public static Map<String, String> getOrderStatusInstance() {
    return orderStatusMap;
}

Is this the correct and efficient way to make a singleton map thread safe in a static method

4 Answers4