5

I am having some trouble getting my program to not deadlock at times. I think I need to add a third synchronized method release, which can be used to release the other thread after a ping is called. code is below. 

// Attempt at a simple handshake.  Girl pings Boy, gets confirmation.
// Then Boy pings girl, get confirmation.
class Monitor {
    String name;

    public Monitor (String name) { this.name = name; }

    public String getName() {  return this.name; }

     // Girl thread invokes ping, asks Boy to confirm.  But Boy invokes ping,
    // and asks Girl to confirm.  Neither Boy nor Girl can give time to their
    // confirm call because they are stuck in ping.  Hence the handshake 
    // cannot be completed.
    public synchronized void ping (Monitor p) {
      System.out.println(this.name + " (ping): pinging " + p.getName());
      p.confirm(this);
      System.out.println(this.name + " (ping): got confirmation");
    }

    public synchronized void confirm (Monitor p) {
       System.out.println(this.name+" (confirm): confirm to "+p.getName());
     }
}

class Runner extends Thread {
    Monitor m1, m2;

    public Runner (Monitor m1, Monitor m2) { 
      this.m1 = m1; 
      this.m2 = m2; 
    }

    public void run () {  m1.ping(m2);  }
}

public class DeadLock {
    public static void main (String args[]) {
      int i=1;
      System.out.println("Starting..."+(i++));
      Monitor a = new Monitor("Girl");
      Monitor b = new Monitor("Boy");
      (new Runner(a, b)).start();
      (new Runner(b, a)).start();
    }
}
Improve this question
4
  • So what exactly are you trying to sync? Right now you're set up to prevent a Monitor from pinging and confirming at the same time. Which race condition are you trying to avoid? Commented Jul 17, 2012 at 16:15
  • Correct me if I'm wrong, but as is, wouldn't the code immediately hang as soon as ping tries to call confirm from within it? Commented Jul 17, 2012 at 16:17
  • Well about 1/10 times I reach a dead lock (both stuck in ping, waiting for the other to confirm). I need the code to be deterministic, so that it never deadlocks. I receive the hint to "Add a third synchronized method to the Monitor class called release. This method can be used by one thread to release another. Change ping to wait then confirm to the other thread. Then release the other thread." Commented Jul 17, 2012 at 16:21
  • 1
    @DennisMeng Not immediately, as the synchronized for ping is locking on this and the synchronized on p.confirm is locking on p. The problem presented is that p is also locked in its ping call on the second thread. Commented Jul 17, 2012 at 16:25

2 Answers 2

Reset to default
6

When some operation needs to get hold of two different locks the only way of ensuring that there are no deadlocks is to make sure that every thread trying to perform those operations acquires the lock on the multiple objects in the same order.

To fix deadlock you need to modify the code like this - not pretty, but it works.

 public void ping (Monitor p) {
  Monitor one = this;
  Monitor two = p;
  // use some criteria to get a consistent order
  if (System.identityHashCode(one) > System.identityHashCode(two)) {
    //swap
    Monitor temp = one;
    one = two;
    two = one;
  }
  synchronized(one) {
       synchronized(two) {
           System.out.println(this.name + " (ping): pinging " + p.getName());
           p.confirm(this);
           System.out.println(this.name + " (ping): got confirmation");
        }
  }
}
Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

I like your approach. Do you see any performance bottlenecks here? Is there a better way of doing it?
The code is a bit ugly, but there shouldn't be any performance overhead. I found one more similar question with more details in the answer - stackoverflow.com/questions/9555413/…. There might be something better in the util.concurrent package - but using synchornized I can't think of any thing better.
Thanks for the response. This will help.
Be careful of the low probability case when both names are equals - you can still get a deadlock in that case. You could use System.identityHashCode() to reduce that risk. For total security, you can add a clause where if the to hashcodes are equal, you use a separate lock that is only used in those cases.
Thanks, I have updated my answer. I think the hashCode can be same only if the objects are same. If the both objects are same the order doesn't mater - the second synchronized will be a noop (synchronized acquires a reentrant lock)
|
0

This is tricky stuff. I'd make Monitor.name volatile, or synchronize it with its own lock object. Or best yet: make it final. Then ditch the synchronized keyword from your two methods. Other than "name", there's nothing in there that's not thread safe.

Otherwise, don't synchronize until you need to. Use synchronized blocks, not methods. Don't put anything into a synch block unless it needs to be there. Synch fields separately on separate lock objects if you can. Don't nest synch blocks, but if you have to always nest them in the same sequence; have a hierarchy of synch objects.

Synching all your methods is a simple, handly technique for staying thread safe. However, it's easy to deadlock or just slow way down when your threads start interacting. Then it gets interesting.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.