CODEWITHSUNDEEP
javamultithreadingsynchronized
Jason Smith
Jason Smith

Reputation: 13

why is this mistake happening in my java synchronized method?

I'm a java fresher,learning the communication of thread in java with wait() notify(), and some stupid things happens:
Here is the thing: I want to set human and get human constantly with multi-Thread, which means the result is (Jack male, Mary female, Jack male, Mary female......)
here is my code:

class Human {
  private String name;
  private String sex;
  private boolean b = false;

  public synchronized void set(String name, String sex) {
    if (b) {
        try {
            this.wait();
        } catch (InterruptedException e) {}
    } 
    this.name = name;
    this.sex = sex;
    b = true;
    this.notify();         
  }      
  public synchronized void get() {
    if (!b) {
        try {
            this.wait();
        } catch (InterruptedException e) {}
    }  
    System.out.println(name+"  "+sex);
    b = false;
    this.notify();        
  }
}
class SetHuman implements Runnable {
  private Human h;
  SetHuman(Human h) {
    this.h = h;
  }
  public void run() {     
    int x = 0;
    while(true) {           
        if (x==0) {
            h.set("Jack","male");
        }  else {
            h.set("Mary","female");
        }
        x = (x+1)%2;
    }
  }
}
class GetHuman implements Runnable {
  private Human h;
  GetHuman(Human h) {
    this.h = h;
  }
  public void run() {
    while (true) {
        h.get();
    }
  }
}

class HumanDemo  {
  public static void main(String[]args) {
    Human h = new Human();
    SetHuman sh = new SetHuman(h);
    GetHuman gh = new GetHuman(h);
    
    Thread t1 = new Thread(sh);
    Thread t2 = new Thread(gh);
    
    t1.start();
    t2.start();
  }
}

When i run HumanDemo ,it worked :result

And then ,I add a else judgement in my synchronized function set() and get(), this thing happed:

public synchronized void set(String name, String sex) {
    if (b) {
        try {
            this.wait();
        } catch (InterruptedException e) {}
    } else {
        this.name = name;
        this.sex = sex;
        b = true;
        this.notify();
     }
}      
public synchronized void get() {
    if (!b) {
        try {
            this.wait();
        } catch (InterruptedException e) {}
    }  else {
        System.out.println(name+"  "+sex);
        b = false;
        this.notify();
    }
}

new result

why is this? anybody please tell me why? thank you ^-^!

Upvotes: 1

Views: 95

Answers (2)

VGR
VGR

Reputation: 44338

A call to wait() must always be in a while loop, as described in the documentation. You are doing that, which is good. But the part you got wrong is that the while loop needs to be inside the synchronized block in order to be thread-safe:

while (!condition) {
    // Wrong --- another thread might call notify or notifyAll
    // when the program is at this point, where this thread
    // will not detect it.
    synchronized (h) {
        h.wait();
    }
}

The while loop must always be inside the synchronized block to ensure thread safety:

synchronized (h) {
    while (!condition) {
        h.wait();
    }
}

Also, an interrupt is a signal to your thread that someone wants it to terminate. Do not ignore it.

By far the easiest way to do this is to place your entire loop inside the try/catch, so the InterruptedException will automatically end the loop:

try {
    synchronized (h) {
        while (!condition) {
            h.wait();
        }
    }
} catch (InterruptedException e) {
    e.printStackTrace();
}

Finally, never ever write an empty catch block. Hiding exceptions makes troubleshooting your code extremely difficult.

Upvotes: 1

Sorcen
Sorcen

Reputation: 141

In your working first example your set/get() methods are mutually exclusive. The this.wait() will force them to wait for the other before they can do the work.

In your 2nd example you break this with the else because there is no guarantee which thread will get the lock on 'this' after the lock has been released. That way an arbitrary number of 'set()' might get lost to the waiting state never setting their value to 'this.name' and 'this.sex'.

Example (gt= get thread st = set thread):

main-method starts thread st

st: h.set("Jack","male"); b is false -> this.name = "Jack"; b=true; (now there is no guarantee if st will execute again or if gt is already created and will get the lock on 'this' via the synchronized get() method. This time let it be st that gets the lock.)

?main-method starts thread gt? (might be later)

st: h.set("Mary","female"); b is true -> this.wait(); (Now st is waiting till someone releases the lock on 'this'. As this.name and this.sex is set in the else statement it will never set its current values and this call with "Marry" and "female" will get lost. So next gt will execute.)

?main-method starts thread gt? (might have already happened)

gt: b is true -> System.out.println(name+" "+sex); b = false ... (at the end of the method gt will release the lock on 'this' now st will leave the waiting state and try to acquire the lock for this. gt is also trying to get the lock for 'this' again. Again there are no guarantees which thread will get the lock and can execute now.)

Long story short:

You are throwing away a 'random' amount of set calls because of the else in the set method (so there is unlikely an alternating order). Without the else in the set method it should work. Although this would waste method calls for get that just run into the wait state to return with no work done.

Upvotes: 1

Related Questions