CODEWITHSUNDEEP
javamultithreadingconcurrency
WonderWoman
WonderWoman

Reputation: 164

Increments and decrements in multi threaded environment

I am trying the classic increment/decrement of an int variable in a multi threaded environment. This is my sample code. 

public class SyncIncDec {


    public static void main(String[] args) {

        SyncCounter count = new SyncCounter();

        Thread incThread = new Thread(() -> {
            count.increment();
        });

        Thread decThread = new Thread(() -> {
            count.decrement();
        });

        Thread displayThread = new Thread(() -> {
            System.out.println("Count value : " + count.getX());
        });

        incThread.start();
        decThread.start();
        displayThread.start();      

        try {
            incThread.join();
        } catch (InterruptedException e) {
//          e.printStackTrace();
        }

        try {
            decThread.join();
        } catch (InterruptedException e) {
//          e.printStackTrace();
        }

        try {
            displayThread.join();
        } catch (InterruptedException e) {
//          e.printStackTrace();
        }

    }

}


class SyncCounter {

    private int x=0;

    public SyncCounter() {
        super();
    }

    public SyncCounter(int y) {
        super();
        x = y ;
    }

    synchronized int  getX() {
        return x; 
    }

    void setX(int y) {
        x = y ;
    }

    void increment() {
        ++x;
    }


    void decrement() {
        --x;
    }

}

Though I have used join() method for all the three threads, I still get inconsistent results. Doesn't join here mean for the main thread to wait until each of the thread has completed its execution? I even tried adding synchronized to each of three method signatures; yet I get inconsistent results.

Apart from using Atomic version of the variable, how else can I ensure that I get 0 always?

Upvotes: 0

Views: 1940

Answers (3)

Gray
Gray

Reputation: 116878

Though I have used join() method for all the three threads, I still get inconsistent results. Doesn't join here mean for the main thread to wait until each of the thread has completed its execution?

You have 2 problems going on in your code.

  • In your SyncCounter class, only the getX() method is synchronized. Because you have 3 threads sharing the same instance of this class, any method that is reading or updating the shared fields needs to be synchronized. This means the increment() and decrement() method need to also be synchronized. As mentioned by @Victor, replacing the SyncCounter with an AtomicInteger is an easy solution although I suspect your exercise needs to do it by hand.

    ...
synchronized int increment() {
...
synchronized int decrement() {

  • In your thread model, you have a race condition between the increment and decrement threads, and the display thread. Just because you start the display thread after the others doesn't mean that it runs last. It could be that the display thread finishes first in which case it would print 0 or it could print -1 or 1 depending on the race conditions. The easiest solution here is to have the main thread join with the increment and decrement threads and then it prints out the result.

    // start the inc and dec threads running in the background
incThread.start();
decThread.start();
// wait for inc thread to finish
incThread.join();
// wait for dec thread to finish
decThread.join();
// now we can print out the value of the counter
System.out.println("Count value : " + count.getX());

    If you must have a display thread then it should be started after the joins of the increment and decrement threads like @davidxxx recommends.

    // start the inc and dec threads running in the background
incThread.start();
decThread.start();
// wait for inc thread to finish
incThread.join();
// wait for dec thread to finish
decThread.join();
// now start the display thread now that the increment/decrement is done
displayThread.start();
// wait for the display thread to finish
displayThread.join();

Upvotes: 0

Victor Gubin
Victor Gubin

Reputation: 2937

Your SyncCounter is not thread safe at all. Mutable methods increment and decrement should be synchronized. Now days correct way to implement such a class would be in atomic orations. For example:

class SyncCounter {

    private final AtomicInteger x;

    public SyncCounter() {
     this(0);   
    }

    public SyncCounter(int x) {
       this.x = new AtomicInteger(x);
    }

    int getX() {
        return x.get(); 
    }

    void setX(int x) {
        this.x.set(x);
    }

    int increment() {
        return x.incrementAndGet();
    }


    int decrement() {
        return x.decrementAndGet();
    }

}

And the test code:

    final Thread incThread = new Thread(() -> {
        count.increment();
    });

    final Thread decThread = new Thread(() -> {
        count.decrement();
    });

    Thread displayThread = new Thread(() -> {
        incThread.join();
        decThread.join();
        System.out.println("Count value : " + count.getX());
    });

Upvotes: 2

davidxxx
davidxxx

Reputation: 131346

You invoke join() on the three threads only after all threads were started. So you don't have the guarantee that the thread referenced by the displayThread variable be run after the threads that increment and decrement the counter.
To ensure that, invoke join() on these threads after you started them :

incThread.start();
decThread.start();
incThread.join();
decThread.join();
displayThread.start();

It will block the current thread until incrementing and decrementing is performed and whatever the order as join() were invoked after the start() invocation of these threads.

Upvotes: 4

Related Questions