CODEWITHSUNDEEP
javamultithreadingrunnableexecutorservice
bart-kosmala
bart-kosmala

Reputation: 981

Problem with thread synchronizing in Java

I'm well aware that this might be considered a duplicate, however I ran through many answers considering my problem here I can't come up with a solution.

I synchronized my runnable with an object shared by multiple threads and explicitly synchronized the method I am using inside, but the outcome of the program is always 3000.

I tried locking the Counter class but it won't change a thing. Could anyone explain me why none of my actions work in this particular example?

    public static void zad3() {
        var counter = new Counter();

        var toRun = new Runnable() {
            @Override
            public void run() {
                synchronized (counter) {
                    for (var i = 0; i < 1000; i++) {
                        counter.add(1);
                    }
                }
            }
        };

        var t1 = new Thread(toRun);
        var t2 = new Thread(toRun);
        var t3 = new Thread(toRun);

        t1.start();
        t2.start();
        t3.start();

        try {
            t1.join();
            t2.join();
            t3.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        System.out.println("counter = " + counter.getCount());
   }

public class Counter {
    protected long count_ = 0;

    public synchronized void add(long value) {
        count_ += value;
    }

    public long getCount() {
        return count_;
    }
}

edit: As suggested the problem was in the loop being constantly ran a 1000 times by each of the threads. My solution:

        var toRun = new Runnable() {
            @Override
            public void run() {
                synchronized (counter) {
                    for (var i = counter.getCount(); i < 1000; i++) {
                        counter.add(1);
                    }
                }
            }
        };

Upvotes: 1

Views: 174

Answers (2)

Alexander Petrov
Alexander Petrov

Reputation: 9492

Well you have synchronized the complete for loop around the "counter" variable which means that each thread will run tthe block once. 3 X 1000 = 3000

this block will be executed once per thread

 for (var i = 0; i < 1000; i++) {
                        counter.add(1);
 }

UPDATE: judging from your comments that you want interrupt on 1000 example code can be:

 t1.start();
 t2.start();
 t3.start();

while(counter.getValue()<1000) {
    Thread.sleep(20)
}

Annother suggestion:

public class Incremetor extends Runnable {
   Counter counter;

public Incremetor(Counter counter) {
    this.counter = counter;
}
public void run() {
   counter.increment();
}

}

ExecutorService executorService = Executors.newFixedThreadPool(8); // this mean 8 threads in total to do your runnables.
for (int i=0;i<1000;++i) {
     executorService.submit(new Incrementor(counter));        
}

Upvotes: 2

Jeppe
Jeppe

Reputation: 2256

So the problem is that you let each thread attempt 1000 increments, so you'll need something like this instead:

while (counter.getCount() < 1000) {
     counter.add(1);
}

The solution you have provided may give you the correct result, but you're actually only incrementing the counter from 1 thread. When you make a synchronized block with synchronized(object) { }, all threads will attempt to get the lock for this block, but only one will. That means in your solution, that the first thread which gets the lock, will do all 1000 increments. When the thread releases the lock and lets the others get it, the work is already done. A solution that actually distributes the increments amongst the 3 threads, should therefore not synchronize the entire for-loop.

If you run the while-loop I suggested, you will get a lot closer to 1000, but it may actually be more than 1000. Remember to run your program 10 times or set up a test-function which runs it 100 times and reports back. The problem, is that from the point of reading counter.getCount(), the value may already have changed by another thread. To reliably always get 1000, you could ensure exclusive rights to both reading and writing to the counter:

while (true) {
    synchronized (counter) {
        if (counter.getCount() < 1000) {
            counter.add(1);
        } else {
            break;
        }
    }
}

Notice that incrementing one variable like this, is slow. You're only doing 1000, but try with one billion. In fact, the 3-threaded version takes (on my PC) 1m17s, whereas a simple sequential loop takes ~1.2 seconds. You can solve this by splitting the workload amongst the threads and letting them work on a local counter with exclusive rights and then finally add the results.

Upvotes: 1

Related Questions