Mutual Exclusion using JAVA 2-Threads Solution

Question

I am trying to implement 2-threads solution using LockOne(Mutually Exclusive) Algorithm. Implementing this algorithm, i am trying work out a thread defining my own lock methods but i am not getting the desired output. When i run the program..all i get as output is "Thread-0 Locked" and "Thread-1 Locked"..Can anyone plz let me know where am i going wrong? my code is given below

public class Main {

    static MyLock lock=null;
    static int count=0;

    public static void main(String[] args) throws InterruptedException {

        Thread[] threads=new Thread[2];

        threads[0]=new Thread(new MyThread());
        threads[1]=new Thread(new MyThread());

        lock=new MyLock();

        threads[0].start();
        threads[1].start();
        threads[0].join();
        threads[1].join();

        System.out.println(count);
    }

}

public class MyLock{

    private boolean locked=false;
    private String current; 

    public void lock() {

        if(!locked){
            locked=true;
            current=Thread.currentThread().getName();
        }

        System.out.println(Thread.currentThread().getName()+" locked");
        while(locked && current!=Thread.currentThread().getName());

    }


    public void unlock() {

        System.out.println(Thread.currentThread().getName()+" unlocked");
        locked=false;

    }
}

public class MyThread implements Runnable{

    @Override
    public void run() {

        int i=1;
        while(i<=100){
            Main.lock.lock();
            Main.count++;
            Main.lock.unlock();
            i++;
        }
    }

}

Answer 1

And another solution in case if you need reentrant lock with fair acquisition policy:

public class MyLock
{
    private String holder;
    private Queue<Long> next = new ConcurrentLinkedQueue<>();

    private Object syncLock = new Object();

    public void lock()
    {
        Long threadId = Thread.currentThread().getId();
        this.next.add(threadId);
        boolean acquired = false;

        while (!acquired)
        {
            synchronized (this.syncLock)
            {
                if (this.holder == null)
                {
                    if (this.next.peek() == threadId)
                    {
                        this.holder = Thread.currentThread().getName();
                        System.out.println(this.holder + " locked");

                        acquired = true;
                    }
                }
                else
                {
                    if (this.holder.equals(Thread.currentThread().getName()))
                    {
                        acquired = true;
                    }
                }
            }
        }

        this.next.remove(threadId);
    }

    public void unlock()
    {
        synchronized (this.syncLock)
        {
            System.out.println(Thread.currentThread().getName() + " unlocked");
            this.holder = null;
        }
    }
}

Answer 2

There are two problems in your code.

1. if(!locked) and set locked=true is not atomic operation, which means two threads can find it not locked and lock it at same time.
2. There is no syncronization on varible locked and current, so one thread may not read the fresh value the other thread set on them because of memory barriar.

You can solve this with AtomicBoolean and volatile:

import java.util.concurrent.atomic.AtomicBoolean;

public class MyLock{

    private AtomicBoolean locked = new AtomicBoolean(false);
    private volatile String current;

    public void lock() {
        for (;;) {
            if(!locked.get()){
                if (locked.compareAndSet(false, true)) {
                    current = Thread.currentThread().getName();
                    System.out.println(current + " locked");
                    return;
                }
            }
        }
    }


    public void unlock() {
        System.out.println(current + " unlocked");
        locked.set(false);
    }
}