CyclicBarrier misunderstanding

Question

I tried to run example with CyclicBarrier from one of tutorials: Service man should fill empty printers when the queue of empty printers is 3. But when I ran the code it appears that printers are filled with 2, 3 or 4 empty printers in the queue:

Printer1 is empty

Printer12 is empty

Printer14 is empty

Printer13 is empty

Filling [Printer1, Printer12, Printer14, Printer13]

Printer2 is empty

Printer7 is empty

Filling [Printer2, Printer7]

So is the example wrong or my understanding of CyclicBarrier? I consider that queue should be exactly 3 elements size. What should I add to the code to fix that? Thanks in advance.

Code:

import java.util.ArrayList;
import java.util.List;
import java.util.Random;
import java.util.concurrent.BrokenBarrierException;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.TimeUnit;

public class PrinterRecharger {
public static void main(String args[]) {
    ServiceMan serviceMan = new ServiceMan(3);

    for (int i = 0; i < 15; i++) {
        new Thread(new Printer(serviceMan, "Printer" + (i + 1))).start();
    }
}
}

class ServiceMan {

private CyclicBarrier queue;
private List<String> inQueue;

public ServiceMan(int hardWorking) {
    inQueue = new ArrayList<String>();
    queue = new CyclicBarrier(hardWorking, new Runnable() {
        @Override
        public void run() {
            System.out.println("Filling " + inQueue);
            inQueue.clear();
        }
    });
}

public void recharge(String name) {
    try {
        inQueue.add(name);
        queue.await();
    } catch (InterruptedException e) {
        e.printStackTrace();
    } catch (BrokenBarrierException e) {
        e.printStackTrace();
    }
}

}

class Printer implements Runnable {

private String name;
private Random rand;
private ServiceMan serviceMan;

public Printer(ServiceMan serviceMan, String name) {
    this.name = name;
    this.serviceMan = serviceMan;
    this.rand = new Random();
}

public void run() {
    try {
        while (true) {
            TimeUnit.SECONDS.sleep(rand.nextInt(10));
            System.out.println(name + " is empty");
            serviceMan.recharge(name);
        }
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
}

}

Marko Topolnik · Answer 1 · 2014-10-13T10:32:20.013

0

Your code is thread-unsafe in several ways that I can immediately see, and probably some more which I have missed. You have data races as well as other types of race conditions.

ArrayList is not a thread-safe class, but you use it from multiple threads with no synchronization. Wrap the list into a Collections.synchronizedList() to see some improvement.
You lack any mutual exclusion between recharge() and the CyclicBarrier's action. A thread could add an item to the queue only to have it cleared away by the action.

edited Oct 13 '14 at 10:32

answered Oct 13 '14 at 10:23

Marko Topolnik

195,646
29
319
436

inQueue = Collections.synchronizedList(new ArrayList()); Doesn't helps. Looks like more changes should be done. What can I do here for mutual exclusion? – Jack Oct 13 '14 at 10:38
Define `private final Object lock = new Object()` and se `synchronized(lock)` in both `recharge` and the action. Be careful not to enter a blocking state while holding the lock, though. – Marko Topolnik Oct 13 '14 at 10:42
It's a lot more convenient! But still I can see 4 or 2 items in queue (very rare now). I added synchronized (lock) for code in barrier action and adding element to queue in recharge method. But looks like something else should be added. – Jack Oct 13 '14 at 10:50

CyclicBarrier misunderstanding

1 Answers1