An problem in Andrew's Denny Dvd?

[Leo Ho]

Folks, I think that there's a mistake in the following piece of code in Andrew's book:

class DvdFileAccess
...
  if (databaseLocked) {
    recordNumbersLock.writeLock().lock();
  } else {
    recordNumbersLock.writeLock().unlock();
  }
...

The thing is that if the network-mode is used, then the thread which calls unlock() may not be the thread that call lock(), thus it will be blocked forever. Just a small problem though, since in the app, Andrew only performs the call to setDatabaseLocked(true), never setDatabaseLocked(false)

How do you think?

Leo

[Mihai Radulescu]

Hi Leo

I am not 100% shore that I understant what you really mean and I am not the (untimate) expert in this new "util.concurrent.locks.*" but I don't see any reason to lock or unlock is done with the same thread.
In my implemention I have an extra thread to lock (read safe shutdown) my database.

Can you detaialte your question ?


Regards, Mihai

[Leo Ho]

Originally posted by Mihai Radulescu:
Hi Leo

I am not 100% shore that I understant what you really mean and I am not the (untimate) expert in this new "util.concurrent.locks.*" but I don't see any reason to lock or unlock is done with the same thread.
In my implemention I have an extra thread to lock (read safe shutdown) my database.

Can you detaialte your question ?


Regards, Mihai

Mihai, it's best if the same thread can call lockDatabase(true) and lockDatabase(false). But think of RMI, you don't have the control over which thread is used for the same RMI client, thus let's look at this situation:

- Thread A calls lockDatabase(true) to lock the database -> successful and the lock will be associated with Thread A
- Thread B calls lockDatabase(false) to unlock the database -> blocked since A is holding the lock.

Leo

[Mihai Radulescu]

Hi Leo

I just read the api and you are right :

A reentrant write lock intrinsically defines an owner and can only be released by the thread that acquired it. In contrast, in this implementation, the read lock has no concept of ownership, and there is no requirement that the thread releasing a read lock is the same as the one that acquired it. However, this property is not guaranteed to hold in future implementations of this class.

It seams tat you are right with the RMI a deadlock scenario can be posible.

Regards, Mihai
[ April 19, 2006: Message edited by: Mihai Radulescu ]

[Barry Gaunt]

I have searched the e-book for that code but cannot find it. What page is it on?

OK it's not in the book, but in the source code bundle...
[ April 19, 2006: Message edited by: Barry Gaunt ]

[Andrew Monkhouse]

Hi Leo,

You are correct, this code pottentially could cause problems in a mulit-threaded environment. However, as you pointed out, I only ever call setDatabaseLocked(true). Furthermore, I never call that function over RMI - I only ever call it from the CleanExit class, so I have a known thread that wont change. So my simplest solution is to add some comments to the method stating how it should be used.

Just to clarify one thing though - rather than blocking on the unlock, the code could potentially throw an exception:

From the API for ReentrantReadWriteLock.WriteLock:
If the current thread is the holder of this lock then the hold count is decremented. If the hold count is now zero then the lock is released. If the current thread is not the holder of this lock then IllegalMonitorStateException is thrown.

As an aside, this is a perfect example of one reason we recommend (both in the book and in this forum) that you should not go beyond the assignment - namely by going beyond the requirements I have created code that I might have lost some marks for.

Regards, Andrew
[ April 19, 2006: Message edited by: Andrew Monkhouse ]

[Leo Ho]

/* As an aside, this is a perfect example of one reason we recommend (both in the book and in this forum) that you should not go beyond the assignment - namely by going beyond the requirements I have created code that I might have lost some marks for. */

Thanks for the reminder, Andrew. I guess I will only include one method closeDatabase() which allows the server code to completely lock and close the database before shutting-down. And there won't be any openDatabase() so that the marker cannot argue about it.

Leo.

[Barry Gaunt]

Notice that IllegalMonitorStateException is a RuntimeException, and so does not have to be declared to be thrown or caught by code that uses it. So the problem could show up when the code is well into production usage.