• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Tim Cooke
  • paul wheaton
  • Jeanne Boyarsky
  • Ron McLeod
Sheriffs:
  • Paul Clapham
  • Liutauras Vilda
  • Devaka Cooray
Saloon Keepers:
  • Tim Holloway
  • Roland Mueller
Bartenders:

Locking strategy with singleton

 
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I just have finished my locking class, called LogicalRecordLock, and I want to know the list opinion.

I am working on the URLyBird 1.2.1.

Just talk a litle for the server design and architecture before:
The interface I need to implement in my assignment is the DBAccess interface.
I implemented this interface as a facade to two subsystems: database file manager (FilePersistence class) and locking manager (LogicalRecordLock class).
The FilePersistence and LogicalRecordLock classes both implement the singleton pattern.

I am not working in network yet, but I will use RMI. As my assignment have a token (cookie) to identify the client, I do not need to care about how to identify a client, my clients are identified by the tokens I generated.

Finally, I will write a "thin client", because I created an adapter class that offer more simple methods to the client, and encapsulates the lock -> operation -> unlock sequence. This class adapts the DBAccess to the interface I'll expose via RMI.

After that said, follow is the pseudo-code of my LogicalRecordLock class lockRecord and unlockRecord methods. All the explanation and issues I thought is written in the comments. It's important to say that I supplied all the information here, and I am not trying to test the list.

Please, I would appreciate any comments.



Thanks again,
Marcelo.
SCJP5, SCJD (in progress...), SCEA (not started yet...)
 
Ranch Hand
Posts: 332
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Does your implementation allow to lock record, that doesn't exist?
 
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
There are a few things I saw you may need to re-think about:

1. what do you do for deleted record or invalid record in lock();

2. why not exit and throw RecordNotFoundException if InterruptedException thrown in lock?

3. in unlock this check is redundant:

if (!locksInUse.containsValue(cookie))

as you did another check later:

if (locksInUse.get(recNo) != cookie)

In another word, even if map contains cookie, it is
not sufficient, the last if check will take care of everything.

4. what about deleted record and invalid record in unlock?

Ziji
 
Ranch Hand
Posts: 152
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi,

You are basically swallowing the interruptedException. I wouldn't do that, as swallowing exceptions is considered bad programming practivce. Better rethrow it as IllegalThreadStateException.
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi John,
Yes, it allow to lock a record that does not exist. But I Thought about this case, and I decided to keep the locking implementation separated from the persistence implementation.
This is for a lot of reasons. The first and the most important, is that, anyway, after lock a record that does not exist, the client need to call another method, like update or delete, passing the locking token. And, in this moment, it will receive an RecordNotFoundException. So, why mix the locking implementation with the persistence, if anyway the client will receive the exception? And, I think it makes more sense, receive an RecordNotFoundException when trying to update or delete a record, than when trying to get a lock.
A second reason is that keeping them separated, we can reuse both implementations in other conexts where not both be necessary.
A third reason is simplicity: both implementation is more clear and easy to understand.

That's why I kept them separated.

What do you think?

Best regards,
Marcelo.
SCJP5(93%), SCJD(in progress...)
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi Ziji,

I will answer your questions:

1. what do you do for deleted record or invalid record in lock();

My implementation does not allow that.
I gave a partial answer in the last post to John, but, basically, I decided to keep the locking implementation separated from the persistence implementation (see the reasons in the last post). But anyway, when the client call another method like update or delete, in a valid or invalid record, it will receive a RecordNotFoundException.
Another thing I do not wrote in the last post is that the client does not see the DBAccess interface: the client see a business interface that exposes methods that perform the lock->operation->unlock at a shot, so, the client cannot try to do this things. But, anyway, it cant do something wrong with the data, if it tries to do some hacking implementation aginst the DBAccess interface anyway.

2. why not exit and throw RecordNotFoundException if InterruptedException thrown in lock?

I do not understand why would I want to do that... The operation system can cause, for any reason, an interruption in my sleeping thread. So, after that, I just re-check the while expression and go to sleep again if my moment to awake has not arrived (my record was not released)... I do not know another reason to receive an InterruptedException, but anyway, wouldnt I need to verify the while-expression and sleep again if my record was not released?

3. in unlock this check is redundant:

if (!locksInUse.containsValue(cookie))

as you did another check later:

if (locksInUse.get(recNo) != cookie)

In another word, even if map contains cookie, it is
not sufficient, the last if check will take care of everything.


You are right ! I just removed the first check from my code, as you suggested. Thak you !!!

4. what about deleted record and invalid record in unlock?

This situation can only be some hacking code. Because, before the client call the unlock method, he would have to call the lock, and an update or delete method. Of course, if the client has just deleted the record, it does not exist in database, but in unlock I dont check this and I dont need to do that. And, if it is an invalid record, it will be stopped in the update or delete methods, before the unlock. But, if this occurs, as this situation is an invalid situation (or a hacking code), I just need to garantee that nothing goes wrong with the data. And, in these situations, my code keep the database intact. What do you think?

Thak for your post,
Marcelo.
SCJP5(93%), SCJD(in progress...)
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi Rinke,

Hi,

You are basically swallowing the interruptedException. I wouldn't do that, as swallowing exceptions is considered bad programming practivce. Better rethrow it as IllegalThreadStateException.


I am really confused about the InterruptedException. Ziji talked about this exception too, and in my last post I tried to explain my reasons. I think this exception is thrown by OS for its reasons (reasons we are not aware of), or for another thread that explicitilly calls that against a sleeping thread. And, in these situations, the thread that just waked-up don't need to re-check the while-expression and sleep again if the record its waiting is not available yet?

Another information is that I am using swallowing in many situations, all of that with an good explanation like that, in my code. Can't I use swallowing, even knowing what I am doing?

Please, Rinke and Ziji, help me with that...

Thank you,
Marcelo.
SCJP5(93%), SCJD(in progress...)
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Marcelo,

One thing you should be aware of is that lock()/unlock() may be tested
by Sun independently, so if you don't validate record within those methods,
the data class implemention may be viewed as incomplete. I agree that client should always call lock()... some operation... unlock(). Just be conservative, not taking any risk. I have read a lot of posts here and finally made this decision.

So in the lock you should have:
1. let lockManager do the locking operation (call mgr.lock() )
2. check isValidRecord(recNo)? (call db.isValid(recNo) )
if not true, then you need invoke lockManager's unlock(), and also throw RNFE.


in the unlock method:

1. check isValidRecord(recNo)?
if not true, then you throw RNFE.
2. if it is true, invoke lockManager's unlock();


you also need to implement
isLocked(int recNo).

Basically, my locking stratigy follows those in Andrew's book,
using ReentrantLock instead of synchronized method or block.

For the interruptedException, Both books (max, and andrew's) don't handle,
rather, the method declares that is throws InterruptedException.
as it is a checked exception, and our instruction didn't allow us to throw such an exception,
so there are a couple of options there, I read some posts here:
option1: throw a RecordNotFoundException
option2: throw a RuntimeException as you mentioned.

Hope that will help.

Ziji
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Marcelo,

One thing you should be aware of is that lock()/unlock() may be tested
by Sun independently, so if you don't validate record within those methods,
the data class implemention may be viewed as incomplete. I agree that client should always call lock()... some operation... unlock(). Just be conservative, not taking any risk. I have read a lot of posts here and finally made this decision.

So in the lock you should have:
1. let lockManager do the locking operation (call mgr.lock() )
2. check isValidRecord(recNo)? (call db.isValid(recNo) )
if not true, then you need invoke lockManager's unlock(), and also throw RNFE.


in the unlock method:

1. check isValidRecord(recNo)?
if not true, then you throw RNFE.
2. if it is true, invoke lockManager's unlock();


you also need to implement
isLocked(int recNo).

Basically, my locking stratigy follows those in Andrew's book,
using ReentrantLock instead of synchronized method or block.

For the interruptedException, Both books (max, and andrew's) don't handle,
rather, the method declares that is throws InterruptedException.
as it is a checked exception, and our instruction didn't allow us to throw such an exception,
so there are a couple of options there, I read some posts here:
option1: throw a RecordNotFoundException
option2: throw a RuntimeException as you mentioned.

Hope that will help.

Ziji
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi Ziji,

I think you are right again. Be conservative will not take off the beauty of the solution.

I think I will make this change too, but I have already 2 more questions:

1. During this sequence: lock->delete record->unlock, if I check the db.isValidRecord() inside the call to locking.unlock(), as you mention, the record will not exist anymore, and an RNFE will be thrown. But its not true, because I need to unlock the record... How did you handle that?

2.About InterruptedException. I did not understand it yet. My implementation is swallowing this exception and checking again the while-expression, and if it was not reached, sleep again. Is this wrong? I need to interrupt the process and return some exception to the client?

Thank you again,
Marcelo.
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
To complement the case 1 above: my DBAccess interface have this signatures for the lock and unlock methods:



Can't I just check the db.isValidRecord() in the lockRecord method, and throw the RNFE it return false?
And, in the unlock method, I just ignore the RNFE, as the signature says...
After all, in the unlock, the client need to have the record lock and the cookie for this lock, and both will be checked anyway... If the client call unlock in a record that does not have the lock (existing or not), it will receive an SecurityException. If the client calls lock in a record that does not exist in the database, it will receive the same SecurityException again, because the record will not be in the locking array list in lock manager AND the cookie will be wrong anyway, even it tryes to create one. So, this last case (call unlock with a invalid record or a record that does not exist) is already covered.

So, I think that we can check if the record exists only in the lockRecord method.

What do you think?

Marcelo.
 
rinke hoekstra
Ranch Hand
Posts: 152
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Originally posted by Marcelo Camargo:
I think this exception is thrown by OS for its reasons (reasons we are not aware of), or for another thread that explicitilly calls that against a sleeping thread. And, in these situations, the thread that just waked-up don't need to re-check the while-expression and sleep again if the record its waiting is not available yet?

Another information is that I am using swallowing in many situations, all of that with an good explanation like that, in my code. Can't I use swallowing, even knowing what I am doing?
[/QB]



Hi Marcelo,

First, on the InterruptedException: I think this exception particularly happens if some other thread deliberately tries to terminate the tread or even the application. To my idea, it is a signal that it should stop what it is doing - so that it, it should stop trying to get the lock. Of course, you may decide that this thread is so important that nobody should be able to interrupt it, but to my feeling this is not the case. The client could always try again to acclaim the lock.

But whatever you decide, the client which was calling this method is the one who should know what to do. The method itself is not aware of the reason of the interruption. Therefore, I think it is better to throw the exception, and let the calling client decide what to do in this situation. It could for example notify the user, or it could retry calling the method.

Then, about swallowing the exceptions in general:

What I call swallowing is this: putting an exception in a catch block, and then doing nothing at all (or just logging). Then, to the calling method, it is like the exception did not happen at all. This is bad, because exceptions have an important role in informing what goes wrong.

Of course, you can catch an exception, and then solve the problem in that catch block, or work around it. This is of course perfect, depending on the circumstances. But swallowing it, in the sense of just catching it but then doing nothing with it, is (almost?) always bad and will certainly cost you 1 or 2 points.
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thank you Rinke,

Your opinions were very good. You are right about the InterruptedException, I will interrupt the lock waiting. And about the swallowing, that's exactly you sad, I am just ignoring them. But, I will reviw all the situations where I did that, to re-write them in other manner.

Thanks,
Marcelo.
 
Greenhorn
Posts: 4
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

while (locksInUse.containsKey(recNo)) {
try {
this.wait();
} catch (InterruptedException ex) {
/* do nothing, check the while again */
}
}

I took the same approach dealing with InterruptedException. I believe there is no reason to leave the while loop when this exception is raised. Although I don't know why this checked exception is raised, I don't think it is appropriate to rethrow it as a RNF exception (they have different meanings) and I strongly believe runtime exceptions shouldn't be used with RMI.
Even if it is raised at all, this exception alone does not mean you should stop waiting for lock.


lock->delete record->unlock sequence

This is one issue I had to spend two cups of coffee to think about. I did not keep locking and persistance classes separated, therefore one could not lock or unlock a deleted record. Since deleting a record requires "owning" its lock, I decided unlocking is not required after deleting one record, for simplicity reasons.


this.notifyAll();


Why notifyAll? Why not notify? Only one thread will be able to acquire the lock thereafter, so waking them all up is unneeded (unless there is an issue about notify and notifyAll I'm unaware of)

And finally, I personally don't think it is wise to allow locking nonexistent records. Locking noncreated records can be problem, but, as you have cleverly stated, it could only be exploited through "hacking".

Other than that, congratulations for seeking beauty in coding solutions, along with reasoning.
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
OK,


1. During this sequence: lock->delete record->unlock, if I check the db.isValidRecord() inside the call to locking.unlock(), as you mention, the record will not exist anymore, and an RNFE will be thrown. But its not true, because I need to unlock the record... How did you handle that?

2.About InterruptedException. I did not understand it yet. My implementation is swallowing this exception and checking again the while-expression, and if it was not reached, sleep again. Is this wrong? I need to interrupt the process and return some exception to the client?


I have read many posts here, Here is the approach:

1. if you delete a record, then you shouldn't call unlock() method in data class, instead, you should release the deleted recNo your self,
but this approach broke the standard sequence of : lock... some operation ... unlock.
2. the second approach is in lock and unlock, you don't care if recNo is
deleted or not, so lock and unlock can also operate on deleted record,
only when recNo is not lock or not in range, you throw RNFE.

About interruptedException, I just throw RNFE,
May be you and others are right, probably, we should wrap it up with a
RuntimeException, you cannot just throw InterruptedException, as the Sun's
instruction doesn't declare this Exception being thrown, you will lose points if you throw it.

Ziji
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Joaquim,


lock->delete record->unlock sequence

This is one issue I had to spend two cups of coffee to think about. I did not keep locking and persistance classes separated, therefore one could not lock or unlock a deleted record. Since deleting a record requires "owning" its lock, I decided unlocking is not required after deleting one record, for simplicity reasons.




I have been reading posts about this issue for a while,
If you don't release the deleted recNo, you will have trouble later,
For example, If you want to re-use that recNo for creating new record,
then that recNo will go back to normal, and you can never lock that record by other thread again, as it is locked by a thread during deleting.


Ziji
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Joaquim,


lock->delete record->unlock sequence

This is one issue I had to spend two cups of coffee to think about. I did not keep locking and persistance classes separated, therefore one could not lock or unlock a deleted record. Since deleting a record requires "owning" its lock, I decided unlocking is not required after deleting one record, for simplicity reasons.




I have been reading posts about this issue for a while,
If you don't release the deleted recNo, you will have trouble later,
For example, If you want to re-use that recNo for creating new record,
then that recNo will go back to normal, and you can never lock that record by other thread again, as it is locked by a thread during deleting.


Ziji
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thank you Joaquim, for the "clever" word you have used about me.
And thank you Ziji too.

But, after I read the posts yesterday, and before read again today, I changed my code in a way I think it became better.

I will explain here what I made:

1. During this sequence: lock->delete record->unlock, if I check the db.isValidRecord() inside the call to locking.unlock(), as you mention, the record will not exist anymore, and an RNFE will be thrown. But its not true, because I need to unlock the record... How did you handle that?


About this issue there are some important decisions I made:
a) In my DBAcess lockRecord lock, I decide to check if the record exist in database. My DBAcess lockRecord signature returns RNFE. So, I thought: if I check this, I will respect the SUN signature! (so changing, makes my code more compliant with what SUN expect, and everybody be happy). So, In my lockRecord, I am first locking the record, then after I check if the record exists. I am doing this in the simplest mode I can, calling an pre-existing method (readRecord). If the record does not exist, readRecord returns RNFE. This solves this first issue:



b) LockAttemptFailedException: Note that I return a new LockAttemptFailedException. If I receive an InterruptException, I am stoping the waiting (I am not just swallowing the exception as I was doing before). This is because an InterruptExceptioin is trown for example by SO when it is going to shutdown, or when the thread needs to stop its process. I can't just ignore it, so if I receive it, I stop waiting and wrap the exception in an LockAttemptFailedException.

c)about the unlock method, I am not checking if the record exists in database. I am doing this just in the lockRecord method. I dont need to do that because if the client call an unlock in a record that does not exist, it will receive an SecurityException, because the cookie it has is 100% invalid (if it has a valid cookie, than the record exists, because the only way to have a valid cookie is calling lockRecord method before). So, why check if the record exists, if this code will never be reached? After this thougt, I looked at my unlock signature, and, for my surprise, it dont have the RNFE... So, again, I think this decision is the best I can made, and it is compliant with the SUN unlock signature. And, after all, this way I dont have the colateral effect of lock->delete->unlock: I will never try to check if a deleted record exists in de last unlock method, and I dont need to do that anyway.
The code for unlock() follow:



d) Joaquim asked a question about the notifyAll(): My lockmanager is a singleton. For all clients, from any records, I make them wait on this single instance of record manager. So, for example, I have 10 clients, waiting for 2 records: 2 clients waiting for record 1, and 2 clients waiting for record 2. So, imagine if a client releases the lock of the record 1, and I call just a notify(). If I do that, just one thread will wake-up. And, imagine if this thread is one who was waiting for the record 2 lock. This situation could never occur, because the thread that just wake-up will sleep again (because it is waiting for the record 2), and none thread will get the lock. Thats because I need to call the notifyAll().

Afterall, I think this is a good solutions, and I thought about it after discussing in this forum.

What do you think?

Thank you,
Marcelo.
 
rinke hoekstra
Ranch Hand
Posts: 152
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi Joaquim,

Originally posted by Joaquim Vasconcellos:

I took the same approach dealing with InterruptedException. I believe there is no reason to leave the while loop when this exception is raised. Although I don't know why this checked exception is raised, I don't think it is appropriate to rethrow it as a RNF exception (they have different meanings) and I strongly believe runtime exceptions shouldn't be used with RMI.



Well, I agree with you that RNF Exception is inappropriate in this case. But a RuntimeException here (for example IllegalThreadStateException) doesn't mean that you are going to use it over RMI. It can be caught again by a business layer, who might again change it into a checked exception (or do anything else with it).

Originally posted by Joaquim Vasconcellos:


Why notifyAll? Why not notify? Only one thread will be able to acquire the lock thereafter, so waking them all up is unneeded (unless there is an issue about notify and notifyAll I'm unaware of)



I first used notifyAll and then realized the same thing. I changed it to notify, and guess what: the code was much faster in the execution of a heavy multithreaded test, and still worked fine. Even though the LockManager class was a singleton. So apparantly the fear Marcelo is expressing just doesn't happen - or I must have been extremely lucky with testing...
[ July 24, 2007: Message edited by: rinke hoekstra ]
 
rinke hoekstra
Ranch Hand
Posts: 152
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

During this sequence: lock->delete record->unlock, if I check the db.isValidRecord() inside the call to locking.unlock(), as you mention, the record will not exist anymore, and an RNFE will be thrown. But its not true, because I need to unlock the record... How did you handle that?



As I argued in this thread, the raising of an RNFE in especially unlock is really really nonsense. So I choose to ingore it, and not raise this exception. Makes the code much more simple.

In short, the reason why it makes no sense:
1) as you all have noticed, if a record is deleted, it is not found. You raise exceptions when something goes wrong, not when something is the condition you want. unlocking a deleted record in the delete method and not calling unlock in this case is not a good sollution to my opinion, as it breaks the general rule of the lock > modify > unlock sequence.
2) In this lock > modify > unlock sequence, the record has been tested on existence in every step of this chain. Why check again what you already checked two times earlier?
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Marcelo,

I think you pretty much figured all out.

There are a couple of things I saw in your unlock method:


Don't you compare cookie/owner of the lock with the current thread?

within your try block, shouldn't you have a if clause:

if (record is locked){
unlock record;
}


Ziji
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Marcelo,

I think you pretty much figured all out.

There are a couple of things I saw in your unlock method:


Don't you compare cookie/owner of the lock with the current thread?

within your try block, shouldn't you have a if clause:

if (record is locked){
unlock record;
}


Ziji
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Marcelo,

I think you pretty much figured all out.

There are a couple of things I saw in your unlock method:


Don't you compare cookie/owner of the lock with the current thread?

within your try block, shouldn't you have a if clause:

if (record is locked){
unlock record;
}


Ziji
 
Ranch Hand
Posts: 145
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi all,

This post is very long. At last i finished to read it.

My two or maybe three cents, I know than swallow a InterruptedException is bad (for some people is an antipattern). I found some time ago this link explains why swallow this exception is not a good practice:

Dealing with InterruptedException

Generally isn't good swallow an exception if we didn't know why we're swallowing it.

In my assigment i verify each time if record exists for lock, update, delete and unlock methods first because my method signature told that and second because i think each method must honor interface tolds. It is true than for efficiency reasons this check can be donde only one time, but for consistency i do when is required. Additionally i release lock in delete method because unlock checks if the record exists, and the most important reason for me is the object locked is the record and not makes sense to unlock a thing than doesn't exists. (I see a deja vu about this point )



DBAccess interface signature:
public long lockRecord(long recNo) throws RecordNotFoundException;

My implementation of this interface:

public long lockRecord(long recNo) throws IllegalArgumentException,
LockAttemptFailedException, RecordNotFoundException,
DatabaseFailureException {

long token = 0;
try {
token = logicalRecordLock.lockRecord(recNo);
} catch (IllegalArgumentException iae) {
/* re-throw the exception */
throw iae;
} catch (LockAttemptFailedException lafe) {

/* re-throw the exception */
throw lafe;
}

try {
filePersistence.readRecord(recNo);
} catch (IllegalArgumentException iae) {
/* unlock and re-throw the exception (not probable to be thrown) */
logicalRecordLock.unlock(recNo, token);
throw iae;
} catch (RecordNotFoundException rnfe) {
/* unlock and re-throw the exception */
logicalRecordLock.unlock(recNo, token);
throw rnfe;
} catch (DatabaseFailureException dbfe) {

/* unlock record and re-throw the exception */
logicalRecordLock.unlock(recNo, token);
throw dbfe;
}
/* The record is valid, then return the lock token */
return token;
}



Marcelo, in your lock method you catch some exceptions and do nothing, i don't find any reason to do that. for the second try i would prefer use semantic defined for Lock class, i means, if finally you must unlock record i think is better to use a finally block and don't catch any exception because again you do nothing. I think it can look like this:



No more writing, it would be eternal . I hope it helps you.
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I agree with Gabriel,

and the most important reason for me is the object locked is the record and not makes sense to unlock a thing than doesn't exists. (I see a deja vu about this point )




I thing to clarify, Should we check isRecordLock() in the delete
method body of the Data class?

I am doing a check before deleting the record from database file.
then invoke lockManager's unlock method (no Data class's unlock )

Ziji
 
Ziji (Jay) Zhang
Ranch Hand
Posts: 42
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I agree with Gabriel,

and the most important reason for me is the object locked is the record and not makes sense to unlock a thing than doesn't exists. (I see a deja vu about this point )




I thing to clarify, Should we check isRecordLock() in the delete
method body of the Data class?

I am doing a check before deleting the record from database file.
then invoke lockManager's unlock method (no Data class's unlock )

Ziji
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Rinke,

I first used notifyAll and then realized the same thing. I changed it to notify, and guess what: the code was much faster in the execution of a heavy multithreaded test, and still worked fine. Even though the LockManager class was a singleton. So apparantly the fear Marcelo is expressing just doesn't happen - or I must have been extremely lucky with testing...



The notify() wake-up just one sleeping thread. So, if the worng one wake-up, it will back to sleep and nothing more occurs... this can cause a big problem. NotifyAll() wake-up all sleeping threads, so, if there is someone waiting, one will wake-up.

Marcelo.
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thank you Ziji,


Don't you compare cookie/owner of the lock with the current thread?

within your try block, shouldn't you have a if clause:

if (record is locked){
unlock record;
}



The method logicalRecordLock.logicallyLocked(recNo, cookie) take care of that. It verifies if the record is locked and the owner.

Marcelo.
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi Gabriel,


Marcelo, in your lock method you catch some exceptions and do nothing, i don't find any reason to do that. for the second try i would prefer use semantic defined for Lock class, i means, if finally you must unlock record i think is better to use a finally block and don't catch any exception because again you do nothing. I think it can look like this:



I agree with you that I can remove a lot of catch blocks that just re-throw the exception. I just removed that. but, in the particular case you detail, I need to call unlock() ONLY if an exception is threw inside the method readRecord(). Otherwise, that is the normal situation, the record keeps locked, because this is the method goal.

So, I don't know how to write it another way, do you know other way? I would like to simplify it... Follow is the code again, to make easy your look:



Marcelo.
 
Marcelo Camargo
Ranch Hand
Posts: 43
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Ziji,


I thing to clarify, Should we check isRecordLock() in the delete
method body of the Data class?

I am doing a check before deleting the record from database file.
then invoke lockManager's unlock method (no Data class's unlock )



You must verify if the client owns the lock on this record before delete it. So, this is the same to say that you need to call the isRecordLock() inside the delete method of the data class (that implements the DBAccess interface).

Marcelo.
 
Gabriel Vargas
Ranch Hand
Posts: 145
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi Marcelo,

I remember now why i don't have this constructions in my lock method, it is because for me lock is not responsible to unlock whe an exception occurs, this constrcution i have in delete method and in another methods using logical locking (i have a 3-tier architecture) so if an exception occurs inside lock method is informed to callers to handle this exceptions and finally unlocking record.
 
Ranch Hand
Posts: 40
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi !

I have the B&S 2.1.3. assigment that uses locking with cookies.

I liked Marcelo's solution because it was easy to understand and simple. This aproach was also what i had in mind before reading the Denny's DVDs example.
I was implementing a solution similar to yours but i found a good reason not to do so.

The specifications of my lock method:
Locks a record so that it can only be updated or deleted by this client.
...
If the specified record is already locked by a different
client
... waits until the record is unlocked.

the expresions "by this client" instead of "the client who owns the cookie" , or "by a different client" instead of "with a different cookie" is confusing for me.
It seems that they whant a solution similar to Denny's DVDs in witch the "client" is passed by parameter by the DvdDatabase class.



If that's what they expect us to implement i don't understand the sense of using a cookie.

If the client must indentify itself for locking , what's the sense of obteinning a random generated coockie if we also need to store the identity of the client that locked the record?

That's because i think it's also posible to understand and replace "the client who owns the cookie"
and "with a different cookie" instead "the client" and "different client" in the specifications.

would this interpretation end in a Automatic Failure ?

does anyone know someone passed this way?

Do you have similar specifications for locking ?

thanks
 
Xabier Martija
Ranch Hand
Posts: 40
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I've started a new topic for my last question in this topic.
 
No one can make you feel inferior without your consent - Eleanor Roosevelt. tiny ad:
Smokeless wood heat with a rocket mass heater
https://woodheat.net
reply
    Bookmark Topic Watch Topic
  • New Topic