How to avoid race conditions on alternating synchronous threads?

Question

I'm trying to remember my old CS days.

Been trying to properly implement, with the lowest possible primitives, a pair of synchronized threads. Of course I should use better concurrency tools on production code (stuff from java.util.concurrency, probably). But hey, I'm doing this for the challenge. Here is my code (this is my first question, so if this is much too long, please forgive me):

public class Test {

    public volatile Object locker1 = new Object();
    public volatile Object locker2 = new Object();
    public volatile Object locker3 = new Object();

    public class MyRunnable2 implements Runnable {
        public void run() {

            System.out.println( "MyRunnable2 started" );



            synchronized( locker3 ) {
                    try {
                        System.out.println( "r2: waiting for locker3" );
                        locker3.wait();
                        System.out.println( "r2: got locker3" );
                    } catch ( java.lang.InterruptedException e ) {
                        System.out.println( "e: " + e );
                    }
            }



            for ( int c = 0; c < 50; ++c ) {

                synchronized( locker2 ) {

                    try {
                        System.out.println( "r2: waiting for locker2" );
                        locker2.wait();
                        System.out.println( "r2: got locker2" );
                    } catch ( java.lang.InterruptedException e ) {
                        System.out.println( "e: " + e );
                    }
                }

                System.out.println( "r2: " + ( c ) );
                try {
                    Thread.sleep(1);
                } catch ( Exception e ) {
                }

                synchronized( locker1 ) {
                    System.out.println( "r2: signaling locker1" );
                    locker1.notify();
                    System.out.println( "r2: locker1 signaled" );
                }
            }
        }
    }

    public class MyRunnable1 implements Runnable {
        public void run() {
            System.out.println( "MyRunnable1 started" );

            synchronized( locker3 ) {
                    try {
                        System.out.println( "r1: waiting for locker3" );
                        locker3.wait();
                        System.out.println( "r1: got locker3" );
                    } catch ( java.lang.InterruptedException e ) {
                        System.out.println( "e: " + e );
                    }
            }

            for ( int c = 0; c < 50; ++c ) {


                synchronized( locker1 ) {

                    try {
                        System.out.println( "r1: waiting for locker1" );
                        locker1.wait();
                        System.out.println( "r1: got locker1" );
                    } catch ( java.lang.InterruptedException e ) {
                        System.out.println( "e: " + e );
                    }
                }

                System.out.println( "r1: " + ( c ) );
                try {
                    Thread.sleep(1);
                } catch ( Exception e ) {
                }

                synchronized( locker2 ) {
                    System.out.println( "r1: signaling locker2" );
                    locker2.notify();
                    System.out.println( "r1: locker2 signaled" );
                }

            }
        }
    }

    public static void main(String[] args) {
        Test t = new Test();
        t.test();
    }

    public void test() {
        MyRunnable1 r1 = new MyRunnable1();
        MyRunnable2 r2 = new MyRunnable2();
        Thread t1 = new Thread( r1 );
        Thread t2 = new Thread( r2 );
        t1.start();
        t2.start();

        try {
            Thread.sleep(1000);
        } catch ( Exception e ) {

        }
        synchronized( locker3 ) {
            System.out.println( "main: signaling locker3" );
            locker3.notifyAll();
            System.out.println( "main: locker3 signaled" );
        }

        try {
            Thread.sleep(1000);
        } catch ( Exception e ) {

        }

        synchronized( locker1 ) {
            System.out.println( "main: signaling locker1" );
            locker1.notify();
            System.out.println( "main: locker1 signaled" );
        }


        try {
            t1.join();
            t2.join();


        } catch ( java.lang.InterruptedException e ) {
            System.out.println( "e: " + e );
        }
    }
}

My question is: How can I avoid the race condition at Test.test()? Most of the time, this works - but I'm not satisfied with the sleep call. Also, please, I ask you people to evaluate my style. I'm always open to self-improvement.

EDIT: Just to make it clearer. I want MyRunnable1 to always run first. Print a number and then wait for MyRunnable2 to print the same number. Then it would print a second number and then wait for MyRunnable2 again. And so on.

I guess I can't comfortably use java.util.concurrency until I know what happens under the hood.

Depending on how long ago your old CS days were it's possible a lot has changed in the concurrency world. Memory and cache optimizations these days do some truly weird stuff it you try to access shared values outside of synchronization blocks. Have a look at Java Concurrency in Practice for a really good description of how to think about concurrency problems. As for this question, what is the race condition you are trying to remove? What is the behaviour you are trying to enforce? Do you want the threads to take turns one after the other? — Superboggly, Oct 19 '12 at 22:59
Exactly, @Superboggly. I want MyRunnable1 to always run first, print a number and then give way to MyRunnable2 to print the same number. Both Threads always taking a halt at the same place. Editing the question to make it clearer. — Daniel Monteiro, Oct 20 '12 at 03:52

score 1 · Answer 1 · answered Oct 20 '12 at 04:08

There are a couple of fundamental problems with this beyond just the sleep call (actually there's nothing inherently wrong with a sleep call...)

For starters, you're not actually doing anything to have the threads signal each other. If I understand your comment correctly you want something like

Thread 1: 1
Thread 2: 1
Thread 1: 2
Thread 2: 2
...

As output. What this code will currently do is start both threads, then both threads will wait. The main thread will then call notifyAll on the locker3 object. That means any thread waiting on that object will get run, but there're no guarantee of what order the threads will get run in, because you're notifying everybody. There's race condition number 1. Additionally, you're only calling notifyAll on the locker2 and locker1 objects once, but you end up waiting on them about 50 times per thread. That means your threads are going to hang.

What you really need is something like this:

Thread 1 and thread 2 are started
Thread 2 immediately waits on some signal object
Thread 1 immediately prints a number and then calls notify on the same signal object
Thread 1 then waits on the same signal object
Thread 2 resumes and prints the number
Thread 2 then calls notify on the signal object
Thread 2 then waits on the signal object
Return to step 3.

I can't guarantee that's completely without race conditions, but to do what you're suggesting, you're going to need an algorithm like that. You also could make it arbitrarily more complicated, but that's kinda a good baseline.

Your proposal was actually my first try, but I was getting deadlocks much more often than with my lousy solution above. After all, I wouldn't want to complicate things without a good reason ;-) In "my old CS days", I remember implementing this in C with pThreads and Win32 with the native thread API the way you described. That's what I'm all about in this question =-) — Daniel Monteiro, Oct 20 '12 at 15:01

score 1 · Accepted Answer · answered Oct 22 '12 at 18:04

@Chris Thompson is right - you can alternate on a single signal object. But you will never guarantee which thread goes first and you have to take some care to make sure that you don't have your last thread waiting for your second-to-last-thread to notify after your second-to-last thread has already finished and exited.

I modified your code to work - but with no guarantee on who goes first, then I also added an alternate "MyRunnableOrdered" that controls the order the two threads execute. In either case if the threads do not have the same number of loops to complete, or if either one exits because of an error, then you will risk starvation. Paying attention to and using the interrupted exception will help in the latter case.

public class Test {

public Object locker = new Object();
public boolean oneDone = false;

public class MyRunnable2 implements Runnable {
    public void run() {

        System.out.println( "MyRunnable2 started" );

        for ( int c = 0; c < 50; ++c ) {
            synchronized( locker ) {
                System.out.println( "r2: " + ( c ) );
                locker.notify();
                if(c == 49) {
                    oneDone = true;
                }

                try {
                    if(!oneDone) {
                        locker.wait();
                    }
                } catch ( java.lang.InterruptedException e ) {
                    System.out.println( "e: " + e );
                }                    
            }
        }
    }
}

public class MyRunnable1 implements Runnable {
    public void run() {
        System.out.println( "MyRunnable1 started" );

        for ( int c = 0; c < 50; ++c ) {
            synchronized( locker ) {
                System.out.println( "r1: " + ( c ) );
                locker.notify();
                if(c == 49) {
                    oneDone = true;
                }

                try {
                    if(!oneDone) {
                        locker.wait();
                    }
                } catch ( java.lang.InterruptedException e ) {
                    System.out.println( "e: " + e );
                }
            }
        }
    }
}


public Object sequenceLock = new Object();
public boolean sequence = true;

public class MyRunnableOrdered implements Runnable {

    private final boolean _match;

    public MyRunnableOrdered(boolean match) 
    {
        _match = match;
    }

    public void run() {
        System.out.println( "MyRunnable1 started" );

        for ( int c = 0; c < 50; ++c ) {
            synchronized( sequenceLock ) {
                while(_match != sequence) {
                    try {
                        sequenceLock.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }

                System.out.println( "r" + _match + ":" + ( c ) );
                sequence = !sequence;
                sequenceLock.notify();
            }
        }
    }
}    


public static void main(String[] args) {
    Test t = new Test();
    t.test();
}

public void test() {
    MyRunnable1 r1 = new MyRunnable1();
    MyRunnable2 r2 = new MyRunnable2();
    Thread t1 = new Thread( r1 );
    Thread t2 = new Thread( r2 );


    synchronized( locker ) {
        t1.start();
        t2.start();
    }


    try {
        t1.join();
        t2.join();
    } catch ( java.lang.InterruptedException e ) {
        System.out.println( "e: " + e );
    }

    System.out.println("Done part 1");

    MyRunnableOrdered o1 = new MyRunnableOrdered(true);
    MyRunnableOrdered o2 = new MyRunnableOrdered(false);
    synchronized(sequenceLock) {
        sequence = true;
    }
    Thread to1 = new Thread( o1 );
    Thread to2 = new Thread( o2 );
    to1.start();
    to2.start();

    try {
        to1.join();
        to2.join();
    } catch ( java.lang.InterruptedException e ) {
        System.out.println( "e: " + e );
    }       
    System.out.println("Done part 2");
}
}

Note that the MyRunnableOrdered idea will not extend beyond two threads, because we do not control who wakes up when notify is called. What you would want in that case is an ordered list of threads to work through. At that point concurrency may not be the best solution!

There is also probably a better implementation of MyRunnableOrdered using AtomicBoolean and no locking if you decide to use the concurrency library.

Also note that we do not need to use "volatile" because all the variable access is protected by synchronized blocks.

Thank you. Your solution looks really good! This was just to get my feet wet. I will do some more experimenting and then jump into the concurrency pack. I specially enjoyed you starting the threads inside the synchronized( locker ) block. Its not easy to wrap your head around concurrency. It takes practice. I hope I will eventually reach the point I'm capable of having such ideas. — Daniel Monteiro, Oct 27 '12 at 13:01

How to avoid race conditions on alternating synchronous threads?

2 Answers2