AtomicInteger incrementation not behaving as expected

Question

I was reading about AtomicInteger and how its operations are atomic and how these properties make it useful for multithreading.

I wrote the following program to test the same.

I am expecting the final size of the set should be 1000, since each thread loops 500 times and assuming each time a thread calls getNext() it should get a unique number.

But the output is always less than 1000. What am i missing here?

public class Sequencer {

private final AtomicInteger i = new AtomicInteger(0);

public int getNext(){
    return i.incrementAndGet();
}

public static void main(String[] args) {

    final Sequencer seq = new Sequencer();

    final Set<Integer> set = new HashSet<Integer>();

    Thread t1 = new Thread(new Runnable() {
        @Override
        public void run() {
            for (int i=0; i<500; i++)
                set.add(seq.getNext());

        }
    },"T1");
    t1.start();


    Thread t2 = new Thread(new Runnable() {
        @Override
        public void run() {
            for (int i=0; i<500; i++)
                set.add(seq.getNext());

        }
    },"T2");

    t2.start();

    try {
        t1.join();
        t2.join();
    } catch (InterruptedException e) {
        e.printStackTrace();
    }

    System.out.println(set.size());

}

}

TwoThe · Accepted Answer · 2014-01-26T01:01:37.197

3

You are missing that HashSet is not thread-safe. In addition the properties of a set would erase all duplicated numbers, so your test would fail if AtomicInteger was not thread-safe.

Try using a ConcurrentLinkedQueue instead.

Edit: Because it has been asked twice: Using a synchronized set works, but it destroys the idea behind using a lock-free algorithm like the Atomic-classes. If in your code above you replace the set with a synchronized set, then the threads will have to block each time add is called.

This will effectively reduce your application to single-threaded, because the only work done happens synchronized. Actually it will even be slower than single-threaded, because synchronized takes its toll as well. So if you want to actually utilize threading, try to avoid synchronized at all cost.

edited Jan 26 '14 at 01:01

answered Jan 25 '14 at 18:27

TwoThe

13,879
6
30
54

I tried this - `final Set set = Collections.synchronizedSortedSet(new TreeSet());` and it worked. Thanks guys. – Kapil Raju Jan 25 '14 at 19:15
I got your point. Idea of using Set above was to test a) if `AtomicInteger` `incrementAndGet()` is atomic , b) If operation is atomic, set will return me size() 1000 if they are unique. But i didn't realize the Hashset operations are not thread safe and will cause this issue. When i switched to synchronizedSortedSet, i tested with both AtomicInteger and primitive int and found that with AtomicInteger the count was always 1000 and with primitive int it was not consistent. But i agree with your point that if you club a thread safe operation with unnecessary synchronization it beats the purpose. – Kapil Raju Jan 26 '14 at 12:27

score 1 · Answer 2 · answered Jan 25 '14 at 18:47

1

HashSet is not thread safe so you are facing problem.You can use Vector or any collection class which is thread safe or run two thread sequentially if you stricly need to use HashSet.

t1.start();
t1.join();

t2.start();
t2.join();

answered Jan 25 '14 at 18:47

Kick

4,823
3
22
29

if i run two threads sequentially it will beat the purpose of my test :) – Kapil Raju Jan 26 '14 at 12:31

score 0 · Answer 3 · answered Apr 01 '15 at 03:33

As mentioned in several answers, it fails due to HashSet not being thread safe.

First, lets verify for the sake of your test, that AtomicInteger is indeed thread-safe then proceed to see why your test failed. Modify your test slightly. Use two hashsets, one for each thread. Finally, after joins, merge the second set into the first, by iterating over the second and adding it to the first which will eliminate duplicates(set property). Then do a count on the first set. The count will be what you expect. This proves that it is HashSet that is not thread safe and not the AtomicInteger.

So lets look at what aspect is not thread safe. You're doing onlyf add()s, so clearly add() is the operation that is not thread safe causing the loss of numbers. Lets look at an example pseudo-code non-thread safe HashMap add() that would lose numbers(this is obviously not how it implemented, just trying to state one way in which it could be non-thread safe):

class HashMap {
 int valueToAdd;
 public add(int valueToAdd) {
   this.valueToAdd = valueToAdd;
   addToBackingStore(this.valueToAdd);
 }
}

If multiple threads call add() and they all reach the addToBackingStore() after they've changed this.valueToAdd, only the final value of valueToAdd is added, all other values are overwritten and lost.

Something similar to this is probably what happened in your test.

score -1 · Answer 4 · answered Jan 25 '14 at 18:35

-1

Try do it in that way using Collections synchronized.

public class Sequencer {

    private final AtomicInteger i = new AtomicInteger(0);

    public static void main(String[] args) {

        final Sequencer seq = new Sequencer();

        final Set<Integer> notSafe = new HashSet<Integer>();
        final Set<Integer> set = Collections.synchronizedSet(notSafe);
        Thread t1 = new Thread(new Runnable() {
            @Override
            public void run() {
                for (int i = 0; i < 500; i++)
                    set.add(seq.getNext());

            }
        }, "T1");
        t1.start();


        Thread t2 = new Thread(new Runnable() {
            @Override
            public void run() {
                for (int i = 0; i < 500; i++)
                    set.add(seq.getNext());

            }
        }, "T2");

        t2.start();

        try {
            t1.join();
            t2.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        System.out.println(set.size());

    }

    public int getNext() {
        return i.incrementAndGet();
    }
}

answered Jan 25 '14 at 18:35

RMachnik

3,598
1
34
51

1

While a synchronized set would work, it totally destroys the reason to use atomic __lock-free__ algorithms. – TwoThe Jan 25 '14 at 18:45
Why? I think it will not have any affect on that first of all set can hold only unique values so if you have there were not atomicInteger you would have also not good sync even if there will be synchronizedSet, because sometimes it will have add 1 and add 1 so set will not allow you to add two same values at all. So I think concurrentHashSet will not broke that conception of that question. – RMachnik Jan 25 '14 at 19:32
I have no clue what you are saying, sorry. But I added an edit to my post, maybe that helps. – TwoThe Jan 26 '14 at 01:02
"While a synchronized set would work, it totally destroys the reason to use atomic lock-free algorithms." -> It will not destroy it. You have to use atomic int or some kind of lock because '++' is not atomic operation :) Another thing is that were you store your data. If you use atomic int and store results in not synchronized container it will also not work at all, you have to use all synchronized and thread safe stuff if all algorithm should be thread safe and work in multi thread env. :) So basically speaking your first commend below my answer is useless and pointless. – RMachnik Jan 26 '14 at 09:54

AtomicInteger incrementation not behaving as expected

4 Answers4