0

Just for practice I wanted to implement the java synchronized keyword as a java object. Would you say the code below is a good design for this? I guess AtomicReference would have a similar performance to AtomicBoolean?

Updated code after suggestions:

public class SynchronizedBlock implements Runnable{

private final Lock lock;
private final Runnable runnable;

public SynchronizedBlock(Runnable r, Lock l){
    runnable = r;
    lock = l;
}

public void run() {
    try {
        while(!lock.compareAndSet(false, true)){
            Thread.sleep(100);
        }
        runnable.run();
    } catch (InterruptedException e) {
        e.printStackTrace();
    } finally {
        lock.unlock();
    }


}

 }

 public class Lock {
private final AtomicReference<Boolean> locked = new AtomicReference<Boolean>(false);

public boolean compareAndSet(boolean expected, boolean update){
    return locked.compareAndSet(expected, update);
}

public boolean isLocked(){
    return locked.get();
}

public void unlock(){
    locked.set(false);
}
 }

@Test
public void test() {



    final SynchronizedBlock sb = new SynchronizedBlock(new Runnable(){

        public void run() {
            x++;
            System.out.println(x);
        }

    }, new Lock());

    Runnable r1 = new Runnable(){

        int c = 0;
        public void run() {
            while(c<10){
                sb.run();
                c++;
            }
        }

    };

    Runnable r2 = new Runnable(){

        int c = 0;
        public void run() {
            while(c<10){
                sb.run();
                c++;
            }
        }

    };

    Thread t1 = new Thread(r1);
    Thread t2 = new Thread(r2);

    t1.start();
    t2.start();

    while (t1.isAlive() && t2.isAlive()){

    }

    assertEquals(20,x);

}
newlogic
  • 807
  • 8
  • 25

2 Answers2

1

You should add a method to encapsulate the compareAndSwap, and there is no point looping for the lock to be free before attempting to obtain it. Why get in the situation where you can see the lock is free but by the time you try to take it, it is gone.

I would remove the lock method and place the unlock in a finally lock so that an Exception/Error doesn't result in a lock which never unlocks.

Also I would use an AtomicBoolean which is more natural than an AtomicReference

Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
  • Hi there, i've updated the code and managed to break it on the way, my unit test (see new code above) is now failing intermittently. Any ideas? its either passing or failing with a difference of 10. – newlogic Oct 06 '13 at 22:53
  • 1
    I would still use AtomicBoolean. Not sure how/why it would be failing. You appear to have enough read/write barriers for `x` to be correct between threads. – Peter Lawrey Oct 07 '13 at 05:50
  • 1
    I think the problem is with the while loop before the assertion in the test, it only loops if both threads are alive, and therefore the assertion is performed when both threads are not alive, this is true if one thread starts and finishes before the other, and for a few other cases. – newlogic Oct 07 '13 at 09:12
  • Correct. You need || instead of && – Peter Lawrey Oct 07 '13 at 13:37
0

Firstly and most importantly, you should remove Thread.sleep(100). This will cause at least 100ms latency even in only 2-thread contention.

You can simply use AtomicBoolean instead of AtomicReference to simplify your code. Also if you're really concerned about concurrency in high-contention situation, you can modify your code to check if it's locked before doing CAS.

while (true) {
  if (lock.isLocked()) continue; // or get() == true if you use AtomicBoolean
  if (lock.compareAndSet(false, true)) 
    break;
}

This is an example of TTAS(Test-Test-And-Set) locking which takes advantage of local-spinning to reduce main-memory access while looping. See http://en.wikipedia.org/wiki/Test_and_Test-and-set

yhpark
  • 175
  • 1
  • 2
  • 9
  • Cheers, the man reason I put the sleep in was to free up the CPU, I don't want to starve other threads. During high contention periods. How would you solve that problem? – newlogic Oct 08 '13 at 11:35
  • @user1037729 I guess I'm too late to answer but I'd rather adjust thread priority or try else beforehand. Profile threads' and see how they behave. – yhpark Mar 27 '14 at 21:53