CODEWITHSUNDEEP
javamultithreading
ling
ling

Reputation: 1675

Is this the right way to count in multithreads?

I would like to get all counts from WorkerThread class after the 5 threads finished. Does the Counter work the way as expected? Thanks. In the WorkerThread constructor, it seems each counter becomes one variable of each thread, not shared among the 5 threads. 

public class Test {

    public static void main(String[] args) {
        ExecutorService executor = Executors.newFixedThreadPool(5);
        Counter counter = new Counter();
        for (int i = 0; i < 10; i++) {
            Runnable worker = new WorkerThread(String.valueOf(i), counter);
            executor.execute(worker);
          }
        executor.shutdown();
        System.out.println("Total Count: " + counter.value());
        while (!executor.isTerminated()) {
        }
        System.out.println("Finished all threads");
    }

}

class Counter{
    private AtomicInteger count;

    public Counter(){
        count = new AtomicInteger();
    }

    public int increment(){
        return count.incrementAndGet();
    }

    public int value(){
        return count.get();
    }
}

class WorkerThread implements Runnable {

    private String command;
    private Counter c;

    public WorkerThread(String s, Counter c){
        this.command=s;
        this.c = c;
    }

    @Override
    public void run() {
        for(int i=0; i<6; i++){         
            c.increment();
        }
        processCommand();       
    }

    private void processCommand() {
        try {
            Thread.sleep(5000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    @Override
    public String toString(){
        return this.command;
    }
}

Upvotes: 1

Views: 116

Answers (1)

isnot2bad
isnot2bad

Reputation: 24454

Synchronization seems correct to me. If counting is your only task, you could skip the Counter class and directly use AtomicInteger to remove complexity. (Keep it if you want to realize more complex tasks which require a proper synchronized shared state object).

Don't call your worker class "Thread". It is not a thread, it is a task that is executed by a thread.

Last but not least, there's something wrong with your shutdown-code. Note that shutdown does not block but return immediately. So you're evaluating your counter too early.

The following code fixes all of this issues:

public class Test {
    public static void main(String[] args) {
        ExecutorService executor = Executors.newFixedThreadPool(5);
        AtomicInteger counter = new AtomicInteger();

        for (int i = 0; i < 10; i++) {
            executor.execute(new Worker(counter));
        }

        executor.shutdown();
        if (!executor.awaitTermination(60, TimeUnit.SECONDS)) {
            // executor does not shut down.
            // try executor.shutdownNow() etc.
        }

        System.out.println("Total Count: " + counter.get());
    }
}

class Worker implements Runnable {
    private final AtomicInteger counter;

    public Worker(AtomicInteger counter) {
        this.counter = counter;
    }

    public void run() {
        // do something
        counter.incrementAndGet();
        // do something else
    }
}

For more information about how to correctly shutdown an ExecutorService, see http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ExecutorService.html, section 'Usage Examples'.

Upvotes: 1

Related Questions