This week's book giveaway is in the OCMJEA forum.
We're giving away four copies of OCM Java EE 6 Enterprise Architect Exam Guide and have Paul Allen & Joseph Bambara on-line!
See this thread for details.
The moose likes Developer Certification (SCJD/OCMJD) and the fly likes Peter, please review LockManager Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of OCM Java EE 6 Enterprise Architect Exam Guide this week in the OCMJEA forum!
JavaRanch » Java Forums » Certification » Developer Certification (SCJD/OCMJD)
Bookmark "Peter, please review LockManager" Watch "Peter, please review LockManager" New topic
Author

Peter, please review LockManager

Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Hi Peter,
I am wondering if you could review my LockManager, thanks.


Aruna A. Raghavan<br />SCJP, SCJD, SCWCD
Bernhard Woditschka
Ranch Hand

Joined: Sep 16, 2002
Posts: 89
Hi,
I think your lock(-1) clearLocks() don't work correct.
If a client locks thw whole db he expects to have a lock on all records. Even if the (-1) lock is lockable there may still be a lot of single locks handed out. I sugest a 2 step process: get the -1 lock then wait until all other single locks are returned.
with clear locks i doubt that

removes all entries from the map where the value (not the key) is clientId
Hope this is usefull, Bern
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Bern,
Thx for taking a look. I agree that it doesn't handle locking when a -1 provided as rec number.
I will look at clearlocks() more closely.
Aruna.
John Smith
Ranch Hand

Joined: Oct 08, 2001
Posts: 2937
Also since your lockDB variable never changes, it should be declared as a private static final on a class level, -- there is no need to instantiate it every time.
Eugene.
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Thanks, how does the following look?
Bernhard Woditschka
Ranch Hand

Joined: Sep 16, 2002
Posts: 89
I'd think now you never lock the whole db
I did it like:
while (wholeTableLock) wait()
lock whole table
while (!siglelocks.empty()) wait()
[ January 13, 2003: Message edited by: Bernhard Woditschka ]
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Bern,
So you used two maps to keep track, one for wholedb locks, the other for singlelocks??
Aruna.
John Smith
Ranch Hand

Joined: Oct 08, 2001
Posts: 2937
Aruna,
What do you need clearLocks() for?
Eugene.
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Eugene,
In spite of your warnings on keeping it simple, I left the unreferenced()in the code. So, connection calls clearLocks when the client dies unexpectedly and unreferenced() gets called.
Aruna.
Bernhard Woditschka
Ranch Hand

Joined: Sep 16, 2002
Posts: 89
Actually I use one variable that stores the id of the client holding the lock on the whole table (or null if no lock) and a HashMap (like you) storing the discrete locks(key=record, value=clientid).
Bern
[ January 13, 2003: Message edited by: Bernhard Woditschka ]
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Hi Bern,
I just realized that and saw your posting while trying to post my new code. Thanks very much for all the help!
Aruna.
Bernhard Woditschka
Ranch Hand

Joined: Sep 16, 2002
Posts: 89
Your'e welcome
I like the javaranch, lots of friedly cowboys here
Peter den Haan
author
Ranch Hand

Joined: Apr 20, 2000
Posts: 3252
In addition to what has been said above---You could consider making this final. Not because it's necessary but because it tells whoever looks at this code that you never plan on modifying this variable. Good code provides as many clues to the reader as it can, through language constructs, coding conventions, names, and if all else fails, comments.
Making clientId an Object makes your LockManager reusable in completely different contexts. This is an example of the idea that classes should make as few assumptions as you can get away with. If your method argument is an ArrayList but it works just as well with a List, then use that. If it is a List but actually any Collection will do, use that. If it says FBNDataInterface but any Object would do...
Why didn't you let the LockManager absorb the InterruptedException? How are you handling it now? (This is not a criticism, just curiosity).
I would suggest defining a public final int DATABASE_LOCK = -1. Purists maintain that the only number literal you should ever have in your code is 0, and that all else should be defined as constants or configured in a configuration file. I wouldn't necessarily take it quite that far, but they have a point.

while (!mapLocks.isEmpty()) is slightly clearer.
I would suggest defining a public final int DATABASE_LOCK_KEY = new Integer(DATABASE_LOCK).
Seeing that an unlock() for a record you haven't locked in the first place is to be ignored, I personally felt that symmetry dictated that a duplicate lock() should be ignored as well. This is arguable though. In any way, to achieve this, replace mapLocks.containsKey(record) with (mapLocks.containsKey(record) && !mapLocks.get(record).equals(clientId)).
Instead of equals(), you could use simple object equality ==. Which one you choose is probably a matter of taste. If you use == here, then you'd also use it in the code I suggested above.
Shouldn't that be "... that may still hold locks"?
Modulo my bugs, the following is functionally equivalent:Some people don't like this use of the for() idiom; to my eyes, for() is the appropriate construct for any iteration over a fixed range or collection. But apart from that, iterating over the value Collection rather than the EntrySet and using Iterator.remove() does simplify the code IMHO. You dont need no steenkin' keys
There's a subtle bug in this method: if any clients are wait()ing for some of the locks you free up, they won't get woken up. You could notifyAll() inside the if statement; if you prefer, you can also maintain a boolean variable to keep track if any locks have been cleared and do a single conditional notifyAll() at the end. I probably wouldn't do this though. Your expectation would be that at most one lock is being held anyway, and if there are more, well, multiple calls to notifyAll() won't do any harm beyond burning a few more CPU cycles.
- Peter
[ January 14, 2003: Message edited by: Peter den Haan ]
Peter den Haan
author
Ranch Hand

Joined: Apr 20, 2000
Posts: 3252
D'oh. Twice I said "public final" where I meant "public static final".
- Peter
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Peter,
Thanks very much for reviewing it.
1) I don't know if I can make the data in LockManager static final. This will make all instances of it get the same mapLocks etc. I may want to pass another instance of LockManager to a different client that wants to use a different database table in the future...
2) I like the usage of "==" in stead of .equals. I come from C++ background and it took me awhile to not use "==" in the wrong places. So, I almost never use "==". But LockManager is one case where it should the reference we compare against, not the object. Very good point! WOW!
3) About InterruptedExceptions - I would like to tell the client something got hosed when this happens so I can display an error message. So I throw it from the LockManager to Connection. My DataFacade handles it. Are there any better ways to handle it?

4) Regarding using FBNDataInterface type for clientID - I think you are saying that it can just be a plain java Object ref, doesn't even have to be FBNDataInterface. Did I understand it correct?
Thanks chief,
Aruna.
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Peter,
BTW, I see no harm in using
public final int DATABASE_LOCK = -1;
public final Integer DATABASE_LOCK_KEY = new Integer(DATABASE_LOCK);
But I hesitate to make the map static.
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Peter,
One more thing.
clearLocks() does not look too safe. It is accessing the map that some one else may have locked. Shouldn't it wait till every one else is done using it?

John Smith
Ranch Hand

Joined: Oct 08, 2001
Posts: 2937

One more thing.
clearLocks() does not look too safe. It is accessing the map that some one else may have locked.

Your map is only accessed from synchronized methods of your lock manager, therefore it is thread safe.
Eugene.
pascal auderset
Greenhorn

Joined: Aug 21, 2002
Posts: 15
Hallo
In the example you have an possible error in your unlock code. In the code
if (mapLocks.get(record).equals(clientId)) {
the get could return null.
get(Object key)
Returns the value to which the specified key is mapped in this identity hash map, or null if the map contains no mapping for this key.
Arjan Broer
Greenhorn

Joined: Dec 20, 2002
Posts: 20
Originally posted by pascal auderset:

if (mapLocks.get(record).equals(clientId)) {
the get could return null.

This is quickly solved by reversing the test
Aruna Raghavan
Ranch Hand

Joined: May 14, 2002
Posts: 194
Hi Pascal,
Yes, I have seen it while testing it and fixed it. Thanks for reviewing it.
Aruna.
Arjan Broer
Greenhorn

Joined: Dec 20, 2002
Posts: 20
Am i doing double work?
The assignment states that you need to implement the methodes

Without altering the method signatures i've implemented this in Data as simple basic record locking.
For more advanced locking functionality i also built a LockManager that maps record locks to clients. The LockManager is merly an administrative class that keeps track of the lock owners. When unlocking it validates if the client is the owner of the lock.
The LockManager does not lock records itself, it only requests locks from the Data object.
Am i on the right track here?
Peter den Haan
author
Ranch Hand

Joined: Apr 20, 2000
Posts: 3252
Originally posted by Aruna Raghavan:
Thanks very much for reviewing it.
My pleasure. You may have gotten more than you bargained for
1) I don't know if I can make the data in LockManager static final.
You can't, I was unclear, my apologies. The only static finals I'd proposed is DATABASE_LOCK and DATABASE_LOCK_KEY. You can make mapLocks final, but not static.
3) About InterruptedExceptions - I would like to tell the client something got hosed when this happens so I can display an error message. So I throw it from the LockManager to Connection. My DataFacade handles it. Are there any better ways to handle it?
It boils down to a design choice: if and how do you handle interrupts?
  • You do support interrupts but don't handle them, so you propagate them to the client code to be handled elsewhere. That's what you're doing right now.
  • You don't support interrupts at all. Then you'd probably catch them and re-thrown them wrapped inside a RuntimeException.
  • [list]You do support interrupts by ignoring them. You would stick your wait() into a while loop like so:
  • You do support interrupts as a way to handle fault conditions in the database, for instance as a way to handle deadlocks or to kick out waiting clients during shutdown. In that case, you would catch InterruptedException and re-throw it wrapped inside a DatabaseException
  • You see that this is not purely a matter of taste or expediency but a genuine design decision: what role do interrupts play in my database?
    4) Regarding using FBNDataInterface type for clientID - I think you are saying that it can just be a plain java Object ref, doesn't even have to be FBNDataInterface. Did I understand it correct?
    Yes. And the nice thing of having an Object there is that LockManager loses all dependency on the FBN project -- it becomess a pretty generic implementation of a certain type of locking semantics.
    clearLocks() does not look too safe. It is accessing the map that some one else may have locked. Shouldn't it wait till every one else is done using it?
    This has already been answered: it's safe because it's synchronized like anything else manipulating the Map.
    - Peter
    Aruna Raghavan
    Ranch Hand

    Joined: May 14, 2002
    Posts: 194
    Hi Peter,
    But clearLock() should also check for wholeDBLocked flag and also do a notifyAll() I think.
    Aruna.
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Originally posted by Aruna Raghavan:
    But clearLock() should also check for wholeDBLocked flag and also do a notifyAll() I think.
    Doesn't it automatically do the latter, as the database lock is in many ways just another lock in the map? I think I mentioned the notifyAll() at the end of my first response, above.
    - Peter
    Kevin Cao
    Ranch Hand

    Joined: Jun 19, 2002
    Posts: 55
    Peter,
    You can make mapLocks final, but not static.

    Can you explain why this is the case? The mapLocks will keep track of all the records that are locked at any given time, should it be a class level variable?
    Thanks,
    Kevin
    Peter den Haan
    author
    Ranch Hand

    Joined: Apr 20, 2000
    Posts: 3252
    Originally posted by Kevin Cao:
    Can you explain why [mapLocks can be final, but not static]? The mapLocks will keep track of all the records that are locked at any given time, should it be a class level variable?
    As LockManager doesn't keep track of the Data it belongs to, you need one LockManager (and one mapLocks) for each Data instance. Making mapLocks static would introduce a completely arbitrary, unnecessary and crippling restriction in your design, namely that you could only ever have one networked Data instance per JVM.
    How many applications do you know that can get by with just one database table?
    - Peter
     
    It is sorta covered in the JavaRanch Style Guide.
     
    subject: Peter, please review LockManager