How to implement thread-safe HashMap lazy initialization when getting value in Java?

user_1667722 0
Viewed 347

I want to implement a util getting an Enum object by its string value. Here is my implementation.

IStringEnum.java

public interface IStringEnum {
    String getValue();
}

StringEnumUtil.java

public class StringEnumUtil {
    private volatile static Map<String, Map<String, Enum>> stringEnumMap = new HashMap<>();

    private StringEnumUtil() {}

    public static <T extends Enum<T>> Enum fromString(Class<T> enumClass, String symbol) {
        final String enumClassName = enumClass.getName();
        if (!stringEnumMap.containsKey(enumClassName)) {
            synchronized (enumClass) {
                if (!stringEnumMap.containsKey(enumClassName)) {
                    System.out.println("aaa:" + stringEnumMap.get(enumClassName));
                    Map<String, Enum> innerMap = new HashMap<>();
                    EnumSet<T> set = EnumSet.allOf(enumClass);
                    for (Enum e: set) {
                        if (e instanceof IStringEnum) {
                            innerMap.put(((IStringEnum) e).getValue(), e);
                        }
                    }
                    stringEnumMap.put(enumClassName, innerMap);
                }
            }
        }
        return stringEnumMap.get(enumClassName).get(symbol);
    }
}

I wrote a unit test in order to test whether it works in multi-thread case.

StringEnumUtilTest.java

public class StringEnumUtilTest {
    enum TestEnum implements IStringEnum {
        ONE("one");
        TestEnum(String value) {
            this.value = value;
        }
        @Override
        public String getValue() {
            return this.value;
        }
        private String value;
    }

    @Test
    public void testFromStringMultiThreadShouldOk() {
        final int numThread = 100;
        CountDownLatch startLatch = new CountDownLatch(1);
        CountDownLatch doneLatch = new CountDownLatch(numThread);
        List<Boolean> resultList = new LinkedList<>();
        for (int i = 0; i < numThread; ++i) {
            new Thread(() -> {
                try {
                    startLatch.await();
                } catch (Exception e) {
                    e.printStackTrace();
                }
                resultList.add(StringEnumUtil.fromString(TestEnum.class, "one") != null);
                doneLatch.countDown();
            }).start();
        }
        startLatch.countDown();
        try {
            doneLatch.await();
        } catch (Exception e) {
            e.printStackTrace();
        }
        assertEquals(numThread, resultList.stream().filter(item -> item.booleanValue()).count());
    }
}

The testing result is:

aaa:null

java.lang.AssertionError: 
Expected :100
Actual   :98

It denotes that only one thread execute this line of code:

System.out.println("aaa:" + stringEnumMap.get(enumClassName));

So the initialization codes should be executed by only one thread.

The strange thing is, the result of some thread will be null after executing this line of code:

return stringEnumMap.get(enumClassName).get(symbol);

Since there is no NullPointerException, stringEnumMap.get(enumClassName) must return the reference of innerMap. But why it will get null after calling get(symbol) of innerMap?

Please help, it drive me crazy the whole day!

java.util.concurrent concurrenthashmap multithreading java
4 Answers

stringEnumMap should be a ConcurrentHashMap<String, Map<String,Enum>>, and use computeIfAbsent to do the lazy initialization.

user_54835260

The problem is due to the line

List<Boolean> resultList = new LinkedList<>();

From JavaDoc of LinkedList:

Note that this implementation is not synchronized.If multiple threads access a linked list concurrently, and at least one of the threads modifies the list structurally, it must be synchronized externally. (A structural modification is any operation that adds or deletes one or more elements; merely setting the value of an element is not a structural modification.) This is typically accomplished by synchronizing on some object that naturally encapsulates the list.If no such object exists, the list should be "wrapped" using the Collections.synchronizedListmethod. This is best done at creation time, to prevent accidental unsynchronized access to the list:
List list = Collections.synchronizedList(new LinkedList(...));

As LinkedList is not thread safe, and unexpected behavior may happens during the add operation. Which cause the resultList size less than the thread count, and hence the expected count is less than the result count.
To get correct result, add Collections.synchronizedList as suggested.

Although you implementation is fine, I suggest you to follow Matt Timmermans answer for simpler and robust solution.

user_79287210

ConcurrentMap interface

As others noted, if manipulating a Map across threads you must account for concurrency.

You could handle concurrent access yourself. But there is no need. Java comes with two implementations of Map that are built to internally handle concurrency. These implementations implement the ConcurrentMap interface.

  • ConcurrentSkipListMap
  • ConcurrentHashMap

The first maintains the keys in sorted order, implementing the NavigableMap interface.

Here is a table I authored to show the characteristics of all the implementations of Map bundled with Java 11.

Table of map implementations in Java 11, comparing their features

You might find other third-party implementations of the ConcurrentMap interface.

user_6427060

try moving
if (!stringEnumMap.containsKey(enumClassName))
and the
return stringEnumMap.get(enumClassName).get(symbol);
into the synchronized block.

user_97747180
Related