multithread state-dependent issue

1
votes

I was given following code snippet:

public class ThreadTest{

  private static class Thread01 extends Thread{
    private Thread02 _th2; 
    public int foo = 0;

    public void setThrd02(Thread02 thrd2){
       _th2 = thrd2;
    }

    public void run(){
      try{
        for(int i=0;i<10;i++) foo += i;

        synchronized(this){this.notify();};

        synchronized(_th2){_th2.wait();};

        System.out.print(" Foo: " + _th2.foo);
      }catch(InterruptedException ie){ ie.printStackTrace();}    
    }
  }

  private static class Thread02 extends Thread{

    private final Thread01 _th1;
    public int foo = 0;

    public Thread02(Thread01 th1){
      _th1 = th1;
    }

    public void Run(){
       try{
         synchronized(_th1){_th1.wait();}
         foo = _th1.foo;

         for(int i=0;i<10;i++) foo += i;
         synchronized(this){this.notify();};
           }
       catch(InterruptedException ie){ie.printStackTrace();}

    }

  }

  public static void main(){
    Thread01 th1 = new Thread01();
    Thread02 th2 = new Thread02(th1);

    th1.setThrd02(th2);

    th1.start(); th2.start();
    th1.join(); th2.join();
  } 
}

I think the assumption and corresponding purpose of the code is like th2 run first, it is changed to waiting status by calling _th1.wait(); Then, th1 calculates foo and wake up th2, th1 goes into waiting status; Th2 reads foo from thread1 and updated to 110, then wakes up th1 and th2 exit. Then th1 exit.

The threads could be very risk because it is very possible that thread one runs first and thread 2 will wait forever.

I am not sure any other potential problems of the code.

One possible way that can fix the problem is, for example in the thread1

public class ThreadTest{

private static boolean updated = false; private static boolean finished = false;

private static Thread01 extends Thread{

public void Run(){ // do calcuation while(finished){ wait(); } // output result } }

private static Thread02 extends Thread{ public void run(){

while(false){ wait(); }

foo = th1.foo; // do calculation // similar mechanism to notify thread 1 } }

3
javamultithreadingconcurrency
is there any other serious problem here? What might be a best way to correct it? – SecureFish
Maybe you want a CyclicBarrier, to make threads meet at a rendezvous point? – ninjalj
Also note that, even if thread 2 successfully waits on thread 1 first, then thread 1 does its loop, notifies thread 2, then thread 2 adds its loop to the sum, it still will not be 110. This is the summation of 0 to 9, not 0 to 10, so the sum will be 45, not 55, making the grand total at the end 90, not 110. – Loduwijk
CyclicBarrier might be a little complicated in this case, following can be good fix: – SecureFish

3 Answers

4
votes

There is no guarantee of ordering in your threads. It's sufficient for Thread01 to go past synchronized(this){this.notify();}; before Thread02 does synchronized(_th1){_th1.wait();} to have both threads waiting indefinitely.

Note: The fact that you are calling wait and notify on _th1 and _th2 is irrelevant. Threads here will be treated as any other object.

2
votes

@Alex has already pointed out the problems with wait and notify not being called in the order the code expects them to be (+1). However, since this is an interview question there are several other things wrong with this code:

  1. Horrible naming conventions and code formatting,
  2. Public field accessors,
  3. Synchronizing on a Thread object (bizarre),
  4. Catching InterruptedException and then just exiting the Thread,
  5. No exception handling,
  6. (Personal preference) Not using the Java concurrency libraries.

I'm sure the question was posed to tie you in knots and figure out why the concurrency is broken but, IMHO, that code is so hideous i wouldn't even begin to debug it - i'd just throw it away.

0
votes

Following can be a better fix

public class ThreadTest{
  private static volatile boolean updated = false; 
  private static volatile boolean finished = false;


   private static class Thread01 extends Thread{
    private Thread02 _th2; 
    public int foo = 0;

    public void setThread2(Thread02 th2){
        _th2 = th2;
    }

    public void Run(){          

        for(int i=0;i<10;i++) foo += i;
        System.out.print(" thread1 calcualtion " + foo + "\n");

        try{
        updated = true; 
        synchronized(this) {this.notify();};

        synchronized(_th2){
             while(!finished) 
                 _th2.wait();
             System.out.print("Foo: " + _th2.foo );    
          } 
    } 
    catch(InterruptedException ie){
        ie.printStackTrace();
    }
}
 }

  private static class Thread02 extends Thread{ 
    private final Thread01 _th1;
    public int foo = 0;

    public Thread02(Thread01 th1){
        _th1 = th1;
    }
    public void run(){
    try{
        synchronized(_th1){
            while(!updated) 
                _th1.wait();
        foo = _th1.foo; 
        } 
    for(int i=0;i<10;i++) foo +=i; 
    finished = true; 
    synchronized(this){ this.notify();}
    }catch(InterruptedException ie){
        ie.printStackTrace();
    }
}
 }

public static void main(String[] args) {
    // TODO Auto-generated method stub
    Thread01 th1 = new Thread01();
    Thread02 th2 = new Thread02(th1);

    th1.setThread2(th2);

    try{
    th1.start();
    th2.start();
    th1.join();
    th2.join();
    }catch(InterruptedException ie){
        ie.printStackTrace();
    }
}