0

My expectation was that the program would return the output "1000".

However, every time the program performs a different output occurs.

The increment() method of the Counter class has been synchronized. Would only this be necessary to avoid competing readings?

What would be the correct way to make it count the 1000 increments?

package app;

public class Counter {

    private Integer value;

    public Counter(int initialValue) {
        value = initialValue;
    }

    public synchronized void increment() {
        value = value + 1;          
    }

    public int getValue() {
        return value;
    }

}

package app;

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class Main {

    public static void main(String[] args) {

        Counter contador = new Counter(0);

        ExecutorService executor = Executors.newFixedThreadPool(10);

        for(int i = 0; i < 1000; i++) {
            executor.submit(() -> {
                contador.increment();   
            });     
        }

        System.out.println(contador.getValue());

    }

}

3 Answers3

2

Your logic is fine.

The problem is that you print the value of the counter before the threads finished incrementing it.

Change your code to this:

for(int i = 0; i < 1000; i++) {
    executor.submit(() -> {
        contador.increment();   
    });     
}

executor.shutdown(); //Shut down the executor

//Wait until the threads have stopped. A maximum of 1 minute is more than enough
executor.awaitTermination(1, TimeUnit.MINUTES); 

System.out.println(contador.getValue()); //prints 1000
Malt
  • 28,965
  • 9
  • 65
  • 105
  • Or use `executor.shutdown();while(!executor.isTerminated()) {}` to block until all tasks are done without an arbitrary time waiting. – Mena Oct 26 '17 at 15:32
  • Got it. Thank you. –  Oct 26 '17 at 15:34
  • @Mena `executor.shutdown();while(!executor.isTerminated()) {}` is a bad idea. It will cause busy waiting and high CPU usage. `awaitTermination` is better. – Malt Oct 26 '17 at 15:41
  • @Malt I agree, it's a very bad idea. My point here was only to offer a cheap test solution, as awaiting for termination with an arbitrary timeout may not satisfy the time requirements for arbitrary tasks to complete. The proper way to aggregate increments from different threads would probably not consist in using an `ExecutorService` in the first place. – Mena Oct 26 '17 at 15:49
1

The answer to your question is that you're not waiting for all your Runnables to finish before printing the value of contador.

So sometimes only 20 calls to increment have been done called and sometimes 50 have been done, etc

Edit:

Looking closer, I think you have a potential issue with your thread safety. You've synchronized the increment value. Should the getValue method match?

For example, if Thread A is in the process of incrementing, what should Thread B see when it calls getValue? Should it see the old value, or should it wait until Thread A is done so it gets the latest value?

tfecw
  • 437
  • 1
  • 6
  • 11
0

This is how your main method should look: 

public static void main(String[] args) {
    Counter contador = new Counter(0);
    ExecutorService executor = Executors.newFixedThreadPool(10);
    for(int i = 0; i < 1000; i++) {
        executor.submit(() -> {
            contador.increment();
        });
    }
    executor.shutdown();
    try {
        executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
    } catch (InterruptedException e) {
    }
    System.out.println(contador.getValue());
}
Yaroslav
  • 446
  • 4
  • 15