Concurrency issue when coordinating two threads

Question

I am trying to divide the work of a loop into two threads.

I am using an ExecutorService to create the 2nd thread while using the main thread as the other one.

I am using a counter to stop the loop when it reaches a certain value but I'm not able to sync the counter among these two threads and loop is running one extra time.

Also, I am using a while loop in main thread to know when the desired count has reached print the end time, which is also not working.

ExecutorService executorService = Executors.newFixedThreadPool(1);
    
    Clock clock = new Clock();

    int count = 5;
    AtomicInteger c = new AtomicInteger(1); 
    clock.start();
    
    executorService.execute(()->{
        while (c.get() <= count) {
            
            System.out.println("LOOP ONE" + "----PRINT_ME");
            c.incrementAndGet();
        }
    });
    
    while (c.get() <= count) {
        
        System.out.println("LOOP TWO" + "----PRINT_ME");
        c.incrementAndGet();
    }
    
    while(true) {
        if(c.get() == count) {
            System.out.println("STOPPED");
            clock.stop();
            break;
        }
    }

Output -

Clock started at -- 2020-07-25T12:32:59.267
LOOP TWO----PRINT_ME
LOOP TWO----PRINT_ME
LOOP TWO----PRINT_ME
LOOP TWO----PRINT_ME
LOOP TWO----PRINT_ME
LOOP ONE----PRINT_ME

In the code above PRINT_ME should get printed 5 times(int count = 5;) only but is is getting printed 6 times (One time extra).

I am not sure what is going on here and how we can share a counted among two threads.

PSI: If I add Thread.sleep(2000); in both loops

executorService.execute(()->{
        while (c.get() <= count) {
            try {
                Thread.sleep(2000);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
            System.out.println("LOOP ONE" + "----PRINT_ME");
            c.incrementAndGet();
        }
    });
    
    while (c.get() <= count) {
        
        try {
            Thread.sleep(2000);
        } catch (InterruptedException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
        System.out.println("LOOP TWO" + "----PRINT_ME");
        c.incrementAndGet();
    }

the output is:

Clock started at -- 2020-07-25T12:54:15.596
LOOP TWO----PRINT_ME
LOOP ONE----PRINT_ME
LOOP TWO----PRINT_ME
LOOP ONE----PRINT_ME
LOOP TWO----PRINT_ME
LOOP ONE----PRINT_ME

I tried running your code. It is printing PRINT_ME for 5 time only. — Ankit Chauhan, Jul 25 '20 at 07:09
@MadProgrammer That explains why it prints out "LOOP TWO" instead of "LOOP ONE". But it does not explain what OP is wondering about ... why it prints out "PRINT_ME" 6 times instead of 5 times. — Seelenvirtuose, Jul 25 '20 at 07:14
I have updated the output it was misprinted .I have added STOPPED in the while loop which should have get printed if count reached 5. Which is not happening. — Jainesh kumar, Jul 25 '20 at 07:14
From playing around with it, it would "seem" that your first loop is completing the task before the second thread has chance to run fully. I added a `Thread.sleep(100)` into both loops and was able to get them to interleave — MadProgrammer, Jul 25 '20 at 07:15
@Seelenvirtuose Threading is fun - I can't replicate the issue - the first loop completes before the second loop runs :/ — MadProgrammer, Jul 25 '20 at 07:15
@Seelenvirtuose It "could" be thread scheduling issue - so the main thread is able to enter the `while (c.get() <= count) {` but doesn't complete before the second thread completes :/ — MadProgrammer, Jul 25 '20 at 07:17
@MadProgrammer What about the while loop in the end why it not getting terminated ? — Jainesh kumar, Jul 25 '20 at 07:18
@Jaineshkumar Use `if (c.get() >= count) {` instead - or better yet, use `shutdown` and `awaitTermination` on the `ExecutorService` — MadProgrammer, Jul 25 '20 at 07:20
`System.out.println` involves locks. Locks can be biased - meaning the thread that just released the lock will get it again. Your executor thread starves - it tries to get the lock, but the lock is held by the main thread. — Johannes Kuhn, Jul 25 '20 at 07:23
@Jaineshkumar I'd also "recommend" using `while (c.incrementAndGet() <= count) {` as it tends to produce a more suitable result and you're only synchronising once per loop instead of twice — MadProgrammer, Jul 25 '20 at 07:23
@MadProgrammer while (c.incrementAndGet() <= count) { also isn't giving the right output. :( — Jainesh kumar, Jul 25 '20 at 07:29
@Jaineshkumar I changed `AtomicInteger c = new AtomicInteger(1);` to `AtomicInteger c = new AtomicInteger(0);`, you could change the `count` to `6`, same thing. I also used two threads for the `ExecutorService` and added both loops to the service and, finally, I added a `Thread.sleep(1)` to the end of each loop so that they would interleave — MadProgrammer, Jul 25 '20 at 07:37

Grzegorz Piwowarek · Accepted Answer · 2020-07-26T06:48:10.653

The problem is that you poll the counter first, perform an action, and then increment the counter signaling that the job has been done.

This creates a window where both threads can be doing the work at the same time and performing the incrementation after they are finished which is just too late.

To make it easier to visualize, let's imagine that there the desired count is 1 and that there are two threads fighting for a task to execute. In your implementation, both threads would start by checking c.get() <= count which would be true for both, and then execute the task which results in the problem you described.

In order to prevent this, threads need to claim a "free slot" first, and only then perform the task. This can be performed by incrementing the counter before the job starts, and then verifying the max count condition before starting each job:

    int count = 5;
    AtomicInteger c = new AtomicInteger(0);

    executorService.execute(()->{
        while (c.incrementAndGet() <= count) {
            // ...
    });

    while (c.incrementAndGet() <= count) {
            // ..
    }

Concurrency issue when coordinating two threads

1 Answers1