Is this class thread-safe?

Question

This class is a singleton. I'am not very good at thread-safety. Is this class thread-safe? Some methods are omitted, but they will used only from one thread. The methods listed here will be accessed from multiple threads simultaneously though.

public class TermsDto {

    private final static MapSplitter mapSplitter = Splitter
            .on(',').trimResults().omitEmptyStrings()
            .withKeyValueSeparator(":");

    private volatile double factorForOthers = 4;

    private volatile Map<String, Double> factorForTermName = 
            new HashMap<String, Double>();

    public void setFactorForOthers(double factorForOthers) {
        this.factorForOthers = factorForOthers;
    }

    public void setFactorForTermNameMapping(String mapping) {
        HashMap<String, Double> tempFactorForTermName = 
                new HashMap<String, Double>();
        for (Map.Entry<String, String> entry : 
                 mapSplitter.split(mapping).entrySet()) {
            double factor = Double.parseDouble(entry.getValue());
            tempFactorForTermName.put(entry.getKey(), factor);
        }
        factorForTermName = tempFactorForTermName;
    }

}

Answer 1

As written, I think the class is thread-safe.

However, the primary reason that it is thread-safe is that the variables factorForOthers and factorForTermName are write only. Since there is no code to read them, there is no possibility that a thread can see them in an inconsistent state.

This of course makes this class singularly useless, and leads us to the obvious conclusion that this is not the real code you are worried about.

---

If factorForOthers was exposed by a getter (for example), it would still be thread-safe. (A double is a primitive, and the reference variable is volatile

If factorForTermName was exposed then there is definitely a risk that the application as a while will not be thread-safe. It depends on whether the exposed map can be updated. If it can be, then there is a significant thread-safety issue. There are two ways to mitigate that:

• You could change setFactorForTermNameMapping to wrap the HashMap using Collections.unModifiableMap(). If your intent is that the map should be read-only, then this is the best solution.

• You could use ConcurrentHashMap instead of HashMap.

Answer 2

Of all the code you have shown, only these are relevant parts:

private volatile double factorForOthers = 4;

private volatile Map<String, Double> factorForTermName = 
        new HashMap<String, Double>();

public void setFactorForOthers(double factorForOthers) {
    this.factorForOthers = factorForOthers;
}

public void setFactorForTermNameMapping(String mapping) {
    HashMap<String, Double> tempFactorForTermName = 
            new HashMap<String, Double>();
    for (Map.Entry<String, String> entry : 
             mapSplitter.split(mapping).entrySet()) {
        double factor = Double.parseDouble(entry.getValue());
        tempFactorForTermName.put(entry.getKey(), factor);
    }
    factorForTermName = tempFactorForTermName;
}

The methods rank and rankSubtractionByCountsPerDay are pure functions, so are thread-safe by definition. Now, since your setFactorForTermNameMapping doesn't depend on any shared state, but only writes to a volatile variable, its operation is atomic.

If the methods you haven't shown only read the map, and are carefully written to access the factorForTermName only once, then the whole class is probably thread-safe.

Answer 3

No. This isn't thread safe. HashMap not thread safe as it is. You can use Synchronized method with HashMap to achieve same thread safe functionality in HashTable

Answer 4

Assuming no other methods modify factorForTermName map this class is thread-safe.

Answer 5

No. The method setFactorForTermNameMapping() traverses a data structure which may not itself be thread-safe for traversal.