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

}

If a class is only used from one thread, why need thread safety? — drvdijk, Aug 06 '13 at 10:09
How is this class used by different threads? And what is `MapSplitter`? — Joni, Aug 06 '13 at 10:12
It is irrelevant if those methods are used from only one thread. If they access shared state, they are subject to the scrutinizing of thread safety. — Marko Topolnik, Aug 06 '13 at 10:13
The fact that a method is used only by one thread is irrelevant. What matters is: is this method accessing state that is also accessed by other threads? — JB Nizet, Aug 06 '13 at 10:13
@JBNizet These methods only read values from map but don't modify it. So i can use factorForTermName = Collections.unmodifiableMap(tempFactorForTermName) insted of — RommSec, Aug 06 '13 at 10:28
@RoKish That's irrelevant unless you can show that nobody else can possibly modify the map at the same time. — user207421, Aug 06 '13 at 10:30
@EJP: it's quite relevant. How could somebody else modify an unmodifiable map? — JB Nizet, Aug 06 '13 at 10:37
@JBNizet There are no unmodifiable maps mentioned in this question. You are going beyond its terms of reference. — user207421, Aug 06 '13 at 10:43
You've got to synchronize externally in case `setFactorForTermNameMapping` and `setFactorForOthers` need to happen atomic so no thread can see a state where those two don't match. Depends on your definition of thread safe whether that makes this class unsafe. — zapl, Aug 06 '13 at 10:44
@EJP: you're becoming more and more insincere. Your comment answers to this OP's comment: "These methods only read values from map but don't modify it. So i can use factorForTermName = Collections.unmodifiableMap(tempFactorForTermName) insted of" If you can't find any unmodifiable map mentioned in this comment, then there's a problem. — JB Nizet, Aug 06 '13 at 10:45
@JBNizet (1) Lay off the personal remarks thanks. (2) I am answering the OP's *comment* that these methods only read values from one map, which isn't relevant unless somebody can show that nobody else can modify it. I didn't understand the second part of his comment about unmodifiable maps, and I still don't, but it formed no part of his actual question. — user207421, Aug 06 '13 at 10:51

Marko Topolnik · Answer 1 · 2013-08-06T10:42:19.850

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.

Make sure you never access `factorForTermName` more than once per method call because that would destroy the atomicity of the `volatile` read. — Marko Topolnik, Aug 06 '13 at 10:52
@MarkoTopolnik - If the map is *effectively* immutable (apart from the `setFactorForTermMapping` call), that restriction is not necessary. What `setFactorForTermMapping` is doing is *safe publication* ... since it fully initializes the map before assigning it to the variable. (OTOH, if the map is mutable, then the map is not thread-safe ... either way.) — Stephen C, Aug 06 '13 at 10:57
@StephenC I have written a comment under your answer, which better explains what I mean. It's a non-data-race race condition. — Marko Topolnik, Aug 06 '13 at 11:00

score 3 · Accepted Answer · edited Aug 06 '13 at 10:55

3

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.

edited Aug 06 '13 at 10:55

Marko Topolnik

195,646
29
319
436

answered Aug 06 '13 at 10:28

Stephen C

698,415
94
811
1,216

There is this other risk of a non-data-race race condition: a method using `factorForTermName` could inadvertently read the reference to this map more than once, assuming it is getting the same value every time. – Marko Topolnik Aug 06 '13 at 10:58
I'll grant you that if a method access `factorForTermName` twice it could see two different maps. But I would hypothesize that you'd *want* that to happen. (Thanks for the edit.) – Stephen C Aug 06 '13 at 11:04
Actually, it is a quite common mistake to forget to save the result of the getter into a local variable, and just use it again and again in a complex expression or a loop. *Especially* if you are not keenly aware of the dangerous consequences. – Marko Topolnik Aug 06 '13 at 11:06
I see your point. But there's not much you can do about it ... I think. – Stephen C Aug 06 '13 at 11:07
Indeed, I just thought it was an important point to stress to the OP, who admits he is not very good at thread safety. – Marko Topolnik Aug 06 '13 at 11:09

score 0 · Answer 3 · answered Aug 06 '13 at 10:11

0

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

answered Aug 06 '13 at 10:11

Ruchira Gayan Ranaweera

34,993
17
75
115

1

It does not look like the code reads and writes his HashMap concurrently, though: only the local gets modified. – Sergey Kalinichenko Aug 06 '13 at 10:12
I haven't said that only setters will be called from different threads – RommSec Aug 06 '13 at 10:13
@dasblinkenlight That doesn't even begin to be relevant. – user207421 Aug 06 '13 at 10:16
1

@EJP Would you care to elaborate on how's that not relevant? – Sergey Kalinichenko Aug 06 '13 at 10:25
@dasblinkenlight It is incumbent on you to show how it is relevant actually, and specifically how the fact that this code doesn't read and write the map concurrently excludes all other non-thread-safe possibilities. It doesn't. – user207421 Aug 06 '13 at 10:29
2

@EJP "All other non-thread-safe possibilities" such as what? I mean, [iterating hash map concurrently is safe, as long as you do not share the iterator itself](http://stackoverflow.com/a/3768659/335858); there's no modifications to the map going on, at least as far as OP cared to show us; what else is there? – Sergey Kalinichenko Aug 06 '13 at 10:34
@dasblinkenlight 'All the other possibilities' such as that there is another thread which has access to the data structure and is modifying it while it's being accessed by this class. – user207421 Aug 06 '13 at 10:41
1

@EJP: the map is private to the class. That's in the code given in the question. And the OP explicitely said that all the other methods of the class only read from the map. He also explicitely said that he could initialize the field with `Collections.unmodifiableMap(...)`. How could another thread get access to the map and modify it? – JB Nizet Aug 06 '13 at 11:04

score 0 · Answer 4 · answered Aug 06 '13 at 10:26

0

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

answered Aug 06 '13 at 10:26

Evgeniy Dorofeev

133,369
30
199
275

score -5 · Answer 5 · answered Aug 06 '13 at 10:13

-5

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

answered Aug 06 '13 at 10:13

user207421

305,947
44
307
483

1

The data structure is local to the method. It's not shared between threads. – JB Nizet Aug 06 '13 at 10:17
3

Err, yes, there is. The map is created by the split() method. See http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/base/Splitter.MapSplitter.html – JB Nizet Aug 06 '13 at 10:21
1

@JBNizet The map *may* be created by the method you cite in a class that isn't specifically referenced by the OP. You are guessing. It remains the case that the data structure 'may not be thread-safe for traversal'. Thread safety needs to be proven, not guessed at. – user207421 Aug 06 '13 at 10:26
1

OK. So if the OP rewrote Guava's Splitter with exactly the same method and class names but in a non-threadsafe way, then there might be a problem. If it's the real Guava Splitter, then the Splitter is thread-safe (documented) and the map as well (documented as unmodifiable) – JB Nizet Aug 06 '13 at 10:31
@JBNizet If that's your answer I suggest you post it where it can attract its own votes and comments. There's nothing here that invalidates mine. – user207421 Aug 06 '13 at 10:34
This does not provide an answer to the question. To critique or request clarification from an author, leave a comment below their post. – Harshal Patil Mar 30 '15 at 04:11
1

@HarshalPatil You are mistaken. 'No' is indeed an answer to the question. It may be a *wrong* answer: that;s a different thing. – user207421 Jun 19 '15 at 02:09

Is this class thread-safe?

5 Answers5