1

Following up on this question (Java thread safety - multiple atomic operations?), I don't want to add more questions to it, but now I have this doubt:

private final Map<String, Set<String>> data = Maps.newConcurrentMap();

... then in a method ...

if (data.containsKey("A")) {
    data.get("A").add("B");
}

It should be something like this:

synchronized(data) {
    if (data.containsKey("A")) {
        data.get("A").add("B");
    }
}

In order to be thread-safe. Is that correct?

So operations are atomic, but combining them would require synchronisation, is that right? At that point, would it make sense to just use a simple HashMap instead of a concurrent one, as we're manually handling sync?

Is there any method in CHM to make this work atomically?

Community
  • 1
  • 1
Will
  • 181
  • 1
  • 14
  • One advantage over simple HashMap is that CHM still does not require synchronization for the readers. – Thilo Jan 13 '17 at 01:42
  • Please also show the code that modifies the CHM. The above code is read-only (only calls `get` and `containsKey`) as far as the Map is concerned. – Thilo Jan 13 '17 at 01:44
  • @Thilo just imagine the `put` method is used by multiple threads. Anyway what I'm interested in is if I need that `synchronised` in order to make it thread-safe. Also, if there's any construct in CHM to help me avoid that sync, and do a `map.addValueIfKeyPresent` (I know that method doesn't exist) – Will Jan 13 '17 at 01:50
  • Possible duplicate of [Should you check if the map containsKey before using ConcurrentMap's putIfAbsent](http://stackoverflow.com/questions/3752194/should-you-check-if-the-map-containskey-before-using-concurrentmaps-putifabsent) – shmosel Jan 13 '17 at 01:51
  • If there is no `remove` method, you maybe don't need synchronization code at all. Adding something to a `Set` twice is a no-op anyway. Is that `Set` thread-safe? – Thilo Jan 13 '17 at 01:53
  • 1
    Isnt computeIfPresent made for this? – Nathan Hughes Jan 13 '17 at 01:58
  • @Thilo there is other threads removing, unfortunately... So I guess I do need a CHM. Yes, the Set is thread safe too. Is my code then thread-safe without requiring `synchronised`? – Will Jan 13 '17 at 01:58
  • 1
    May I ask what kind of set you're using that's thread safe? – shmosel Jan 13 '17 at 02:07
  • 2
    @shmosel I'm using this: https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/collect/Sets.html#newConcurrentHashSet-- – Will Jan 13 '17 at 02:09
  • You can use AtomicReference : https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/AtomicReference.html – Ravindra babu Jan 13 '17 at 14:27

2 Answers2

2

In your specific case, you might want to use computeIfPresent method of ConcurrentHashMap:

data.computeIfPresent("A", (k, v) -> { v.add("B"); return v; } );

From the javadocs:

If the value for the specified key is present, attempts to compute a new mapping given the key and its current mapped value. The entire method invocation is performed atomically.

So there's no need for explicit synchronization.

fps
  • 33,623
  • 8
  • 55
  • 110
  • That's great, so there is a method in CHM for this.... Great! Still I have the question: if that method didn't exist, would my first code be OK? Or would I need the second one with explicit sync? – Will Jan 13 '17 at 02:03
  • Usually `computeIfAbsent()` is what you want. It initializes the value if it's absent, then returns it. But it's not entirely clear what you need here. – shmosel Jan 13 '17 at 02:04
  • @Will I think you'd need to synchronize explicitly, but the best approach would be the one suggested by @shmosel: `data.computeIfAbsent("A", k -> Sets.newConcurrentHashSet()).add("B");` – fps Jan 13 '17 at 02:15
  • 2
    I'm worried about the (mis)use of `computeIfPresent()` to mutate the value instead of replacing it. I'm not sure if the atomicity guarantee would still hold true in this case. – shmosel Jan 13 '17 at 02:15
  • @FedericoPeraltaSchaffner: `computeIfAbsent` will add a new Set. The original code only added to an existing set. – Thilo Jan 13 '17 at 02:17
  • 1
    @FedericoPeraltaSchaffner why is that better? I don't want to do anything if the element is absent, I only want to do it if the element is present. @shmosel why do you say it's better to do `computeIfAbsent`? – Will Jan 13 '17 at 02:17
  • 1
    @shmosel: The atomicity guarantee still holds, but it only extends to the Map. The Set needs to be threadsafe as well (no matter how). – Thilo Jan 13 '17 at 02:18
  • The way I'm adding to the map (using multiple threads): `data.put("A", Sets. newConcurrentHashSet());`. Makes sense? – Will Jan 13 '17 at 02:21
  • @Will You are OK, disregard my comment. It's just that I've always used `computeIfAbsent` and have never needed to use `computeIfPresent`, so it looks a little bit awkward to me. But if that's your use case, then go ahead with it! – fps Jan 13 '17 at 02:21
  • 1
    @Will: You probably want `putIfAbsent` if you add from multiple threads. Otherwise you might overwrite an existing (and populated) set. – Thilo Jan 13 '17 at 02:24
  • @Thilo I was about to say exactly that – fps Jan 13 '17 at 02:25
  • 1
    Guys thanks a lot to all of you, I've learnt a lot. I think this is the right answer, so I'll accept it. But thanks a lot @Thilo for your great help, I really appreciate it! Same goes for @shmosel! – Will Jan 13 '17 at 02:27
1
synchronised(data) {
    if (data.containsKey("A")) {
        data.get("A").add("B");
    }
}

You probably need to show more code.

Looking only at this, the only possible issue is that someone removes the Set found at "A" after your if check. If you don't ever remove map entries you need no synchronization at all.

If you do remove map entries concurrently, you could use computeIfPresent to arrive at the updated map.

You could also do 

Set<String> set = data.get("A");
if (set != null) set.add("B");

Since you are not actually producting a new Set, I find this more idiomatic than computeIfPresent (which should compute a new value).

Note that you need to make all these Sets thread-safe as well.

Thilo
  • 257,207
  • 101
  • 511
  • 656
  • 1
    `containsKey()` and `get()` are still redundant operations. I would do `Set set = data.get("A"); if (set != null) set.add("B");`... Beat me to it :) – shmosel Jan 13 '17 at 02:06
  • @shmosel: Me too. Seems better than `computeIfPresent` as well (because no new value is computed, but the existing value is mutated). – Thilo Jan 13 '17 at 02:07