CODEWITHSUNDEEP
javamultithreadingconcurrencyjava.util.concurrentconcurrenthashmap
AkshaiShah
AkshaiShah

Reputation: 5939

Adding to AtomicInteger within ConcurrentHashMap

I have the following defined

private ConcurrentMap<Integer, AtomicInteger>  = new ConcurrentHashMap<Integer, AtomicInteger>();

private void add() {
  staffValues.replace(100, staffValues.get(100), new AtomicInteger(staffValues.get(100).addAndGet(200)));
}

After testing, the values I am getting are not expected, and I think there is a race condition here. Does anyone know if this would be considered threadsafe by wrapping the get call in the replace function?

Upvotes: 0

Views: 3348

Answers (3)

Ferrybig
Ferrybig

Reputation: 18834

A good way to handle situations like this is using the computeIfAbsent method (not the compute method that @the8472 recommends)

The computeIfAbsent accepts 2 arguments, the key, and a Function<K, V> that will only be called if the existing value is missing. Since a AtomicInteger is thread safe to increment from multiple threads, you can use it easely in the following manner:

staffValues.computeIfAbsent(100, k -> new AtomicInteger(0)).addAndGet(200);

Upvotes: 4

ruakh
ruakh

Reputation: 183456

There are a few issues with your code. The biggest is that you're ignoring the return-value of ConcurrentHashMap.replace: if the replacement doesn't happen (due to another thread having made a replacement in parallel), you simply proceed as if it happened. This is the main reason you're getting wrong results.

I also think it's a design mistake to mutate an AtomicInteger and then immediately replace it with a different AtomicInteger; even if you can get this working, there's simply no reason for it.

Lastly, I don't think you should call staffValues.get(100) twice. I don't think that causes a bug in the current code — your correctness depends only on the second call returning a "newer" result than the first, which I think is actually guaranteed by ConcurrentHashMap — but it's fragile and subtle and confusing. In general, when you call ConcurrentHashMap.replace, its third argument should be something you computed using the second.

Overall, you can simplify your code either by not using AtomicInteger:

private ConcurrentMap<Integer, Integer> staffValues = new ConcurrentHashMap<>();

private void add() {
    final Integer prevValue = staffValues.get(100);
    staffValues.replace(100, prevValue, prevValue + 200);
}

or by not using replace (and perhaps not even ConcurrentMap, depending how else you're touching this map):

private Map<Integer, AtomicInteger> staffValues = new HashMap<>();

private void add() {
    staffValues.get(100).addAndGet(200);
}

Upvotes: 2

the8472
the8472

Reputation: 43107

You don't need to use replace(). AtomicInteger is a mutable value that does not need to be substituted whenever you want to increment it. In fact addAndGet already increments it in place.

Instead use compute to put a default value (presumably 0) into the map when none is present and otherwise get the pre-existing value and increment that.

If, on the other hand, you want to use immutable values put Integer instances instead of AtomicInteger into the map and update them with the atomic compute/replace/merge operations.

Upvotes: 0

Related Questions