Should we always use a ConcurrentHashMap when using multiple threads?

Question

If I have a hash map and this method:

private Map<String, String> m = new HashMap<>();

private void add(String key, String value) {
    String val = m.get(key);
    if (val == null) {
        m.put(key, value);
    }
}

If I have two threads A and B calling the method with the same key and value, A and B may both see that the key is not in the map, and so may both write to the map simultaneously. However, the write order (A before B or B before A) should not affect the result because they both write the same value. But I am just wondering whether concurrent writes would be dangerous and could lead to unexpected results. In that case I should maybe use a ConcurrentHashMap.

What you have there is definitely wrong. `add()` needs to be synchronized, and `m` needs to be `final` or `volatile`. A `ConcurrentHashMap` won't fix those issues. — markspace, Mar 01 '21 at 18:43
It's just that in my case, they would always write the same value for each key, so I don't think synchronized is necessary. Good point about final and volatile, but I saw this and maybe final is enough and we don't need volatile? https://stackoverflow.com/questions/29404851/do-we-need-to-make-concurrenthashmap-volatile — alfer, Mar 01 '21 at 18:47
@Dorian Gray : But I would still need it to be final for memory visibility? — alfer, Mar 01 '21 at 18:51
If I do use synchronized, do I even need a ConcurrentHashMap? — alfer, Mar 01 '21 at 18:59
No, if you use `final` and make `add()` synchronized, it will work. `ConcurrentHashMap` probably offers better performance under heavy load (many threads) however. — markspace, Mar 01 '21 at 19:07
If I use synchronized I think we don’t even need final for memory visibility? — alfer, Mar 01 '21 at 20:42
As long as you don't reassign the map, it will not make a difference, if that's your point. Anyway, instead of using synchronized, I recommend using putIfAbsent instead — Dorian Gray, Mar 02 '21 at 08:01
Yes that was my point, I just wanted to make sure and I agree it’s better to use putIfAbsent — alfer, Mar 02 '21 at 08:02

Dorian Gray · Answer 1 · 2021-03-06T20:16:54.020

0

Yes, you should use a ConcurrentHashMap (which is internally thread-safe), and use the m.putIfAbsent(key, value) of it.

m should also be final, to avoid that it is being reassigned.

edited Mar 06 '21 at 20:16

answered Mar 01 '21 at 18:42

Dorian Gray

2,913
1
9
25

2

`m` needs to be `final` or `volatile` as well. – markspace Mar 01 '21 at 18:43
2

@markspace only `final`. There would be no point in declaring it `volatile`. – Holger Mar 02 '21 at 13:18
@Holger Either could be used, and the OP might not have shown us full code. `m` could be replaced at some point in the code. But the point is that *either could be used,* not that we should tell the OP to "only use `final.`" – markspace Mar 02 '21 at 13:59
2

@markspace if `m` gets reassigned, the code is broken by design, in 99% of all cases. In the other cases, `volatile` is not needed as whatever construct has been used to ensure logical correctness between code working with the old map and the code working with the new map will be sufficient. – Holger Mar 02 '21 at 16:03
I just want to point out to anyone reading this that that's a bunch of twaddle. `m` is not even guaranteed to be visible the first time it is assigned, exactly as the OP has above. This object is not thread safe as the OP has presented it. No additional "logical correctness code" needs to be assumed. The replacement code doesn't matter because if `m` is changed later for any reason, `m` right now must be `volatile` or the first assignment, exactly as the OP has above, isn't thread safe. (Please remember that constructors cannot be synchronized.) – markspace Mar 02 '21 at 16:08
2

@markspace it seems you are assuming an improper (racy) publication of the object containing `m`, as otherwise, there would be no need for changing `m`’s modifier. But in that case, declaring it `volatile` would not help, as that would only ensure the visibility of writes preceding the `volatile` write, but not prevent the premature visibility of the racy subsequent write. Note that you *can* use `synchronized(this)` within the constructor, but it would be as pointless as `volatile`, it can not prevent other threads from seeing the object too early, only `final` can do that. – Holger Mar 03 '21 at 15:24
The write to initialize `m` is a field initialization, it's done during object construction. I'm not sure what to say other than that's a classic and 100% appropriate use of `volatile` for safe publication. @Holger – markspace Mar 03 '21 at 15:39
2

@markspace so you don’t understand the JMM at all, but keep insisting on something, because “that's a classic” and then, you’re mixing it up. The safe publication pattern is to make the field referencing the constructed object `volatile`, not the constructed object’s member field. That’s a fundamental difference. – Holger Mar 03 '21 at 15:42
@Holger I am definitely unclear on the difference between a "field referencing the constructed object" and "the constructed object's member field". I call that a distinction without a difference. The object in question here is the `HashMap` that `m` refers to. That object and the field `m` may not be visible if `m` is not declared `volatile`. If `add()` is called above by a thread other than the constructing one then `m` may not be visible, and any fields within the associated `HashMap` may not be visible. – markspace Mar 03 '21 at 16:01
@markspace"field referencing the constructed object" is the reference to m. There is no point in declaring it volatile, because that only refers to the reference itsself, not to the internal state of that object. In regards to that last sentence of yours: `volatile` doesn't guarantee that, as it doesn't affect "any fields within the associated HashMap", `volatile` comes into place when you assign a different object to m. – Dorian Gray Mar 06 '21 at 20:16
@DorianGray No, check out any good source on the Java memory model. Assignment to `volatile` establishes *happens-before* for that write and for all previous writes too. Therefore the internal state of the object gets published for any other thread reading the `m` variable. For initial assignments, this is called "safe publication". I'm pretty sure Holger is arguing something else (which I disagree with). – markspace Mar 07 '21 at 02:04
No, it's not, and thats exactly what Holger wrote. Happens before only refers to the reference, not the internal state of rhe objct itsself. E.g. https://stackoverflow.com/questions/36200870/does-a-volatile-reference-really-guarantee-that-the-inner-state-of-the-object-is – Dorian Gray Mar 07 '21 at 07:29
In that comment you posted about reads and writes: Those are reads and writes of the variable itsself. Calling a method on a reference is not a write in terms of the JMM. – Dorian Gray Mar 07 '21 at 07:37

Should we always use a ConcurrentHashMap when using multiple threads?

1 Answers1