lock record method doubt

[Mary John]

Hi all


I have a Map which holds list of all locked records as
private static Map<Long, Long> lockedRecords = new HashMap<Long, Long>();

Should I synchronise my lock method with the object lock on "lockedRecords"
ie with synchronised(lockedRecords), so that other methods( which may come in future enhancements )will not access lockedRecords while my lock method checks whether a particular record is present in it or not.

Or, is the code below which I have done with lock.lock() and lock.unlock() method just fine?

 
public long lockRecord(long recNo)
throws RecordNotFoundException
{

long lockCookie = rand.nextLong();
try
{
lock.lock();
while(lockedRecords.containsKey(recNo))
{

try{
lockReleased.await();
}
catch(InterruptedException intr)
{
intr.printStackTrace();
}
}
 
lockedRecords.put(recNo, lockCookie);
}
finally
{
lock.unlock();
}
 
return lockCookie;


}
   

[ April 09, 2008: Message edited by: Mary John ]

[Edwin Dalorzo]

Well, I assume that if you are writing a lock mechanism is because you know that multiple threads will be consuming this service simultaneously, right?

If two clients were invoking the services of your system simultaneously over the same record, how could you prevent that only one of them got the lock over the record.

Evidently, you cannot let two threads to modify your record map simultaneously.

[Gaurav Arora]

If the two threads use the same JVM then yeah, using a map would work. But if they don't, it won't.

Am I correct to assume that the locking is only required when the database access is provided over a network? And no locking is required for a local database?

[Edwin Dalorzo]

Well, I think you are right, at least that is the case in my implementation. I use RMI for networked clients, and you know that one never knows how many threads RMI might create behind the scenes. Therefore, I know that if I have at least two network clients using the database simultaneously, they could be assigned different RMI threads.

However, when I run the standalone application, there is only one client accessing the database, and therefore, the locking mechanism may sound unnecessary. My requirements are not clear enough about this. I mean there is not a single statement that says I should overlook the lock mechanism for the standalone mode. However, I do agree that in a standalone mode, overlooking the lock might improve performance, but at the end, I believe it will complicate much more your design. Now you will need to add a condition to all your update, delete and create methods to check the status of the mode flag and then decide whether to use or not to use the lock.

I think it is a good performance improvement candidate, but as with many other performance ideas, I would not do it unless I knew it is really going to gain serious performance improvement, and for this application, I am not sure if it matters that much.

It is smart idea, though. If you do it, please let me know how it turns out.

At the end, the standalone application will be modifying a few records every now and then, and since there is only one client, I do not believe you would resent performance that much.
[ April 10, 2008: Message edited by: Edwin Dalorzo ]

[Ciaran Cahill]

I think the idiom for locking is :

mylock.lock();
try {

...
}
catch {

} finally {
mylock.unlock();
}

This ensures that the lock is released on all paths out of the method

[Mary John]

thanks edwin and gaurav for replies

I think I didnt make myself very clear.
actually my question was how to synchronise the lockRecord method.
ie with synchronised(lockedRecords) keyword or
lock/unlock methods of Lock interface.


Now I realse that both are same.

thanks all.

[Mary John]

Thanks ciaran for pointing out that.
In my code, the lock.lock() was inside the try block. As you mentioned I should bring that out of try block. thanks

[Michael Fisherman]

I did not use synchronize, I just went with the lock, dbfunction(...,lockcookie), unlock(...,lockcookie) all surrounded by a
try, catch.

[Ulises Pulido]

Mary John wrote:Hi all


I have a Map which holds list of all locked records as
private static Map<Long, Long> lockedRecords = new HashMap<Long, Long>();

Should I synchronise my lock method with the object lock on "lockedRecords"
ie with synchronised(lockedRecords), so that other methods( which may come in future enhancements )will not access lockedRecords while my lock method checks whether a particular record is present in it or not.

Or, is the code below which I have done with lock.lock() and lock.unlock() method just fine?

 
public long lockRecord(long recNo)
throws RecordNotFoundException
{

long lockCookie = rand.nextLong();
try
{
lock.lock();
while(lockedRecords.containsKey(recNo))
{

try{
lockReleased.await();
}
catch(InterruptedException intr)
{
intr.printStackTrace();
}
}
 
lockedRecords.put(recNo, lockCookie);
}
finally
{
lock.unlock();
}
 
return lockCookie;


}
   

[ April 09, 2008: Message edited by: Mary John ]

Hello, what would you think about her approach because I followed the same one on my project

[Fola Fadairo]

There is a missed scenario in this implementation.

1. Thread A locks record 23.
2. Thread B attempts to lock record 23 and is placed in a waiting state.
3. Thread A deletes record 23.
4. Thread A releases the lock on record 23 and notifies Thread B.
5. Thread B locks record 23.


On step 5, a RecordNotFoundException should be thrown because record 23 was deleted while Thread B was waiting for the lock. The implementation does not perform this check. The check should be made again after the lock on a record has been obtained on line 21.

It is a subtle bug, but nonetheless a bug.

Regards.

P.S:
The validity of the record should also be checked before attempting to lock, i.e on entering the lock method.

[Ulises Pulido]

Fola Fadairo wrote:There is a missed scenario in this implementation.

1. Thread A locks record 23.
2. Thread B attempts to lock record 23 and is placed in a waiting state.
3. Thread A deletes record 23.
4. Thread A releases the lock on record 23 and notifies Thread B.
5. Thread B locks record 23.


On step 5, a RecordNotFoundException should be thrown because record 23 was deleted while Thread B was waiting for the lock. The implementation does not perform this check. The check should be made again after the lock on a record has been obtained on line 21.

It is a subtle bug, but nonetheless a bug.

Regards.

P.S:
The validity of the record should also be checked before attempting to lock, i.e on entering the lock method.

Thanks for the answer.

About the scenario that you mention what I do is to leave let the record locking for the row as you said, but I check if the row was deleted when I try to delete or update again launching a RecordNotFoundException not during locking.
I mean I perform that validation in the update and delete methods

[Alexandre Baldo]

Ulises Pulido wrote:
... what I do is to leave let the record locking for the row as you said, but I check if the row was deleted when I try to delete or update again launching a RecordNotFoundException not during locking.
I mean I perform that validation in the update and delete methods

I did this too... but I don't think it is correct because we could hold a lock of a register that no longer exists...
I tried to re-check if the record was still valid before locking but I got a deadlock.

I don't know what to do.....

[Ulises Pulido]

Alexandre Baldo wrote:

Ulises Pulido wrote:
... what I do is to leave let the record locking for the row as you said, but I check if the row was deleted when I try to delete or update again launching a RecordNotFoundException not during locking.
I mean I perform that validation in the update and delete methods

I did this too... but I don't think it is correct because we could hold a lock of a register that no longer exists...
I tried to re-check if the record was still valid before locking but I got a deadlock.

I don't know what to do.....

Well maybe unlocking inside a finally clause will solve the problem.

[Fola Fadairo]

Ulises Pulido wrote:...
About the scenario that you mention what I do is to leave let the record locking for the row as you said, but I check if the row was deleted when I try to delete or update again launching a RecordNotFoundException not during locking.
I mean I perform that validation in the update and delete methods

Your implementation should depend on your specs. If your specs contain:

Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file.

and your lock method signature is:

 // Locks a record so that it can only be updated or deleted by this client.
// Returned value is a cookie that must be used when the record is unlocked,
// updated, or deleted. If the specified record is already locked by a different
// client, the current thread gives up the CPU and consumes no CPU cycles until
// the record is unlocked.
public long lock(int recNo) throws RecordNotFoundException;
   

then your lock method should throw RecordNotFoundException if a Thread attempts to lock an invalid record of any kind, deleted or non-existing.

Regards.

[Ulises Pulido]

Hello Fola:

You know what, you are completely right.

Thanks for helping me on this =) I owe you something


Regards!

[Ulises Pulido]

That got me into another question, the unlock method throws a RecordNotFoundException also, but I think that we won't see if the row is deleted because we might have deleted the row and then we want to unlock it, the only validation would be if the row is out of bounds Am I Right ?

[Fola Fadairo]

According to the specs I have, the same condition for throwing RecordNotFoundException in the lock method holds for the unlock method and all other methods throwing RecordNotFoundException.

For example, you might be sure that it that the RecordNotFoundException will never propagate to the unlock method, but you don't know what automated tests will be run on your Data class to validate that it satisfies the specs.


P.S:
BTW: Your assertion is totally correct.

[Fola Fadairo]

There is actually a 'gotcha' in the unlock method. The scenario is for a method that has been deleted but not yet unlocked. I assume that you are using the sequence:

1. Lock record.
2. update or delete record.
3. Unlock record.

It is perfectly valid to unlock a record deleted with the a valid lock cookie. I did something like this:

 if (state == INVALID_RECORD_INDEX) {
		throw new RecordNotFoundException();
 } else if (recordState == DELETED_RECORD
		    && lockCookie == null) {
		// record is deleted and unlocked
		throw new RecordNotFoundException();
 } else if (lockCookie == cookie) {
		cookiesCollection.remove(recNo);
		recordLock.setCookie(null);
 } else {
		SecurityException e = new SecurityException(
			"Attempt to unlock record failed.");
		throw e;
 }

You just have to perform some condition checks to get around the 'gotcha'. Of course there are other ways to meet the specs, it is just a matter of taste.

I hope this helps.