CODEWITHSUNDEEP
javacollectionsthread-safetygetter-setter
RommSec
RommSec

Reputation: 13

Is this class thread-safe?

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

}

Upvotes: 0

Views: 276

Answers (5)

Stephen C
Stephen C

Reputation: 718758

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.

Upvotes: 3

Marko Topolnik
Marko Topolnik

Reputation: 200148

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.

Upvotes: 5

Evgeniy Dorofeev
Evgeniy Dorofeev

Reputation: 135992

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

Upvotes: 0

user207421
user207421

Reputation: 310885

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

Upvotes: -5

Ruchira Gayan Ranaweera
Ruchira Gayan Ranaweera

Reputation: 35557

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

Upvotes: 0

Related Questions