java threads alternative to synchronization

Question

I am new to concurrent programming and I am facing few issues with the below code using Java threads.

Status Class (this class tracks the position availability):

public class Status {

    private static Map<String, Boolean> positions = new HashMap<>();

    static {
        //Initially all positions are free (so set to true)
        positions.put("A", true);
        positions.put("B", true);
    }

    public synchronized void occupyOrClear(String position, 
                                    boolean status) {
         positions.put(position, status);
   }

    public boolean isClear(String position) {
        return positions.get(position);
    }
}

MyThread Class:

public class MyThread implements Runnable {

    private String[] positions;

    private String customer;

    public MyThread(String customer, String[] positions) {
        this.positions = positions;
        this.customer = customer;
    }

    private Status status = new Status();

    public void run() {
        for (int i = 0; i < positions.length;) {
            String position = positions[i];
            if (status.isClear(position)) {
                // position occupied now
                status.occupyOrClear(position, false);
                System.out.println(position + " occupied by :"+customer);

                try {
                    //my real application logic goes below (instead of sleep)
                    Thread.sleep(2000);
                } catch (InterruptedException inteExe) {
                    System.out.println(" Thread interrupted ");
                }

                // Now clear the position
                status.occupyOrClear(position, true);
                System.out.println(position + " finished & cleared by:"+customer);
                i++;
            } else {
                try {
                    Thread.sleep(1000);
                } catch (InterruptedException inteExe) {
                    System.out.println(" Thread interrupted ");
                }
            }
        }
    }
}

ThreadTest Class:

public class ThreadTest {
    public static void main(String[] args) {
    String[] positions = { "A", "B"};
    Status status = new Status();
    Thread customerThread1 = new Thread(new MyThread(status, "customer1", positions));
    Thread customerThread2 = new Thread(new MyThread(status, "customer2", positions));
    Thread customerThread3 = new Thread(new MyThread(status, "customer3", positions));
    customerThread1.start();
    customerThread2.start();
    customerThread3.start();
}
}

Even though I have used 'synchronized' I could notice that some times Thread3 is picking up prior to Thread2 and could you please help me to resolve this issue and to acheive the following results ?

(1) Always customerThread1 should take the positions first and then followed by customerThread2 and then customerThread3 (etc...)

(2) As soon as the A's position is freed by customerThread1, the position should be immediately picked up by customerThread2 (rather than customerThread2 and customerThread3 waiting till all positions are done by customerThread1).And as soon as customerThread2 finishes position 'A', then customerThread3 should pick it up, etc..

(3) As soon as the position (A, B, etc..) is freed/available, the next customerThread should pick it up immediately.

(4) The solution should avoid all race conditions

Answer 1

There are several fundamental problems.

1. You have broken code and already noticed that it doesn’t work. But instead of asking how to fix that broken code, you are asking for alternatives with higher performance. You will never manage to write working programs with that attitude.

2. Apparently, you have no idea, what synchronized does. It acquires a lock on a particular object instance which can be held by one thread only. Therefore, all code fragments synchronizing on the same object are enforced to be executed ordered, with the necessary memory visibility. So your code fails for two reasons:

   1. You are creating multiple instances of Status accessing the same objects referenced by a static variable. Since all threads use different locks, this access is entirely unsafe.
   2. Your occupyOrClear is declared synchronized, but your method isClear is not. So even if all threads were using the same lock instance for occupyOrClear, the result of isClear remained unpredictable due to its unsafe access to the map.

3. You have code of the form
   if(status.isClear(position)) { status.occupyOrClear(position, false); …
   which matches the check-then-act anti-pattern. Even if each of these two method calls were thread-safe, this sequence still remained unsafe, because between these two invocations, everything can happen, most notably, the condition, the thread just checked, may change without the thread noticing. So two or more threads could invoke isClear, receiving true and then proceed with occupyOrClear.

4. You are using Thread.sleep.

Answer 2

You can try with the following pseudocode:

main() {
    //some concurrent queues, eg ConcurrentLinkedQueue
    Queue t1Tasks = new Queue("A","B","C");
    Queue t2Tasks = new Queue();
    Queue t3Tasks = new Queue();
    Thread t1 = new PThread(t1Tasks,t2Tasks,"customer1");
    Thread t2 = new PThread(t2Tasks,t3Tasks,"customer2");
    Thread t3 = new PThread(t3Tasks,null,"customer3");
}

PThread {
       Queue q1,q2;
       PThread(Queue q1, Queue q2,...){}
       run() {
            while (item = q1.get()) {
                  //process item
                  q2.put(item); //to be processed by next thread
            }
       }
}