Win a copy of The Little Book of Impediments (e-book only) this week in the Agile and Other Processes forum!
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

NX: lock(Data data, int recNo) allowing multiple locks of one record!! why???

 
Dave Knipp
Ranch Hand
Posts: 146
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi,
I'm having some trouble with my lock method and was curious anyone can see a problem with it.

I'm doing my database testing right now and it seems that multiple threads can lock the same record # at the same time and i dont know why! lockRecords is a synchronized hashmap that stores the instance of Data trying to the get the lock on the specified record number. Each client/thread has their own instance of Data and will lock the requested record number only if the record number is not already locked by someone else, or at least its supposed to. Can anyone tell why its not!?!??!?
Thanks,
Dave
 
Jay Bromley
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello,
I'll take a whack at it. While from the code snippet you posted one cannot be certain, I'll bet a cup of coffee that your Data.doesRecordExist method is operating on some instance object (the HashMap you mention, I assume) instead of a class object (like a static HashMap). Thus, when a Data instance tries to lock something it is only checking records it locks and has no knowledge of records other Data instances lock.If this is correct the solution would be to replace with in your Data class.
So, do I owe you a cup of coffee?
Regards,
Jay
 
Dave Knipp
Ranch Hand
Posts: 146
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jay,
I think i should have explained a few things more in depth. I actually do my locking, lock(), unlock(), isLocked(), hasLock(), in my LockManager class which is maintained as a singleton so only one instance is passed around to all the different instances of Data (which are the threads of all clients connected). So i have one private static Map lockedRecords; and one instance of LockManager that all threads have call hasLock(Data, recNo) to check before performing any actions, delete, read, update.
So i don't think its what you suggested. Any other suggestions
Dave
 
Filip Moens
Greenhorn
Posts: 24
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Dave,
first of all, threading issues are not my forte, so be warned.
I 'll try to explain what I see as the possible problem by an example.
Thread 1 acquires a lock.
Thread 2 tries to lock the same record and goes into the wait state.
Thread 3 tries to lock the same record and goes into the wait state.
Thread 1 unlocks the record (probably it calls notifyAll).
Thread 2 leaves the "while(lockedRecords.containsValue(new Integer(recNo)))" loop and executes for example the lines
"log.info("Got Lock = " + Thread.currentThread().getName());
System.out.println("Got Lock = " + Thread.currentThread().getName());"
Then it is interrupted.
Thread 3 leaves the "while(lockedRecords.containsValue(new Integer(recNo)))" loop and locks the record.
Thread 2 continues and locks the record too!
 
Jay Bromley
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Dave,
I owe you a cup of joe then! With your additional comments things
have started to make more sense. One thing that caught my attention is that you pass the data instance reference into the lock method and use this as a key. This perhaps is the source of the problem. A HashMap can only hold one value per key, so as soon as a Data instance tries to store a second record number in the lockedRecords HashMap using its reference as a key, the first record number goes away, allowing someone else to lock it. In effect, each Data instance can only lock one record, since adding a second record with the same key (the Data reference) eliminates the previous value for the key. To wit, from the javadoc for HashMap.put(...):

public Object put(Object key,
Object value)
Associates the specified value with the specified key in this map. If the map previously contained a mapping for this key, the old value is replaced.

(Later that day...) I was looking at this again, and while the post is correct, it would only be pertinent if one Data instance could lock multiple records. In the assignment I'm doing (B & S contractors) multirecord locking isn't needed, but perhaps you've got some other requirements in your assignment.
Regards,
Jay
[ November 22, 2003: Message edited by: Jay Bromley ]
 
Jay Bromley
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Filip,
I think the locking itself is ok, it looks pretty much text book. Let's take a look (I'm verifying my understanding of Java threading here...)

Thread 1 acquires a lock.
Thread 2 tries to lock the same record and goes into the wait state.
Thread 3 tries to lock the same record and goes into the wait state.
Thread 1 unlocks the record (probably it calls notifyAll).

So far, so good, at this point threads 2 and 3 are waiting on the lockedRecords object and as you say thread 1 releases the lock with notifyAll(). At this point there is a mad scramble between threads 2 and 3 for the lockRecord's lock. Let's assume 2 wins.

Thread 2 leaves the "while(lockedRecords.containsValue(new Integer(recNo)))" loop and executes for example the lines

Then it is interrupted.
Thread 3 leaves the "while(lockedRecords.containsValue(new Integer(recNo)))" loop and locks the record.
Thread 2 continues and locks the record too!

Ok, here, I think things happen a little differently. When thread 2 is executing the code you mention above it has reacquired the lock on lockedRecords (remember objects don't hold locks while they wait, but they do reacquire them when notified.) That is, no one else (i.e. thread 3) can enter the code following the while, even if thread 2 is interrupted. So, thread 2 gets the lock and locks the record by placing it in the lockedRecords map and then exits. At this point thread 3 gets the lockRecords lock object, but it stays stuck waiting in the while loop until thread 2 unlocks the record by removing it from the lockedRecords map.
Does this sound right/good? I'm somewhat new to Java and definitely not a threading guru, so I could be wrong here, but I'm fairly convinced the above is what happens with respect to synchronization.
Regards,
Jay
 
Filip Moens
Greenhorn
Posts: 24
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Jay,
you are absolutely right.
I'll be a bit more careful before posting stupidities like I did.
Filip
 
Jay Bromley
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Filip,
No sweat. I think nearly everyone in this forum is hear to learn and has made a mistake somewhere along the line. I was just kicking myself about my earlier post about the HashMap. It's true, but probably not applicable.
Take care,
Jay
 
Stephen Galbraith
Ranch Hand
Posts: 90
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Dave,
Just a thought, (it may be very wrong!), but is your LockManager's Map a WeakHashMap with the Integer object the key?
If so, what happens when you replace it with a HashMap. Does it work now?
If this is the case, you'll need to manage the creation of the Integer objects as these may get removed by the garbage collector and so remove the entry in the Map in the LockManager class without you knowing, causing some chaos.
Let me know if this helps or not.
 
Andrew Monkhouse
author and jackaroo
Marshal Commander
Pie
Posts: 12014
220
C++ Firefox Browser IntelliJ IDE Java Mac Oracle
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Dave,
I can't see any problem with the code that would cause multiple clients to lock one record.
As Jay mentioned, you might have a problem if your testing code is attempting to have each client lock multiple records simultaneously, since your current code can only allow each client to lock one record at any given time.
You might also want to verify that you do have only one instance of the LockManager (and I mean verify by adding debug code).
Some more general issues (not related to your problem):
There is the potential logic hole where a client gains a lock on a deleted record. That is: client A and B both try to lock record #5. Client A succeeds and deletes the record, then releases the lock. Client B then gains the lock on what is now a deleted record.
Creating a new Integer object on each itteration through the "while locked" loop is not very efficient. Perhaps you could create the one value earlier in the method, and then just use it wherever required.
Regards, Andrew
 
Stephen Galbraith
Ranch Hand
Posts: 90
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi Andrew,
I've got a few questions on your last post. Dave says his lockedRecords is an instance of a LockManager class which contains a Map.
As Jay mentioned, you might have a problem if your testing code is attempting to have each client lock multiple records simultaneously, since your current code can only allow each client to lock one record at any given time.

Is this assumption made because of the method call

This looks like a wrapper to the Map put method, but we don't know from the posting what is used as the key and what is the value (although it's very tempting to assume that Dave will not have changed the order in his call in the LockManager class - Dave can you clarify? ). If the order wasn't changed then I agree, but if the order has been changed so that the record is the key then I disagree (would this not be the more natural order - I mean that each record has a client that locks it? I agree then that if we're using a WeakHashMap, we'll also need to store the Integer in the Data class so it doesn't get garbage collected and remove the ref.).
There is the potential logic hole where a client gains a lock on a deleted record. That is: client A and B both try to lock record #5. Client A succeeds and deletes the record, then releases the lock. Client B then gains the lock on what is now a deleted record.
Again, this may be handled by Dave's LockManager. It could throw RecordNotFound on it's put method and he may have handled that there. I have a very similar architecture for this part of the assignment. My LockManger class has a method lock (like the put method in Dave's code), but this will check to see if the record still exists on locking and throw the exception if not.
So is it the case that if we did the above then we'd still have an issue? Or is it just like the assumptions we're making regarding Dave's implementation of the LockManager class?
 
Dave Knipp
Ranch Hand
Posts: 146
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello everyone,
First let me clear up some of the assumptions you have been making, since i didnt provide you with all of the details at first, sorry Each thread/client that is accessing the database has their own instance of Data. Data accesses Database which contains the actual database implementation. I did this so i could prevent threads from being able to unlock a record that they did not lock in the first place. Hence the HashMap (not a weak hash map ) storing the instances of Data as the keys and mapping to the record number that is locked.
Andrew, thats a great suggestion, i didnt think about it having to reconstruct the integer object everytime, i'll make that change and save some memory
I did finally find the flaw in my logic, it was not in the lock method, but in my book method. I was calling lock on the record number to book and then before i unlocked it was retrieving all of the records in the database via Data.read which requires a lock on the record it is reading. So i was calling a lock within a lock on the same record number. So i just changed my book method to only book the record and then had the client do an empty search to refresh the records after it was booked.
Hope that cleared some of the confusion up, but im pretty confident that my lock method is working pretty well now. The one change i did make since this post was that after the thread gets the lock within the lock method i have it do a check to see if the record still exists. So it would eliminate that one situation that was mentioned about the record being deleted while the thread was waiting to get a lock on it.
Thanks for all your replies,
Dave
 
Stephen Galbraith
Ranch Hand
Posts: 90
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Glad you got it sorted!
As ever, Andrew's assumptions turn out to be correct
In that case don't forget about Jay's And Andrew's other comments on multiple locking.
Also, what happens if your Client crashes when it has a record locked?
Regards, Steve.
 
Trym Moeller
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My assignment sayes to use no cpu when waiting to get the lock on a record.
It seems to my that your lock waits on the map instance in which case all waiting will have to figure out if their lock has been released, but only one will have a go - is that part of your assignment or has anyone considered it?
Best regards Trym
 
Jay Bromley
Ranch Hand
Posts: 48
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hey Trym,
My assignment has the same requirement (I'll bet all assignments do, things would be pretty slow if waits consumed cpu time...) but I think Dave's setup covers it. Remember, while in a wait, threads are not runnable, and so consume no cpu time.
You are correct in that everyone waiting on lockRecords will be notified and made runnable. Each waiting thread will (in an undefined order) go through the while loop once more before continuing and so only be able to run if the record it is waiting for has been unlocked. Otherwise, the thread waits yet again. This applies whether two threads are waiting for different or the same record.
Regards,
Jay
 
Trym Moeller
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
As you said "Each waiting thread will (in an undefined order) go through the while loop once more before continuing and so only be able to run if the record it is waiting for has been unlocked. Otherwise, the thread waits yet again." which I read as the thread is doing some work, unless it was the lucky thread which had its record unlocked. I am currently trying to make a lock on record level, but cannot find a way to handle this in all situations.
pseudo code:
Map aRecNo2LockCookieMap;// map from recNo to lockCookie
public long lock (recNo) {
boolean tryAgain = true;
while (tryAgain) {
Object lock = aRecNo2LockCookieMap.get(recNo);
if (lock != null) {
sync(lock) lock.wait();
}
// here another thread can get in//
sync(aRecNo2LockCookieMap) {
if (aRecNo2LockCookieMap.contains(recNo)) {
continue;
} else {
tryAgain = false;
aRecNo2LockCookieMap.put(recNo, lock != null ? lock : getUniqueLock());
}
}
}
}
public void unlock(recNo, lock) {
sync(aRecNo2LockCookieMap) {
Object lock = aRecNo2LockCookieMap.remove(recNo);
}
lock.nofify();
}
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic