Do I need notify()/wait() etc. when I just mark all Data-methods synchronized?

[oli mueller]

Hi all,

After a bit of reading, it appears that a number of people have successfully passed with solving the thread-safety issue by marking ALL Data-methods as synchronized. I am also leaning towards this solution, because I believe it is the simplest/safest solution and according to the spec simplicity is more important than performance (I hope the junior programmer will like my approach...;)...)

Now, my humble question - could you reassure IF I am correct:

(1) IF I do it the easy way (all methods synchronized), I do not need any wait/notify-call EXCEPT within the lock/unlock()-method because of the requirement that "If the specified record is already locked by a different client, the current thread gives up the CPU and consumes no CPU cycles until the record is unlocked".

(2) Within the lock/unlock() methods the wait/notify calls should be invoked on the Data-object - and NOT on the hashmap, holding the locks. correct? Because otherwise, something like this could happen:
Thread A: lock(1)
Thread B: lock(1) // Thread B entered the lock method after Thread A had left it, but now it keeps waiting for recNo 1 to be unlocked...Thread A however cannot unlock it, because Thread B blocks the whole Data-class...hello deadlock..;)

Thanks a lot for your help.
x.

[Roel De Nijs]

(1)
(2)

[oli mueller]

Hi,

thanks for your answer, however i managed to run into a deadlock (synchronized ALL methods; in lock() I wait on "this"; in unlock() I call this.notifyAll() just before the end)

Here is the deadlock scenario: I have two treads T1 and T2. Both do a lock on record 5, sleep a bit and then shall call unlock on record 5
T1: lock(5) //T1 successfully receives the lock and leaves
T2: lock(5) //T2 must wait because lockMap contains record 5, thereby blocking the whole Data object
--> now T1 cannot enter the unlock()-method anymore, because T2 is still waiting within the lock()-method...DEADLOCK..

This is what I tried to resolve it (without success)
(1) I removed the synchronized keyword from unlock, but this causes a IllegalMonitorState-Exception at this.notifyAll()...no wonder...
(2) I removed the synchronized keyword from both lock/unlock und synchronized on this.lockedRecordsMap instead...again i run into a deadlock...

...it is like chicken and egg....when I wait within the lock-method, I prevent other threads from releasing the lock...grrr...

I wonder if it is not right how my wait within lock is implemented:

 while (this.lockedRecords.containsKey(recNo)) {
        try {
          this.lockedRecords.wait();
        } catch (InterruptedException e) {
            // clear interrupt status
          Thread.currentThread().interrupt();
        }
      }

[Roel De Nijs]

Hi Oli,

What's the behavior you'll see to conclude your program has a deadlock? And which test program are you trying to run? Because my lock-method calls wait (when a record is already locked) and my unlock-method calls notifyAll, both on the this-object without deadlocking.

Kind regards,
Roel

[oli mueller]

Hi Roel,

I have written a simple test myself. I create two threads t1 and t2. Within the run()-method of t1, it calls db.lock(5), then sleep(10), then db.unlock(5). Within the run()-method of t2, it calls db.lock(5), then sleep(7), then db.unlock(5). Additionally, I have made prints after each method call, and also within the lock/unlock methods, both at the very beginning and at the end of the method. The expected behaviour is that BOTH threads will at some point get the lock and also release the lock at some point.

The behaviour i am getting is usually that I see t1 successfully receiving the lock, then I see t2 entering the lock()-method. That is it...my explanation is that t1 cannot call unlock(), because t2 is already blocking the Data-object while it is waiting for t1 to call notify() within unlock(), which of course will never happen as t1 cannot even enter unlock() and so on and so on and so on...;)...

I would really appreciate your help on this, as for me the whole wait/notify-thing appears to be a chicken-egg problem...I mean, because wait/notify must be called within a synchronized-block (in my case the synchronized in the method signature), both threads prevent each other from carrying on...

THANKS...

ps.: I start both threads properly with start()...;); the sleep-times are arbitrary - I get the same problem with other sleep-times...

[Roel De Nijs]

During break I created a small program which simulates your approach. But it doesn't deadlock (and I tried with several sleep times).

public class LockTest {
	public static void main(String[] args) {
		Runnable runnable = new Runnable() {
			private final MyObject o = new MyObject();

			public void run() {
				o.lock();
				try {
					Thread.sleep(100);
				} catch (InterruptedException e) {
					e.printStackTrace();
				}
				o.unlock();
			}
		};
		new Thread(runnable, "t1").start();
		new Thread(runnable, "t2").start();
	}
}

class MyObject {
	private boolean locked = false;

	public synchronized void lock() {
		System.out.println(getThreadName() + " in lock");
		while (locked) {
			System.out.println(getThreadName() + " has to wait");
			try {
				wait();
			} catch (InterruptedException e) {
				e.printStackTrace();
			}
		}
		locked = true;
		System.out.println(getThreadName() + " out lock");
	}

	public synchronized void unlock() {
		System.out.println(getThreadName() + " in unlock");
		locked = false;
		notifyAll();
		System.out.println(getThreadName() + " out unlock");
	}

	private String getThreadName() {
		return Thread.currentThread().getName();
	}
}

A possible output:

t1 in lock
t1 out lock
t2 in lock
t2 has to wait
t1 in unlock
t1 out unlock
t2 out lock
t2 in unlock
t2 out unlock

[oli mueller]

Hi Roel,

ok, I found out what it was....the solution to my mysterious deadlock is rather simple....I wrote my test as a junit test, and apparently junit terminates all threads in the end...so, actually i didnt have a deadlock - I just didnt realise that all threads were just killed..;)...Running the same program as a normal java-application or putting join() in the end brings the expected outcome....stupid mistake...

THANKS a lot for your help!!!...

[Sean Keane]

oli mueller wrote:I wrote my test as a junit test, and apparently junit terminates all threads in the end...so, actually i didnt have a deadlock - I just didnt realise that all threads were just killed..;).

Hi Oli, I plan to use JUnit to test my code, so I'm interested in this. But I don't understand how using JUnit caused you problem in your locking\unlocking - do you have a concrete example?

[oli mueller]

Hi,

I basically did what roel described - just within a junit-test

@Test
  public void lockTest() throws RecordNotFoundException {  
    Thread t1 = new Thread(){
      public void run(){lock, sleep, unlock}
    }

     Thread t2 = new Thread(){
      public void run(){lock, sleep, unlock}
    }

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

     try {
      t1.join();
      t2.join();
    } catch (InterruptedException e) {
      e.printStackTrace();
    }
}

..without the joins caused me problems, so I added joins after it, so that the junit thread waits until t1 and t2 are finished
..with the joins it works fine...

[Sean Keane]

Roel De Nijs wrote:During break I created a small program which simulates your approach. But it doesn't deadlock (and I tried with several sleep times).

(see code from previous post)

A possible output:

t1 in lock
t1 out lock
t2 in lock
t2 has to wait
t1 in unlock
t1 out unlock
t2 out lock
t2 in unlock
t2 out unlock

Hi Roel. I ran your sample program and I got the same output. However something is confusing for me, maybe you can explain?

Both the lock and unlock methods are synchronized. From this I understand that it is not possible for two different threads to execute either of these methods at the same time. So if T1 is executing unlock, then no other thread can execute either lock or unlock. Is my understanding correct here?

The reason I am confused is that I see this is the output:

t1 in lock
t1 out lock
t2 in lock

At this point, I assume t2 has the lock, so no other thread can enter any synchronized method on MyObject until t2 leaves the lock method and releases the lock it holds. But then I see this in the output:

t2 has to wait
t1 in unlock

Which obviously goes against what I was thinking. As t1 has gone into the unlock method whilst t2 is still in the lock method.

Is my misunderstanding simply because the two threads, t1 and t2, are using different instances of the MyObject class, thus have separate locks. So the effect of synchronizing the methods on MyObject in this example means that for a particular instance of the MyObject class only one thread at a time is allowed enter a synchronized method of that instance.

[Sean Keane]

oli mueller wrote:Hi,

I basically did what roel described - just within a junit-test

Thanks Oli, I modified Roels code, and got the exact problem you mentioned.

I've listed the modified code below. When I run this code, I simply see the following output:

t1 in lock
t1 out lock
t2 in lock
t2 has to wait

As you mentioned Oli, JUnit is not waiting around for these other threads to complete.

This article here talks about the problem, and has a solution, so it may be of use http://today.java.net/pub/a/today/2003/08/06/multithreadedTests.html

import org.junit.Test;

public class LockTest {
	@Test
	public void test(){
		Runnable runnable = new Runnable() {
			private final MyObject o = new MyObject();

			public void run() {
				o.lock();
				try {
					Thread.sleep(100);
				} catch (InterruptedException e) {
					e.printStackTrace();
				}
				o.unlock();
			}
		};
		new Thread(runnable, "t1").start();
		new Thread(runnable, "t2").start();
	}
}

class MyObject {
	private boolean locked = false;

	public synchronized void lock() {
		System.out.println(getThreadName() + " in lock");
		while (locked) {
			System.out.println(getThreadName() + " has to wait");
			try {
				wait();
			} catch (InterruptedException e) {
				e.printStackTrace();
			}
		}
		locked = true;
		System.out.println(getThreadName() + " out lock");
	}

	public synchronized void unlock() {
		System.out.println(getThreadName() + " in unlock");
		locked = false;
		notifyAll();
		System.out.println(getThreadName() + " out unlock");
	}

	private String getThreadName() {
		return Thread.currentThread().getName();
	}
}

[Roel De Nijs]

Sean Keane wrote:I ran your sample program and I got the same output. However something is confusing for me, maybe you can explain?

You are simply missing one of the most important things in java concurrency Reading the javadoc of Object's wait method will explain a lot.

You could aslo have tested your last assumption yourself: if you put a simple sysout in MyObject's constructor (or just add tje this instance to the sysouts in lock/unlock methods) and you'll see that just 1 object is created. Finally, if I would have given an example where 2 threads both have their own object to get the lock on, it would simply be a bad example (in the context of the OP's question).
Playing around with some small code examples (like the one I provided) will give you the possibility to learn a lot about concurrency (make a change to the code, think about what output you should see and then run the changed example and see if you got it correct). You can simply adjust the snippet and let both threads use their own object and see if you get the same output (I'm pretty sure it won't, because t2 will not have to wait at all, because locked will be false)

[Sean Keane]

Roel De Nijs wrote:

Sean Keane wrote:I ran your sample program and I got the same output. However something is confusing for me, maybe you can explain?

You are simply missing one of the most important things in java concurrency Reading the javadoc of Object's wait method will explain a lot.

God, what a stupid post! Can't think what I was thinking!!!

So we had the following in the output:

t1 in lock
t1 out lock
t2 in lock

Then I was confused...

Sean Keane wrote:
At this point, I assume t2 has the lock, so no other thread can enter any synchronized method on MyObject until t2 leaves the lock method and releases the lock it holds. But then I see this in the output:

t2 has to wait
t1 in unlock

Which obviously goes against what I was thinking. As t1 has gone into the unlock method whilst t2 is still in the lock method.

What I failed to see was the shared variable locked. This is set to true at the end of the lock method and it is not set to false until the unlock method is called. If locked is set to true whilst a thread is checking the condition in the lock method, then the thread will call wait() and release the lock it is holding on the particular instance of the MyObject class.

So looking at the part of the output again that I was confused about...

t2 has to wait (this makes sense, as t2 checks the shared variable locked to see if it is true, which it is, then calls wait(), this releases the lock on this instance of MyObject)
t1 in unlock (t1 is able to get into the unlock method, as t2 has released the lock that it had on this instance of MyObject when it called wait())

[Roel De Nijs]

Sean, that's a spot-on analysis