AtomicInteger doesn't increment concurrently

Question

I am testing the use of the class AtomicInteger but increments doesn't seem to be applied under mutual exclusion control.

Here's my test:

static class AtomicIntegerRunnable implements Runnable
{
    private static AtomicInteger x;

    AtomicIntegerRunnable() {}

    AtomicIntegerRunnable(AtomicInteger x) 
    {
        this.x = x;
    }

    @Override
    public void run()
    {
        System.out.println(x.get());
        x.getAndIncrement();
    }
}

public static void main(String[] args) {
    ExecutorService e = Executors.newFixedThreadPool(n_whatever);
    AtomicInteger x = new AtomicInteger();
    int n = 10;
    for (int i=0; i<n; i++) {
        e.submit(new AtomicIntegerRunnable(x));
            }
    e.shutdown();
    while (!e.isTerminated());
}

In the print I am getting something like

0 0 1 1 1 5 4 3 2 6

instead of

0 1 2 3 4 5 6 7 8 9

. What's wrong?

EDIT for @Louis Wasserman

static class AtomicIntegerRunnable implements Runnable
{
    private AtomicInteger x;

    AtomicIntegerRunnable() {}

    AtomicIntegerRunnable(AtomicInteger x) 
    {
        this.x = x;
    }

    @Override
    public void run()
    {
        x.getAndIncrement();
    }
}

public static void main(String[] args) 
{
    ExecutorService e = Executors.newFixedThreadPool(n_whatever);
    AtomicIntegerRunnable x = new AtomicIntegerRunnable(new AtomicInteger());
    int n = 10;
    for (int i=0; i<n; i++) 
        e.submit(x);
    e.shutdown();
    while (!e.isTerminated());
}

Instead of posting "something like" the output you got, can you copy/paste actual output? — user2357112, Jan 13 '14 at 21:42
`while (!e.isTerminated());` is a terrible pattern. You should use `e.awaitTermination(Long.MAX_VALUE, TimeUnit.MILLISECONDS);` — Gray, Jan 13 '14 at 21:46
@user2357112 it is not atomic http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/atomic/AtomicInteger.html#get() — dabadaba, Jan 13 '14 at 22:05
@dabadaba: You should just delete the word `static`. Each `AtomicIntegerRunnable` should have a separate reference to the `AtomicInteger`, based on how you've done things in your constructor; setting `static` variables in your constructor is more or less nonsensical. — Louis Wasserman, Jan 13 '14 at 22:16
@dabadaba You'll notice that the only operations described as "atomic" in the docs are those with *two* actions -- that's because operations with only one action are "atomic" by nature. Those with two (or more) operations must be specially built to act atomically. — Nicole, Jan 13 '14 at 22:19
@LouisWasserman I don't see it. Each `AtomicIntegerRunnable` MUST have the SAME reference to the `AtomicInteger` "x" I declare, else the runnable class would be working with different objects. — dabadaba, Jan 13 '14 at 22:19
@dabadaba You would have the same reference because you are passing it into the constructor: `new AtomicIntegerRunnable(x)`. You would have 10 different objects each with their own member field pointing to the same instance of AtomicInteger. Right now you only have 1 reference pointing to 1 field. I believe Louis is just saying it's weird (and not useful) to share them statically when you're already passing that reference to each instance. — Nicole, Jan 13 '14 at 22:21
@dabadaba: Like I said, it works in this case, but you're mixing idioms weirdly. You could avoid having a constructor on `AtomicIntegerRunnable` and just write `AtomicIntegerRunnable.x = new AtomicInteger()` once. Heck, you could only create one `AtomicIntegerRunnable` and submit the same runnable `n` times. — Louis Wasserman, Jan 13 '14 at 22:22
The difference would show up if you ever wanted to have two different AtomicInteger counters and two different pools of threads incrementing them (i.e. a page counter and a unique IP address counter). With the code you have, you couldn't -- you'd edit the same instance. With @LouisWasserman's suggestion, you could. — Nicole, Jan 13 '14 at 22:23
I am trying to do what you guys say but it gives `NullPointerException`, only my way works... I still don't get why what I'm doing is wrong: I create one `AtomicInteger` object and pass it to the submitted runnable class because the same reference to the object is needed. And then `static` because the object is shared by all of the runnable class objects. — dabadaba, Jan 13 '14 at 22:32
Can you show us your modified version that's generating an NPE? — Louis Wasserman, Jan 13 '14 at 22:48
@dabadaba Do you see what's different between your own two examples? You are now creating only one `AtomicIntegerRunnable`. Go back to your previous code and *only delete the word `static`*. Someone must have explained to you that "static means shared by all objects" but it's *not the only way to share objects*. You don't need it here, they are already shared because you passed the same object to every constructor. — Nicole, Jan 14 '14 at 01:57

Nicole · Accepted Answer · 2014-01-13T21:49:10.877

11

You are getting it and incrementing it in two separate actions. You are printing out the first, which makes your own code non-atomic.

Each AtomicInteger operation is atomic on its own, but you cannot use two in sequence and consider the combination atomic -- by defintion, two operations are not atomic.

To fix this:

@Override
public void run()
{
    System.out.println(x.getAndIncrement());
}

However, as pointed out in the comments, you have a third action, the printing. This is also not atomic with the increment action. So, you would need to synchronize the two in order to make your list print out in order.

You could call that method printAndIncrement.

Usually AtomicInteger is used more to ensure that multithreaded code never accidentally operates on the same integer, not to ensure that an external system (like an output buffer) sees those increments in order (because you still need something else to synchronize communication with that external system).

edited Jan 13 '14 at 21:49

answered Jan 13 '14 at 21:39

Nicole

32,841
11
75
101

5

They still won't necessarily be ordered though. – Sotirios Delimanolis Jan 13 '14 at 21:39
This answer doesn't explain the part where the output counts down. – user2357112 Jan 13 '14 at 21:41
@SotiriosDelimanolis Maybe I'm having a brain blink, but I can't see why not? It prints and *always increments*. That means that the next time it prints it will be a higher number. The only other varying factor is multiple output buffers, which I haven't tested to see if will factor in on such a small scale. – Nicole Jan 13 '14 at 21:44
5

The printing is a separate action from the getting. So thread one might get value `1`, context switch, thread two gets value `2` and prints first, then thread one prints. – Sotirios Delimanolis Jan 13 '14 at 21:44
@SotiriosDelimanolis Fixed -- does my revised explanation cover this adequately? – Nicole Jan 13 '14 at 21:50
Yeah +1. The whole idea is that there are (were) three methods that where each individually atomic. – Sotirios Delimanolis Jan 13 '14 at 21:51
@Sotirios, *"three methods that where each individually atomic"*, print functions are not atomic in most languages. So I'm guessing `println()` is not atomic in Java. Or is it? – Qtax Jan 14 '14 at 15:28
@Qtax `System.out` is a `PrintStream` which internally synchronizes on itself in all its write methods. – Sotirios Delimanolis Jan 14 '14 at 15:32

AtomicInteger doesn't increment concurrently

1 Answers1

Linked