wood burning stoves 2.0*
The moose likes Developer Certification (SCJD/OCMJD) and the fly likes Lock and Unlock Design  Question Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Certification » Developer Certification (SCJD/OCMJD)
Bookmark "Lock and Unlock Design  Question" Watch "Lock and Unlock Design  Question" New topic
Author

Lock and Unlock Design Question

Raju, Gentle
Greenhorn

Joined: Sep 06, 2001
Posts: 28
I have written lock and unlock code. But I still doubt this code is not thread safe. I am no tracking the clinet Id. Can some one
comment on this ?.
-------------------------------------------------

I will call the above methods in the bookFligeht in the following order.
try{
lock
read
update Seats
modify
}catch(Exception){
Some exception
}finally{
unlock
}
-----------------------------------

[This message has been edited by Raju, Gentle (edited September 23, 2001).]
[This message has been edited by Raju, Gentle (edited September 23, 2001).]
[This message has been edited by Raju, Gentle (edited September 23, 2001).]
[This message has been edited by Raju, Gentle (edited September 23, 2001).]
[Inserted CODE tags -- PdH]
[This message has been edited by Peter den Haan (edited September 24, 2001).]
Peter den Haan
author
Ranch Hand

Joined: Apr 20, 2000
Posts: 3252

The client ID ought to be tracked in some way in order to satisfy the requirements. Also, where is this code living? If it's in Data, reconsider.
This code is threadsafe in principle. The "indexOf() != -1" becomes a lot clearer if you replace it by "contains()". However, your lock data structure is inappropriate; the use of indexOf() tells me it's a List, probably an ArrayList or a Vector. It is inappropriate for two reasons.
The first reason is that it does not express the constraints on the data you want to hold. A List is an ordered structure which allows duplicate elements. But you don't care about the order in which locks are held. Also, a record lock can be granted only once, which means that you don't want duplicates. This indicates a Set and not a List.
The second reason, closely related, is that the code above takes O(n) time, where n is the number of outstanding locks. Replacing m_lock by a HashSet reduces this to O(1). (NB: do not be tempted to use a synchronized HashSet. It doesn't buy you anything).
There is a slip-up in the code; the wait() method will throw an IllegalMonitorStateException because at that point you own a lock on m_lock, not on this. Looking at your unlock() method, you probably meant to write m_lock.wait().
This code is not threadsafe in conjunction with your lock() implementation; the latter synchronizes on m_lock, while lockDatabase() synchronizes on this. As a consequence they may be executed in parallel.
A more fundamental problem is that this method is happy to grant database locks while there are outstanding record locks - true, no new record locks will be granted, but surely the semantics of a database lock is that no other client has any kind of lock.
Finally, style; CAPITALS are normally used only for static constants. Also, DATABASE_LOCK apparently is an int, but you are using it as a boolean; make it a boolean. Furthermore, if there are "magic values" such as 1 or -1, they are best declared as static constants (eg public static final int DATABASE_LOCK = -1). Finally, don't catch "Exception" if all you need to catch is "InterruptedException"; it obscures your intention and can easily introduce bugs in your exception handling - by the way, shouldn't this behave like the lock() method does and throw an IOException?
To amplify the remarks above: the notifyAll() will fail to wake up wait()ing record lock threads if they are wait()ing on m_lock. Database locks and record locks should clearly be synchronized on the very same object.
Hope this helps,
- Peter
Raju, Gentle
Greenhorn

Joined: Sep 06, 2001
Posts: 28
Hi Peter,
Thanks for pointing the problems in the code. One of the comment that you made is "If I am putting this code in Data class consider changing the place." But the requirement is that lock and unlock should be implemented in Data. Am I missing some thing here ? Also I have read that it is not a must that client Id needs to be tracked. Comments please ?
Raju, Gentle
Greenhorn

Joined: Sep 06, 2001
Posts: 28
Hi Peter,
Thanks for pointing the mistakes in the code. You said not to put this code in Data ? I think the requirement is to implement lock and unlock in Data class. Am I missing some thing here ?
Please let me know if there is a better approach than this. You also said that ClientId is must. But some where here I have read it is not must. Comments Please ?
Peter den Haan
author
Ranch Hand

Joined: Apr 20, 2000
Posts: 3252
Originally posted by Raju, Gentle:
"If I am putting this code in Data class consider changing the place." But the requirement is that lock and unlock should be implemented in Data.
Nope, it isn't. It is left up to you whether you implement your code in Data, a subclass of Data, or wherever. In terms of responsibilities, you can make a case for giving Data merely the responsibility for implementing a local, single-user database. In that case, Data only needs empty stubs for the locking methods. A separate class would be responsible for adding a multi-user, networked layer on top of a Data instance; it is this class which would implement nontrivial locking methods.
Am I missing some thing here ? Also I have read that it is not a must that client Id needs to be tracked. Comments please ?
The requirements don't state that explicitly, but it is implicit in the requirement that a duplicate call to lock() should return immediately. However, it is true that quite a few people have simply ignored it. It may cost you some points but it won't prevent you from passing.
- Peter
 
 
subject: Lock and Unlock Design Question