Problem with thread synchronizing in Java

Question

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);
                    }
                }
            }
        };

@Jeppe the outcome is always 3000, while I'm trying to achieve 1000. — bart-kosmala
– bart-kosmala, Commented Jun 6, 2019 at 16:17
Then you should restrict the loop to run while the counter.getCount() < 1000, not i. And this means 2 operations that must be atomic (read, then write), (if you remove the synchonize around the loop). — Jeppe
– Jeppe, Commented Jun 6, 2019 at 16:18
Why do you expect the result to be 1000 if you have 3 threads incrementing it 1000 times each? — Jacob G.
– Jacob G., Commented Jun 6, 2019 at 16:18

Alexander Petrov · Accepted Answer · 2019-06-07 11:05:53Z

2

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));        
}

edited Jun 7, 2019 at 11:05

answered Jun 6, 2019 at 16:15

Alexander Petrov

9,5422 gold badges36 silver badges76 bronze badges

Sign up to request clarification or add additional context in comments.

21 Comments

bart-kosmala Over a year ago

I see, but is there a way I can split the 1000 I try to get between the threads? Because this is what my excercise is about, I need to do it using the for loop.

Alexander Petrov Over a year ago

@bart-kosmala i don't understand what you want to be honest. Explain more.

bart-kosmala Over a year ago

You have answered my question, I'll mark it as accepted. My fix will be in the edited question.

Alexander Petrov Over a year ago

There is no best aproach. There are many aproaches and all depend on the case. In your case the best aproach is to not use threads at all:) Or to just pick one multithreading option and explain what problems is it solving.

Jeppe Over a year ago

On my machine, with N = 1_000_000_000L a sequential solution takes 0m1.181s whereas a 8-thread solution with thread-local counters takes 0m0.208s. It depends on the problem, it may not always be possible to work thread-locally. In general you should consider how well a solution behaves as N increases infinity.

|

Jeppe · Accepted Answer · 2019-06-07 09:53:44Z

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.

Great point, I'll keep that in mind when working further with threads.

Collectives™ on Stack Overflow

Problem with thread synchronizing in Java

2 Answers 2

21 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

21 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related