Bodgitt and Scarper : locking and unlocking

[Anayonkar Shivalkar]

Hi,

I'm having few doubt regarding locking and unlocking:

1) Since it is not mentioned in document (and Roel made it clear), i agree that there's no need to put explicit lock/unlock buttons(i.e. we don't have to give client the option of locking and unlocking explicitly, right?), is it necessary to lock the record first, then provide the new data(for modification) and then modify the record?


However, in this case, client will first provide the data and then only it will be able to update it (rather client won't be able to actually 'lock' record, but server would do it). Also, by following this approach, I could get rid of client crashes and long-time locks (there won't be cases like someone locks the record, and forget to unlock etc.). Going further, I can also get rid of client identification.

So is this valid approach?

2) Since lock method is void, how to make sure that we actually got a lock? Please reply.
Also, in above approach, due to 'if(record is not locked)' I guess we can safely invoke 'lock' (what if after we check that record is not lock, and actual invokation of 'lock', some other thread gets a lock? that is why I got a master lock there).
Let me know if you think about any flaw(s) here.

3) When we delete a record, is it necessary to unlock it? It will throw RCNF (record not found) exception anyway. How about simply removing the entry from lock-info map? Also, if I follow approach mentioned in first query above, this map itself will be unnecessary, right?

Thanks in advance.

[Roel De Nijs]

1/ you can follow a thin client approach: your server is calling the necessary methods on the Data class and your gui will call 1 method from your business service. To update a record you should have something like:

data.lock(1);
data.update(1, newData);
data.unlock(1);

So calls to lock and unlock method should be not included in the update or delete method.

2/ if the method successfully completes, the record should be locked. Otherwise a RNFE or another exception should be thrown.

3/ throwing a RNFE from the unlock-method after deleting a record would be a bit stupid. But you need to call the unlock-method, because otherwise the record would be locked forever and what if another thread is also waiting on this deleted record. The application would never run to completion.

[Anayonkar Shivalkar]

So calls to lock and unlock method should be not included in the update or delete method.

Why so? For update method, I guess there must be a lock (otherwise we'll have same problem of two threads updating same record etc.)

throwing a RNFE from the unlock-method after deleting a record would be a bit stupid. But you need to call the unlock-method, because otherwise the record would be locked forever and what if another thread is also waiting on this deleted record. The application would never run to completion.

Well, then what I can do is : simply remove the entry of lock info of deleted record from the map. It will work because:

1) Due to master-lock, when one thread does some operation, another thread will simply wait for master lock. After a thread finishes, another thread will first check if the record can be locked and then only try to lock the record.

2) Thus, all threads are actually waiting over master-lock and then those will attempt to get actual record lock.

3) Since I'm not reusing the record number, nobody will get lock on record number 'x' once the record is deleted (isLocked will throw RNFE).

p.s. I've used master-lock (a simple static lock) just to ensure that once one thread confirm that a record is not locked and can be locked by that thread, other thread should not step in between and lock the record. I was thinking of synchronizing over lock info map in lock,unlock,islock etc., but that map is also static, so it's gonna be a class level lock anyway. Code with static master-lock is easier to read. What say?

[Roel De Nijs]

Anayonkar Shivalkar wrote:Why so? For update method, I guess there must be a lock (otherwise we'll have same problem of two threads updating same record etc.)

Because one of the good design principles is 'each method should have one well-defined purpose", so for the update-method that's updating a record, not locking and unlocking.

Anayonkar Shivalkar wrote:simply remove the entry of lock info of deleted record from the map.

You could wonder if it's needed to check for a valid record number in methods like update, delete and unlock.

Anayonkar Shivalkar wrote:I've used master-lock (a simple static lock)

So you have 1 static object which you synchronize on to make your code thread-safe? And when you need a bunch of statements as an atomic operation (other threads are not allowed to step in and spoil the fun ) you just use synchronized (staticObject) { } around these statements? Sounds reasonable to me.

[Anayonkar Shivalkar]

Hi,

For DB operations, I'm using below approach:

1) Client cannot explicitly lock/unlock records
2) Client can only call 'business' methods like display record, book a record etc. and server internally locks - performs operation - unlocks the record (i think this is what 'thin client' does mean, right?)
3) If lock cannot be obtained on a record, there will simply be a message stating that record is already locked (or record does not exist; whatever applicable)

Now, 3rd point mentioned above is bugging me. As per requirements "Any attempt to lock a resource that is already locked should cause the current thread to give up the CPU, consuming no CPU cycles until the desired resource becomes available."

Does this mean that after the desired resource becomes available, lock should be obtained? Does my code needs to call wait during obtaining a lock? As of now, I haven't made a single call to wait because I'm obtaining a class level (static) lock before even checking if record is locked etc. Thus all other threads will automatically wait and put on hold by master-lock.

Is this approach complying to requirements? Please advice.

Thanks.

[Roel De Nijs]

(i think this is what 'thin client' does mean, right?)

3) If lock cannot be obtained on a record, there will simply be a message stating that record is already locked (or record does not exist; whatever applicable)

That's against a must requirement, so that means automatic failure!

"Any attempt to lock a resource that is already locked should cause the current thread to give up the CPU, consuming no CPU cycles until the desired resource becomes available."

That simply means if 2 threads A and B try to lock the same record, threadB will lock the record, threadA has to wait until the lock on the record will be released again (that can only happen when threadB calls the unlock method with the number of that record). Then threadA will try to lock that record again. So you don't display a message to the user "Sorry, the record is locked. Please try another one."

[Anayonkar Shivalkar]

That's against a must requirement, so that means automatic failure!

Actually, since I'm using master-lock phenomenon, once thread A obtains lock on record 'x', thread B will wait till thread A finishes with unlock. Then thread B will go for requesting lock on record 'x':

master_lock.lock
if(record 'x' not locked) {
x.lock
do stuff here
x.unlock
}
master_lock.unlock

Thus, thread B is anyway waiting till thread A finishes (and there won't be any message display like 'sorry you cannot get lock' etc.). So I guess I'm complying with must requirement, right?

I thought of using 'await' in lock method and 'signal' in unlock method (I'll still implement it anyway, so that, direct call to 'lock' outside synchronized context would wait till the record is available for locking). But now using both await/signal and master_lock is confusing me.
Because, anyway, only one thread is gonna get lock and 'do stuff' at a time - thanks to master lock, and thus there's no need for await/signal calls in locking methods.
And if I use await/signal, then there won't be any need of master_lock (when thread A gets the lock on record x, thread B will wait till A releases lock). Right? (please let me know if I'm right here)

So, what should I do?

Roel, as per your reply, I understand that:
1) If I make calls to await/signal via my locking mechanism, I would be complying to requirements, and there won't be automatic failure.
2) Further, if I go for master_lock (i.e. a static lock), the actual calls to await/signal will hold no significance ('condition' in while loop will never be satisfied, and await won't be called. further, there won't be anyone waiting for 'signal' so it is useless there). But it will provide an extra level of thread safety (and I guess there won't be automatic failure for this case either).

So, my current approach is to have both : await/signal and master_lock. Please advice.

Also, I read about someone who failed because he did not call wait and notify method from lock method in Data class.

What I've done is:
1) Create another class LockManager which contains reserve/release methods
2) Data class contains lock and unlock
3) lock invokes reserve and unlock invokes release
4) (now) reserve will call await and release will call notify

Is this approach valid? I guess so (since I'm calling await and signal - also I guess calling those methods instead of wait/notify is fine - since those methods are equivalent of each other). Please suggest.

Also, I'm allowing one client to lock only one record (to avoid deadlock). For this, I'm having a private boolean variable. When client gets a lock, variable is set to true.

What should happen if same client tries to lock another record? Is it fine if I throw an exception (which is child of RecordNotFoundException) and catch it at one level above 'lock' method? I'm really confuse over this. Does requirements expect me to wait here as well? I'm trying to avoid this (more locks by one client) situation because I can run into deadlock very quickly here. Please suggest.

And if client tries to lock same record for second time? Should I show message or not giving 'another' lock and pretend that nothing happened?
(Due to master_lock, those conditions are theoretically impossible, but anyway I would like to know)

I know I'm asking too much, but this(locking) is the most dangerous area of certification (and that is why it carries 80 points).

Roel, thanks a lot for making me aware of this big mistake (i.e. I must not show 'no lock for you' kind of messages).

[Anayonkar Shivalkar]

Hi,

To avoid further confusion, I've decided to follow below approach:

1) There won't be a seperate class to handle locking mechanism. Data class itself will be handling it.
2) IsLocked method would be called only via lock method (i.e. if record is locked then wait).
3) Master lock would be used only to lock and unlock and lock-update-unlock operation would not be atomic. Since updation would happen only after lock is obtained, I don't think there is any need to make all three operations atomic. Please suggest if this might cause locking issues/data corruption (I don't think so).
4) Client would only call 'update' method and update would internally call lock, update file and unlock.

Let me know if this is valid approach. Also

1) Since lock and unlock methods are public, should I consider scenario like in future someone may call those methods directly? In that case my code might fail. Currently, only methods from Data class will be calling lock and unlock methods.
2) Do I have to expose lock and unlock method via RMI?

[Roel De Nijs]

I already told you methods from Data class should not call lock/unlock.

If you use a thin client approach, you only expose business methods through RMI, lock/unlock methods are not business methods. find, book,... are business methods and these methods will call the methods from the Data class.

[Anayonkar Shivalkar]

Thanks Roel.

Now, Data class is not calling lock/unlock internally. So business level code is something like (pseudo code):

try {
lock(recNo)
bookRecord(recNo)
}
finally {
unlock(recNo)
}

Now, my doubt is about isLocked method. Should I call it internally in lock code? Currently I'm 'await'ing in lock method over output of isLocked:

while(isLocked(recNo))
await()

Is this OK? Or should I move this code to business level (i.e. wait till record is locked, and then call lock method) - I think this second approach would add some complexity. Please suggest.
Thanks.

[Roel De Nijs]

Anayonkar Shivalkar wrote:Is this OK? Or should I move this code to business level (i.e. wait till record is locked, and then call lock method)

According to the comments of the lock-method a thread should wait when a record is already locked, so I would keep this logic in the Data class.

[Anayonkar Shivalkar]

Thanks Roel.

I'm doing the same thing. My only doubt is : is it OK if I never call isLocked method from outside Data class?

Currently, as mentioned, isLocked is only getting invoked internally by lock method (if record is locked, wait till it gets unlocked and then obtain a lock).

[Roel De Nijs]

I'll guess there will be more than 1 method from Data class which are not invoked at all, so why would that not be OK?