Thread safety of enum valueOf()

Question

This is a variation of the perpertual "lengthy-if or switch" dilemma...

Consider a multithreaded application using a static method that contains a long (over a dozen conditions) if statement, which checks the type of an object and returns a value accordingly, i.e. something like

public static String checkType(Class<?> type)
{
    if (type == A.class)
    {
        return aString;
    }
    else if (type == B.class)
    {
        return bString;
    }
    ...
    else if (type == z.class)
    {
        return zString;
    }
}

Obviously a switch statement is not directly applicable here, so a common pattern is to have an enum and call its valueOf(), i.e. do something like

public enum Strings
{
    A(aString), B(bString), ..., Z(zString)

    private final String value;

    private Strings(String value)
    {
        this.value = value;
    }

    public String value()
    {
        return this.value;
    }
}

So, checkType() could be re-written as

public static String checkType(Class<?> type)
{
    return Strings.valueOf(getActualTypeName(type.getClass().getName())).value();
}

with appropriate checks for null values added in production code and some String processing for non-primitive types, inside the getActualTypeName() method, to retrieve the actual type name from strings like "class java.lang.Long" (for primitives, the getName() method returns the expected string, e.g. "long").

However, if valueOf() is not thread-safe, this will not work in a concurrent environment. The same applies to using a (normal) Map object and probably these two alternatives are variants of the same pattern, since enum.valueOf() is apparently based on

Enum.valueOf(Class<T> enumType, String name)

which calls

enumType.enumConstantDirectory().get(name);

in the Class.java class.

The enumConstantDirectory() method, every time invoked, returns a new HashMap, created from a copy of the values() array.

Would that be thread safe?

Answer 1

I can't find any reasons why enum.valueOf(String) would not be thread safe:

• strings are immutable, so the argument can't be mutated while valueOf does its job
• valueOf checks the argument vs. the names of the enum constants and they are all static and final

What makes you think that enum.valueOf() is not thread safe?

EDIT

valueOf calls:

T result = enumType.enumConstantDirectory().get(name);

where enumType is your enum class.

enumConstantDirectory() uses this pattern:

Map<String, T> enumConstantDirectory() {
    if (enumConstantDirectory == null) {
        T[] universe = getEnumConstantsShared();
        if (universe == null)
            throw new IllegalArgumentException(
                getName() + " is not an enum type");
        Map<String, T> m = new HashMap<>(2 * universe.length);
        for (T constant : universe)
            m.put(((Enum<?>)constant).name(), constant);
        enumConstantDirectory = m;
    }
    return enumConstantDirectory;
}

where enumConstantDirectory is a volatile variable:

private volatile transient Map<String, T> enumConstantDirectory = null;

Imagine a thread arriving concurrently in that method:

• if enumConstantDirectory is null (there is no visibility issue here because it is volatile), it will construct the map and assign it to that variable. Because of the volatile guarantees, all other threads, from that point in time, will see the map fully constructed.
• if another thread arrives in the method at the same time and also observe a null value of enumConstantDirectory, it will recreate the map and safely publish it again

The worst case scenario here is that 2 threads could potentially be using 2 different maps (different instances) but their content will be the same so it would not cause any issues.

Bottom line: there is no way that a thread could see a map which is half constructed, because the map construction is done on a local variable, which is assigned to the volatile variable after it has been populated.

Answer 2

There is no reason to suppose that Enum.valueOf() is not thread-safe. It does not mutate anything, and it is only accessing state in the actual enum class that is effectively final.

If this method was non-thread-safe, I think there would be something in the javadocs to say so.

Answer 3

May be I am wrong but it seems there is a subtle issue here:

public static <T extends Enum<T>> T valueOf(Class<T> enumType,
                                                       String name) {
     T result = enumType.enumConstantDirectory().get(name);
     if (result != null)
           return result;
     if (name == null)
           throw new NullPointerException("Name is null");
     throw new IllegalArgumentException(
                     "No enum constant " + enumType.getCanonicalName() + "." + name);
}  

This is the code for valueOf. It uses the passed in enumType to create an internal HashMap with the constants and the code is not sychronized.
There seems a subtle issue here: T result = enumType.enumConstantDirectory().get(name);
The enumConstantDirectory() does a check for enumConstantDirectory == null but it is not synchronized, in order to create the HashMap. Perhaps the side-effects are not important (I don't know what info the Class stores) but in any case it is surely safe as long as enumType is not shared in your application code