2

Hi
Is the class below threadsafe?

class ImmutablePossiblyThreadsafeClass<K, V> { private final Map<K, V> map; public ImmutablePossiblyThreadsafeClass(final Map<K, V> map) { this.map = new HashMap<K, V>(); for (Entry<K, V> entry : map.entrySet()) { this.map.put(entry.getKey(), entry.getValue()); } } public V get(K key) { return this.map.get(key); } }
Improve this question
2

8 Answers 8

Reset to default
5

If this is the whole class definition, i.e. there are no other setter/modifier methods, the usage of the map itself is thread safe:

  • its initialization and visibility is safeguarded by declaring it final,
  • the containing class has only a getter, no modifier methods,
  • the contents of the map are copied from the constructor parameter map, so there can be no outside references through which it can be modified.

(Note though that this refers to the structure of the map only - the individual elements inside the map may still be unsafe.)

Update

Regarding the claim that the class is not guarded from the constructor parameter being concurrently modified during construction, I tend to agree with others (incl. @Bozho) who have already pointed out that

  1. there is no known way to guard against this (not even using a ConcurrentHashMap as @Grundlefleck suggested - I checked the source code and its constructor just calls putAll(), which is not guarded against this either),
  2. it is the caller's responsibility rather than the callee's to ensure that the constructor parameter is not being concurrently modified. In fact, can anyone define the "right" behaviour for processing a constructor parameter which is being concurrently modified? Should the created object contain the original elements in the parameter map, or the latest elements?... If one starts to think about this, IMHO it is not difficult to realize that the only consistent solution is to disallow concurrent modification of the constructor parameter, and this can only be ensured by the caller.
Improve this answer
Sign up to request clarification or add additional context in comments.

13 Comments

During copying, another thread can modify the parameter. And entrySet says If the map is modified while an iteration over the set is in progress, the results of the iteration are undefined. Usage is threadsafe, but construction is not.
@Péter Török - That there is nothing to prevent it, does not make it safe. It is almost threadsafe, as Grundlefleck explains.
@Péter Török The class isnt completely thread-safe has the class is not final.
@Grundlefleck, I guess what he meant was that it is possible to add e.g. an unsynchronized setter method in a subclass, thus breaking both encapsulation and thread safety.
@Grundlefleck - The reason I made notion the class needing to be final in order to be thread safe is mostly based on the name of the class. ImmutablePossiblyThreadsafeClass implies the class itself needs to be immutable. And as we know if a class is able to be extended it cannot be immutable, in my opinion this breaks the thread-safety the OP is after.
|
3

There's a potential problem when the constructor parameter's contents change while you're copying them. But there isn't really anything you can do to prevent that. Another potential problem would be with mutable keys.

Also, you can replace the constructor with this:

public ImmutablePossiblyThreadsafeClass(final Map<K, V> map) { this.map = new HashMap<K, V>(map); }
Improve this answer

2 Comments

Good point! However, wouldn't that result in a ConcurrentModificationException, preventing the construction of the object?
@Péter: often but not always. From the API doc: "Note that fail-fast behavior cannot be guaranteed as it is, generally speaking, impossible to make any hard guarantees in the presence of unsynchronized concurrent modification."
1

Assuming that nothing changes the map field after construction (since you've named it "immutable" I guess that's the idea) AND the map provided as constructor argument is not altered during construction, that is a thread-safe class.

Mind that there are easier ways of copying entries from one map to another than iteration. You might also want to look at some of the methods in Collections to create immutable versions of collections. You'd first have to copy the map, of course, since the underlying map could still be changed. The "unmodifiable*" methods only return a view of a collection.

Improve this answer

Comments

1

Yes, it is thread-safe. It is immutable so no thread can make any changes that will interfere with the results for another thread.

Note: If the map is modified during construction, you won't get the contents you thought you passed, but when accessing the map afterwards, it will be thread-safe.

Improve this answer

4 Comments

what about: V v = objOfTheClass.get(key); v.changesomething();... is that still immutable?
@fluring - and? If V itself is not threadsafe - that's a different story.
It is mutated during construction, a thread can make changes to the map parameter that will interfere with the results for the constructing thread. No, it is not thread-safe.
@Ishtar - that is obvious, but there is no way to prevent that.. in fact, this would be the responsibility of the caller, to pass an unmodifiable map
1

Yes and no, for different values of "thread safe" (sorry for the obtuse answer). The construction is not thread safe, but once the constructor completes, and assuming that has happened correctly, it will be immutable and threadsafe.

During construction, the map paramater may be modified by another thread between the call to map.entrySet and the calls to entry.getKey and entry.getValue. From the javadoc of Map.entrySet. This can result in undefined behaviour.

From the java.util.Map javadoc for entrySet:

Returns a set view of the mappings contained in this map. Each element in the returned set is a Map.Entry. The set is backed by the map, so changes to the map are reflected in the set, and vice-versa. If the map is modified while an iteration over the set is in progress, the results of the iteration are undefined. The set supports element removal, which removes the corresponding mapping from the map, via the Iterator.remove, Set.remove, removeAll, retainAll and clear operations. It does not support the add or addAll operations.

So depending on who has a reference to the map, and how they manipulate it across threads, and how long it takes to copy the elements, you have undefined behaviour, and possibly different behaviour on different executions of the program.

There seems to be little you can do about ensuring it is constructed correctly from within this class (since, for example, ConcurrentHashMap has a similar problem). If the rest of your code can ensure that the constructor is given a thread-safe map, it will also be thread safe, but that's a wider scope than you asked.

Once, however, the constructor completes, and it is safely published[1] it will be immutable and thread safe - though only in a shallow way - if the keys or entries are mutable, this taints your class, making it neither immutable nor thread-safe. So, if your keys are String's and your values are Integer's, it will be immutable and thus thread safe. If however, your keys are a bean-like object with setters, and your values are other mutable collection types, then your class is not immutable. I tend to label this a "turtles all the way down" condition.

Essentially, your class could be immutable and thread safe, under certain, out-of-scope conditions not covered in your question.

[1] Your class will currently be published safely, but if, for instance, it becomes a nested class, or you pass the this reference to another method during the constructor, this can become an unsafe publication.

Improve this answer

6 Comments

How does making a shallow copy eliminate the risk of concurrent modification? The source map can still be modified during the shallow copy process...
@Peter I just realised that oversight, hopefully corrected now. Happen to know if the info about ConcurrentHashMap is correct?
+1 There is no 'threadsafe'. "you could have undefined behaviour" implies it is impossible to define the behavior, thus it is undefined. The "could" can be removed. :)
ConcurrentHashMap's constructor has exactly the same problem.
Strongly concur with your obtuseness - "thread-safe" is a term that is ambiguous and, even when not ambiguous, poorly understood; it's important to realise it's not a binary flag.
|
0

Use ConcurrentHashMap instead of HashMap and ConcurrentMap when declaring field and it should be.

Improve this answer

Comments

0

Class itself is thread-safe.

Content of the map contained with the class is not. Client can change the entry after getting it. Does it make the whole class thread-safe depends on the logical relationship between this class and V.

Improve this answer

Comments

0

I think it is thread-safe the way it is. You've created (effectively) a copy of the input map and are doing only read-only operations on it. I've sort of always held the belief that immutable classes are inherently thread-safe. But be aware that unless your K and V classes are immutable, you could still run into thread safety issues.

You could also replace it with:

Map<K, V> m = Collections.unmodifiableMap(Collections.synchronizedMap(map));
Improve this answer

Comments