CODEWITHSUNDEEP
javamultithreadingatomicinteger
LivingRobot
LivingRobot

Reputation: 913

Update AtomicInteger from other thread

I have a class that creates many new objects each on their own thread and I would like to keep a running count across the threads. I wanted to have an AtomicInteger but it's not doing what I expected and instead just gets a smaller version. I'm assuming that this is a race condition error - but I'm not entirely sure.

A created this test example which recreates what I want to do.

public class Test {

    public static void main(String args[]) {
        AtomicInteger total = new AtomicInteger(0);
        for (int i = 0; i < 10; i++) {

            DoThing doThing = new DoThing();

            Thread thread = new Thread(doThing);
            thread.start();
            total.addAndGet(doThing.getTally());
        }

        System.out.println(total.get());
    }
}

class DoThing implements Runnable {

    int tally = 0;
    @Override
    public void run() {

        for(int i = 0; i< 100; i++) {
            tally++;
        }

        System.out.println("Tally is " + tally);

    }

    public int getTally() {
        return tally;
    }
}

However, this outputs: 

Tally is 100
Tally is 100
Tally is 100
Tally is 100
Tally is 100
Tally is 100
Tally is 100
Tally is 100
0
Tally is 100
Tally is 100

When I would like the final output to be 1000. How can I increment across threads?

Thanks in advance.

Upvotes: 0

Views: 2017

Answers (4)

xtratic
xtratic

Reputation: 4699

Try with this:

public static void main(String args[]) {
    AtomicInteger tally = new AtomicInteger(0);
    List<Thread> threadList = new ArrayList<Thread>();
    for (int i = 0; i < 10; i++) {
        Thread t = new Thread(new DoThing(tally));
        t.start();
        threadList.add(t);
    }
    for (Thread t : threadList) {
        try { t.join(); } catch (Exception e){}
    }
    System.out.println("Total tally: " + tally.get());
}

public static class DoThing implements Runnable {
    private static final Random rand = new Random();
    private final AtomicInteger tally;

    public DoThing(AtomicInteger tally) {
        this.tally = tally;
    }

    @Override public void run() {
        for (int i = 0; i < 100; i++) {
            int currTally  = tally.incrementAndGet();
            System.out.println("Thread " + Thread.currentThread().getName() + ": " + currTally);
            // Random sleep to show that your threads are properly concurrently incrementing
            try { Thread.sleep(rand.nextInt(10)); } catch (Exception e) {}
        }
    }
}

The root of your problem is that you are misunderstanding how to use AtomicInteger, you are treating it just like a normal int and it is not being accessed concurrently at all.

Also getTally() is a race condition until you assure that the thread has completed by calling Thread.join().

So you can keep an up-to-date tally by having all the Runnables in the threads update the same AtomicInteger instance and you can ensure that you have the right total by waiting for all the threads to finish their tallies by join()ing them before getting the tally.

Upvotes: 2

Solomon Slow
Solomon Slow

Reputation: 27115

Yes, there is a data race. The race is between the main thread calling doThing.getTally() and the "worker" thread starting up. Looks like each time your main thread is reliably able to get the "tally" from each worker before the worker even gets a chance to enter its for loop. It could be happening even before the worker calls its run() method.

Your main thread needs to join the workers:

  • Create and start the ten worker threads, and add each one to a List<Thread>.
  • Then in a separate loop, call t.join() for each thread t in the list. The t.join() function waits for the thread t to finish its job.
  • Finally, get the tallies and add them up.

Upvotes: 2

ekaerovets
ekaerovets

Reputation: 1168

CountDownLatch latch = new CountDownLatch(10);
List<DoThing> things = new ArrayList();
AtomicInteger total = new AtomicInteger(0);
    for (int i = 0; i < 10; i++) {

        DoThing doThing = new DoThing(latch);
        things.add(doThing);
        Thread thread = new Thread(doThing);
        thread.start();
        total.addAndGet(doThing.getTally());
    }
// Wait till all the threads are done.
// Each thread counts the latch down
latch.await()
int total = 0;

// Calculate sum after all the threads are done.
for (DoThing thing: things) {
    total += thing.getTally();
}
System.out.println(total);



class DoThing implements Runnable {

    private CountDownLatch latch;

    public DoThing(CountDownLatch latch) {
        this.latch = latch;
    }

    int tally = 0;
    @Override
    public void run() {

        for(int i = 0; i< 100; i++) {
            tally++;
        }
        latch.countDown();
        System.out.println("Tally is " + tally);

    }

    public int getTally() {
        return tally;
    }
}

Upvotes: 1

miskender
miskender

Reputation: 7938

In your example code, total is only accessed from main thread. Making it Atomic won't have any impact on the result. You should pass Atomic value to your threads and increase the value in there. Or use LongAdder (increment method).
And you have to wait all threads to finish before printing Atomic value in main thread.
If you want to use low level blocking, you can use CyclicBarrier to make main thread wait all threads.

Upvotes: 2

Related Questions