CyclicBarrier hangs

[Mouss Diouf]

Can you tell me why this code hangs?

public class Test {
	public static void removeAnimals() {System.out.println("Remove animals");}
	public static void cleanPen() {System.out.println("Clean the pen");}
	public static void addAnimals() {System.out.println("Add animals");}
	public static void performTask() {removeAnimals(); cleanPen(); addAnimals();}
	public static void performTask(CyclicBarrier c1, CyclicBarrier c2) {
		try {
			removeAnimals(); c1.await(); cleanPen(); c2.await(); addAnimals();
		} catch (InterruptedException | BrokenBarrierException e) {
			// Handle checked exceptions here
		}
	}

	public static void main(String[] args) {
		ExecutorService service = null;
		try {
			service = Executors.newFixedThreadPool(4);
			CyclicBarrier c1 = new CyclicBarrier(3);
			CyclicBarrier c2 = new CyclicBarrier(3, () -> System.out.println("*** Pen Cleaned!"));
			for (int i = 0; i < 4; i++) {
				service.submit(() -> performTask(c1, c2));
			}
		} finally {
			service.shutdown();
		}
	}
}

[Stephan van Hulst]

Can you explain in your own words what the difference is between a CyclicBarrier and a CountDownLatch? If you understand the difference, you will be able to answer your own question.

[Piet Souris]

hi Mouss,

Interesting code snippet! How many times do you call 'c1.await()' and with what count are your CyclicBarriers constructed? And what about the size of your ThreadPool, is that any relevant as well?

@Stefan
Are you referring to that a CountDownLatch is a one-time-only thing, contrary to a CyclicBarrier? I do not use these things often, not much experience with these. Okay if you wait till OP has answered your question. Thanks.

[Tyoma Sakurakoji]

Agreed with the posts above. You should read how CyclicBarrier is work, the answer would be quite obvious.

[Mouss Diouf]

Hi Piet,

The answers to your questions for me:

How many times do you call 'c1.await()'

4 times because I call the method performTask 4 times

what count are your CyclicBarriers constructed

count is equal to 3 for both c1 and c2

what about the size of your ThreadPool

the pool contains 4 available threads

According to the definition : The CyclicBarrier takes in its constructors a limit value, indicating the number of threads to wait for. As each thread finishes, it calls the await() method on the cyclic barrier. Once the specified number of threads have each called await(), the barrier is released and all threads can continue.

I think I got it, the answer to my question is here "Once the specified number of threads have each called await(), the barrier is released". Since the await() is called by 4 threads, the first three threads allow to release the barrier but the last (the 4th) thread calls await and waits for 2 other threads to call await() in order to be released, since these 2 other threads never come, the program hangs in await() method called by the 4th thread. If my understanding is true, if I call performTask 6 times (or any multiple of 3), it will not hang anymore => I have tested and that's indeed the case.

Do you confirm my understanding sir Piet?

[Piet Souris]

Yessir! Have a cow for a clear explanation.

I referred to the size of your ThreadPool. Suppose you set this size to one less than the number of tasks that you submit. What happens then, and can you explain?

PS: it is true that they often call me in Holland 'sir Piet', only it is spelt as 'ZeurPiet' and the meaning is, hmm, not much like 'sir'...    

[Mouss Diouf]

Hi Piet,

The answer to "What happens then" is the program hangs.
To be completely honest I have tested the code with some "System.out.println" before proposing an explanation. Anyway let's take the previous code and change just this part, like you proposed "the size of your ThreadPool. Suppose you set this size to one less than the number of tasks that you submit" :

service = Executors.newFixedThreadPool(3);
CyclicBarrier c1 = new CyclicBarrier(3, () -> System.out.println("1st barrier released"));
CyclicBarrier c2 = new CyclicBarrier(3, () -> System.out.println("2nd barrier released"));
for (int i = 0; i < 4; i++) {
    service.submit(() -> performTask(c1, c2));
}

And here is my explanation :
1. The 3 threads available in the pool are consumed to perform the 3 first submitted tasks. Since the task has been submitted 4 times, there is one submit that is waiting a thread to be available.
2. The 3 threads execute performTask and since the barrier of CyclicBarrier is equal to 3, the CyclicBarrier c1 and c2 are released and the 3 threads finish the task.
3. The 4th submit can now be handled because there are threads available. Only one thread is required. This thread execute removeAnimals() method and get stuck in c1.await(). Because c1 needs 3 threads to call c1.await() to be released. Like my previous answer, the program will not hang if number of submitted tasks is a multiple of 3.

Am I right SuperPiet?

PS : I have google-translated "Zeur", since you guided me to find myself the answer to my question, I prefer "Super" than "Zeur"  

[Piet Souris]

hi Mouss,

You're the first (and very likely the last) person who calls me super, but thanks!

I think you know more about CyclicBarriers now than I do!

I was reffering to having a ThreadPool of size 5, the CBs having a count of 6 and sending 6 Runnables to the pool. You will see that the program again hangs, but no doubt you can explain that as well.

Meanwhile, I was grabbing the opportunity to gain some experience with CB's, and I was thinking of a way to break out of this hanging. So I changed your code a little as follows, just to see what happens:

public static void main(String[] args) {
    ExecutorService service = null;
    try {
        int nr = 4;
        service = Executors.newFixedThreadPool(nr - 1);
        CyclicBarrier c1 = new CyclicBarrier(nr);
        CyclicBarrier c2 = new CyclicBarrier(nr, () -> System.out.println("*** Pen Cleaned!"));
        for (int i = 0; i < nr; i++) {
            service.submit(() -> performTask(c1, c2));
        }
        Runnable checkerC1 = () -> check(5, TimeUnit.SECONDS, c1);
        Runnable checkerC2 = () -> check(5, TimeUnit.SECONDS, c2);
        new Thread(checkerC1).start();
        new Thread(checkerC2).start();

    } 
    finally {
        System.out.println("shutting down service!!!");
        service.shutdown();
    }
}

private static void check(long time, TimeUnit unit, CyclicBarrier cb) {
    System.out.println("Starting check.............");
    try {
        unit.sleep(time);
    }
    catch (InterruptedException e) {
        System.out.println("checker interrupted!!!");
        return;
    }
    System.out.println("waiting time check over...., resetting cb");
    if (cb.getParties() > 0) cb.reset();
}

Well, this approach didn't work (well, it actually worked, but not enough. Why?) Thinking of some other strategies, like cancelling the futures that arise when you submit tasks to the pool, or using the second form of await, where you supply a time limit. See what that delivers...   to be continued.

[Stephan van Hulst]

A cow from me as well for answering your own question. You have indeed grasped the difference well and how it affects the execution of your program. I'm especially impressed with testing your hypothesis by writing code to make the application terminate normally.

[Mouss Diouf]

Hi Piet,

That's a good exercise, I will try your code. For "You will see that the program again hangs, but no doubt you can explain that as well" thanks to you, I think yes I can explain that. The CB's barrier is never reached since the number of threads in the pool is smaller the CB's barrier count, that is why the CB is never released.

[Mouss Diouf]

Hi Stephan van Hulst,

Thank you for the cow To be honest most of the time when I have code question, I post question on stackoverflow. But here I decided to post my question on coderanch and I do not regret because you guys have been patient with me and allowed me to find the answer on my own. I really like that.

Thank you and all coderanch community

[Piet Souris]

hi Mouss,

yes, this site is much more personal than SO. Some like it, and some prefer the "no nonsense - here's the answer" style of SO. And you're welcome, it is for the repliers here also very pleasant to get such inspiring feedback!

Meanwhile, I found some sort of workaround for the hanging problem, although I think it is very ugly, but most important: I learned some things I didn't know. I created a class that extended a CB, and created a method 'awaitPiet', that first starts a Thread that waits 5 seconds and then, if necessary, resets the CB. The existing thread (if any) is interrupted, otherwise it would become a mess.

The output is incomprehensible ( ), by the way. But the fun outweighs it! Remaining problem is that the Runnable from c2 isn't executed (the "***** Pen Cleaned"). Hmm...

package javaranch18;

import java.util.concurrent.BrokenBarrierException;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

/**
 *
 * @author Piet
 */

public class MoussDiouf {

}

//***********************************************************
class Test {
    public static void removeAnimals() {
        System.out.println("Remove animals");
    }

    public static void cleanPen() {
        System.out.println("Clean the pen");
    }

    public static void addAnimals() {
        System.out.println("Add animals");
    }

    public static void performTask(PietsEllende c1, PietsEllende c2) { 
        removeAnimals();
        c1.awaitPiet();
        cleanPen();
        c2.awaitPiet();
        addAnimals();
    }

    public static void main(String[] args) {
        int nrOfTasks = 2;
        var service = Executors.newFixedThreadPool(nrOfTasks - 1);
        var c1 = new PietsEllende(nrOfTasks, 5, TimeUnit.SECONDS, "c1");
        var c2 = new PietsEllende(nrOfTasks, () -> System.out.println("*** Pen Cleaned!"), 5, TimeUnit.SECONDS, "c2");
        for (int i = 0; i < nrOfTasks; i++) {
            service.submit(() -> {System.out.println("submitted task"); performTask(c1, c2);});
        }
        System.out.println("shutting down service!!!");
        service.shutdown();
    }
}

//************************************************************************
// PietsEllende
//************************************************************************
class PietsEllende extends CyclicBarrier {
    private long delay;
    private TimeUnit unit;
    private Thread thread;
    private Runnable runnable = () -> checkCB(5, TimeUnit.SECONDS);
    private String name;
    
    PietsEllende(int nr, long delay, TimeUnit unit, String name) {
        super(nr);
        this.delay = delay;
        this.unit = unit;
        this.name = name;
    }
    PietsEllende(int nr, Runnable r, long delay, TimeUnit unit, String name) {
        super(nr, r);
        this.delay = delay;
        this.unit = unit;
        this.name = name;
    }
    
    public int awaitPiet() {
        if (thread != null) thread.interrupt();
        thread = new Thread(runnable);
        thread.start();
        try {
            await();
        }
        catch (InterruptedException | BrokenBarrierException e) {
//            System.out.println("Exception while awaiting...");
            return -1;
        }
        return getNumberWaiting() - 1;
    }
    
    private void checkCB(long time, TimeUnit unit) {
        System.out.println("Starting check for CB " + name);
        try {
            unit.sleep(time);
        }
        catch (InterruptedException e) {
//            System.out.println("checker interrupted!!!");
            return;
        }
        System.out.println("waiting time check over for " + name + "; resetting if necessary");
        if (getNumberWaiting() > 0) reset();
    }
}

[Tyoma Sakurakoji]

Actually most of the blocking methods in java.util.concurrent package have an overloaded methods allowing you to specify timeout:

cb.await(1000, TimeUnit.MILLISECONDS)

[Stephan van Hulst]

Timeouts are not in general a correct way to deal with deadlocks . You must always try to make sure that there's a possibility for the application to get through blocking calls by itself. Timeouts are only intended to cancel blocking operations that you don't have control over, such as when waiting for user input or network communication, or a call to a plugin to return.

[Tyoma Sakurakoji]

Stephan van Hulst wrote:Timeouts are not in general a correct way to deal with deadlocks . You must always try to make sure that there's a possibility for the application to get through blocking calls by itself. Timeouts are only intended to cancel blocking operations that you don't have control over, such as when waiting for user input or network communication, or a call to a plugin to return.

Trying to "solve" or "deal" with deadlocks is a bad strategy either. Your program design should be writen in such way to prevent it's possibility in the first place.
My last comment was about why coding so complex logic of fallback if CyclicBarrier is already have one leading to the same result - throwing exception on CyclibBarrier after some time and realeasing "stucked" threads.

[Piet Souris]

The meaning of the excercise is that a Timer starts whenever an 'await' is submitted, and this holds only for the last 'await'. Yes, you could have that overloaded version of await, but that would hold for every await, also for the first one, not what I had in mind.

This is only experimenting, see what is possible, practical use is unimportant. I just tried to avoid  the 'reset', by issuing just enough 'awaits to get to the barrier. Like

// after a time out
for (int i = 0; i < this.getParties() - this.getNumberWaiting(); i++) {
    try {
        await();
    }
    catch (Exception e) {
        return;
    }
}

but guess what: that didn't work too!

So I created a new ThreadPool, with just enough 'this.await()' issuing threads to make it work! Well, I can say that the code is a big mess, the output is incomprehensible, but the fun far outwheighted the misery!

[Stephan van Hulst]

Tyoma Sakurakoji wrote:Your program design should be writen in such way to prevent it's possibility in the first place.

We agree on this point. However, using a timeout to do that for execution paths that you control yourself is not the way to do it. It says: "I wrote this code, sometimes it locks up and I don't know why". It's a poor patch. The proper way is not to timeout the operation, but to debug the code and find the cause of the deadlock. Timeouts are only for situations where you know the cause, and can't control it. For instance, if my application waits for the internet to cough up data it needs, and the internet never responds, I know the cause of the problem, but I don't control it. That's when a timeout is appropriate.

My last comment was about why coding so complex logic of fallback if CyclicBarrier is already have one leading to the same result - throwing exception on CyclibBarrier after some time and realeasing "stucked" threads.

It doesn't lead to the same result. The 'complex' logic leads to successful attempts. The timeout leads to a failed attempt, band-aided with a "sorry, I tried" message.

[edit]

I missed that you likely responded in regards to Piet's code. My comments apply to so-called 'real code'. As Piet said, he is just experimenting.

[Tyoma Sakurakoji]

Stephan van Hulst wrote:I missed that you likely responded in regards to Piet's code. My comments apply to so-called 'real code'. As Piet said, he is just experimenting.

Oops, sorry for misleading you. I'll use quote next time...