CODEWITHSUNDEEP
java
Tavo Meile
Tavo Meile

Reputation: 1

java program is not finishing for loop why?

I do not understand why each time I run this code I get different answer ? Correct answer should be one 98098 two 98099. Anyone have any ideas why this is not working that way ? For example one time answer comes back "one 49047 two 49047" then another time it comes back "one 40072 two 40072". I am so confused at this point and lack any reasonable explanation

public class TestThreads {
    public static void main(String[] args){

        ThreadOne t1 = new ThreadOne();
        ThreadTwo t2 = new ThreadTwo();

        Thread one = new Thread(t1);
        Thread two = new Thread(t2);

        one.start();
        two.start();

    }
}

class ThreadOne implements Runnable {
    Accum a = Accum.getAccum();
    public void run(){
        for(int x = 0; x < 98; x++){
            a.updateCounter(1000);
            try{
                Thread.sleep(50);
            }catch(InterruptedException ex){

            }
        }
        System.out.println("one " + a.getCount());
    }

}

class ThreadTwo implements Runnable {
    Accum a = Accum.getAccum();
    public void run(){
        for(int x = 0; x < 99; x++){
            a.updateCounter(1);
            try{
                Thread.sleep(50);
            }catch(InterruptedException ex){

            }
        }
        System.out.println("two " + a.getCount());
    }

}

class Accum {

    private static Accum a = new Accum();
    public static Accum getAccum(){
        return a;
    }

    private int counter = 0;

    public int getCount(){
        return counter;
    }

    public void updateCounter(int add){
        counter += add;
    }


    private Accum(){ }

}

Upvotes: 0

Views: 88

Answers (3)

Old Nick
Old Nick

Reputation: 1095

You can get to 98099 by declaring the methods in Accum as synchronized.

This will ensure that only one of the threads can access it's information at a time.

As the other answers have pointed out, you are getting unexpected results because there is nothing to stop each thread overwriting what the other had done.

Try this:

class Accum {

    private static Accum a = new Accum();
    public static synchronized Accum getAccum(){
        return a;               
    }

    private int counter = 0;

    public synchronized int getCount(){
        return counter;
    }

    public synchronized void updateCounter(int add){
        counter += add;
    }


    private Accum(){ }

}

Upvotes: 1

Peter Lawrey
Peter Lawrey

Reputation: 533880

As you have two threads updating the same data without thread safety, one thread easily overwrites the value set by the other one.

Each thread works on it's own thread cached value. e.g.

Thread 1 adds 1 one hundred times. It has the value 100

Thread 2 adds 1000 one hundred times. It has the value 100000

At this point, the one value is chosen. say it's thread 1's value.

Thread 1 adds 1 one hundred times. It has the value 200

Thread 2 adds 1000 one hundred times. It has the value 100100

This time, thread 2's value is chosen.

In the end only half the updates on average are retained as the value chosen is somewhat random.

Upvotes: 1

Daan
Daan

Reputation: 373

your problem is this:

 private static Accum a = new Accum();
 public static Accum getAccum(){
    return a;
}

Since its a STATIC there is only one instance shared by all threads. so when you set it in one thread, all threads get the same new value. if you remove the static notifier and instantiate a new object of class Accum for each thread it should work.

Upvotes: 0

Related Questions