RecordNotFoundException in locking

[rinke hoekstra]

Dear all,

According to my URLyBird 1.1.1 the lock/unlock methods throw a RecordNotFoundException. I came to the conclusion that this is really nonsense, and decided to ignore this.

I came to the following reasoning:

1) The normal documentend method sequence is lock - (verify) - modify - unlock. Unlock is at the end of this sequence. The existence of the record has been checked already at least once in this chain, and possibly even 2 or three times. So there is no reason to check it, and so no reason to throw it.

2) Not only is there no reason: checking if a record exists inside unlock is even a hassle. Not only must we provide access to the DataAccess class solely for this reason, but it creates also (often mentioned before) problems with the delete method having just deleted a record. Checking on this creates unnecessary, unlogical extra code.

3) An alternative might be to interpret the RecordNotFoundException in this case as "not found in the locking HashMap". Unelegant, to my opinion, because that would mean that RecordNotFoundException has different meanings in different situations.

So I chose to ignore the "throws RecordNotFoundException" clause in unlock. This saves me from a lot of trouble, and creates more clear and clean code. Analogous, I adapt the same logic to the lock method:

1) ignore the throws RecordNotFoundException clause
2) will NOT check if the record exists or not. This will be done anyway by the next method in line (being update or delete).

I realize this is not the most common design. So: am I overlooking something? Any comments? Is it "dangerous" to ignore a throws clause in the interface??
[ July 03, 2007: Message edited by: rinke hoekstra ]

[Herman Schelti]

hi Rinke,

I agree with you that the interface supplied by Sun is less than ideal, but the method unlock() is a public one, so it does not have to be in a sequence like 'lock - (verify) - modify - unlock'.

Sun just might run a program that calls unlock(40, cookie), and there might only be 39 records in their database.

My version will throw the RecordNotFoundException in that case.

Herman

[Gabriel Vargas]

Hi,

I agree with Herman. I throw RecordNotFoundException in lock and unlock because i think than lock or unlock a record than doesn't exist or is marked as deleted aren't a good thing. In performace application isn't down too much with these validations. I think code complexity isn't affected because i use read method than throws RecordNofFoundException to validate than record exist in these methods (only add a line to validate this ).

When record isn't found in locking map i throw a SecurityException as is indicated in my interface (B&S) but i don't know if you can throw this exception. It is throw in lock manager, in lock method of Data i call read method of database manager (throws RecordNotFoundException), then i call lock method of lock manager (throws SecurityException), so expected exceptions are throws by their respectives managers making my code more clear. I hope it heps you.
[ July 04, 2007: Message edited by: Juan Gabriel Vargas Bola�os ]

[rinke hoekstra]

Hi Herman,

Sun just might run a program that calls unlock(40, cookie), and there might only be 39 records in their database.

Well, that's simple. There are two cases possible:
1) record 40 existed before and just has been deleted. The unlock method then removes record 40 from the locking HashMap, and nothing happens. No exceptions raised.
2) Record 40 did not exist before, so it was not locked before too. In that case, SecurityException is raised, because it cannot be that the correct cookie values was passed too.

the method unlock() is a public one, so it does not have to be in a sequence like 'lock - (verify) - modify - unlock'.

Yes, but my javadoc code clearly states that this is the order which must be followed. Calling unlock outside this scope is a programming error. Programming errors should be covered by RuntimeExceptions. In case the unlock method is called with a wrong cookie value, that is a programming error, and then a SecurityException extends RuntimeException is raised. In case somebody calls unlock on a record which does not exist in the HashMap, that is a programming error too, so in this case I throw a LockException extends RuntimeException.

So in my app, LockExceptions is raised where others raise RecordNotFoundException, for two reasons:
1) A RuntimeException is more appropriate in this case, see above
2) The only thing the unlock method should care about is checking if the passed recordnumber is inside the locking HashMap. It should not be interested in the fact if the record exists. That's the task for the DataManager, not for the LockManager. RecordNotFoundException is not appropriate here, as this means that the record does not exist in the database - which is a condition to care about for the DataManager, not for the LockManager. LockException just means that the record was not locked - nothing more, nothing less.


For normal use of the program, the record may or may not exist anymore when calling unlock; for example when the previous method called was delete, then the record does not exist (any more). Should we raise an exception in case someone uses the program in a normal, intended way? I think that is silly. Exceptions are to be raised in cases when something happens which was not supposed to happen. A record not existing anymore when unlock is called is a perfectly legal state in the program - so nothing to raise an exception about.
Of course, I know some people here are resolving this problem by letting the delete method itself do the unlocking. But that is to my opionion "creating a trick to bypass a silly situation", where the better sollution would be to not let happen the silly situation. And, this propagates inconsistant behavior: for delete, we should not call unlock afterwards, but for update we should - this is inconsistant and not logical.
[ July 04, 2007: Message edited by: rinke hoekstra ]

[rinke hoekstra]

Hi Gabriel,

RecordNofFoundException to validate than record exist in these methods (only add a line to validate this ).

... in lock method of Data i call read method of database manager (throws RecordNotFoundException), then i call lock method of lock manager (throws SecurityException), so expected exceptions are throws by their respectives managers making my code more clear.

Yes, good point. I have been doubting on that sollution. But then, You need to check from the lock method if the record exists. This can be done in two ways:

1) provide access to DataManager from LockManager. A hassle, to my opinion, and not in correspondence with the idea to separate the functionality of the two classes.

OR

2) (your sollution):
a) From Data.lock: Call LockManager.lock to put the rec in the HashMap. Notice that this method might be caught in a waiting loop when the record was locked already.
b) After LockManager.lock returns, we should check if the rec exists. This is done by calling DataManager.read (from Data.lock)
c) If it appears not to exist, then we should remove the lock again by calling LockManager.unlock, and then raise the RecordNotFoundException.

I have to disagree with you that this is just one line of extra code. In the Data.lock method, code flow is jumping from LockManager.lock to DataManager.read and back to LockManager.unlock, which I think is quite unelegant for a class which is only supposed to be a Facade, suggesting that method implementations should be as empty as possible.

To my opinion: quite some hassle for a check which is again performed by the next method in line to be called (update or delete).

Even in the case that the same record has been deleted while the current thread was in wait condition trying to obtain the lock, still nothing goes wrong, as the update or delete methods themselves again perform the check if the record exits.

The problem comes back to this: will I be punished by Sun for not checking on RecordNotFoundException in lock/unlock?

On the other hand: my create method throws a DuplicateKeyException. I think most people ignore this exception because it does not make sense (just like I do). In fact this is the same case: I'm ignoring the throws RecordNotFoundException in lock/unlock because I think it does not make sense.
[ July 04, 2007: Message edited by: rinke hoekstra ]

[Gabriel Vargas]

Hi Rinke.

Yes, good point. I have been doubting on that sollution. But then, You need to check from the lock method if the record exists. This can be done in two ways:

1) provide access to DataManager from LockManager. A hassle, to my opinion, and not in correspondence with the idea to separate the functionality of the two classes.

OR

2) (your sollution):
a) From Data.lock: Call LockManager.lock to put the rec in the HashMap. Notice that this method might be caught in a waiting loop when the record was locked already.
b) After LockManager.lock returns, we should check if the rec exists. This is done by calling DataManager.read (from Data.lock)
c) If it appears not to exist, then we should remove the lock again by calling LockManager.unlock, and then raise the RecordNotFoundException.

I don't do at this way. There is another way. I do in follow way:

There are three methods on LockManager: lock, unlock and verifyRecord (synchronized over the lock hash map).

In DatabaseManager crud methods, syncronized over a helper than do all basic operations over datafile.

In Data.lock i first verify than record exists, then i try to lock the record with the lock manager. But to do this i will need to avoid a call for example to a delete between reading record (verify) and locking record. If i try to create another monitor in Data class i get a deadlock because monitor of lock manager is released but monitor of Data is still owned (there are two differents monitors, It seems a bit confuse but when i do the test i see that problem). My solution was use the lock object (Java 5 lock classes :roll: i love it) than i create in the lock manager also in data class, creating a four method in lock manager than returns that lock object, and then i use reentrant lock property present in java (there is only one monitor, the lock object of lock manager shared between lock manager and data) avoiding deadlock and keeping their responsabilites to each class.

I think than DuplicateKeyException is ommited by ranchers (I included) because is an scenario present only when entries are reused, but a RecordNofFoundException is a scenario always present when you try to lock or unlock a record, it's true than our javadoc cleary states lock-update-unlock sequence but not all times we read all javadoc of all classes, we read parts than we are interested so if DB interface tells than lock method throws a RecordNotFoundException and Data class tells than it is ommited, it could be difficut getting cause of an error because Data doesn't honor contract of DB interface.

I hope it help you.

[rinke hoekstra]

Hi Gabriel, maybe others.

It is clear to me what you mean. I might reconsider my approach on details...

However, to come back on my design: Let me phrase it different:

Lets just ignore the fact that this is an assignment. Do you think that, purely argued from a programming point of view it is bad to NOT check on existence of records in the database inside the lock method??

Besides the fact that it may cost points because not following precisely commented instructions, would it be harmfull for the functionality of the application??

[Gabriel Vargas]

Hi Rinke,

In a real world application i think there are a lot of improvements like you expose. When i read this i doubt on my own implementation, but later i remember collections classes, normally we rely on comments of interfaces like Collection, List, Set, etc. I don't need to see implementations comments because interfaces have generic comments (sometimes these comments send to implementations comments). I think than ommit RecordNotFoundException in lock and unlock method must be specified in DB interface and in Data class if you think than this a good design, you must be consistent and you must argument your design decisions. (I know there are a lot of ways to do the assigment).

Remember read carefully your specifications, i found this part:

Correctness

Your project must conform to this specification. Features that deviate from specification will not receive full credit. You will not receive extra credit points for work beyond the requirements of the specification.

but i read some threads than deviates from original specification (like DuplicateKeyException ) but have a very good justification in choices.txt and passed.

I hope it help you.

[Nina Binde]

Hi rinke,

I have been watching this thread for some interesting thoughts from you guys. Thought I did chip in too though might be a little too trivial for you.
The way I want to do is:
The LockManager class would definitely not need to throw RecordNotFoundException. That way, we preserve the sanity of the lock and unlock methods in that they do just what they are supposed to do like rinke suggested. So, this class can be reused elsewhere too. But, the data class would throw the exceptions mentioned in the interface since I strongly believe that the interface should drive the implementation and not viceversa. The create method, I think rightly throws the DuplicateKeyException. I do not see the point in not throwing it like you all have suggested. I am going to assume some combination of fields as primary key(in my case B&S, name and location) and do that way.

[Gabriel Vargas]

Hi Nina,

I agree with you for RecordNofFoundException, but in DuplicateKeyException you made doubt me. I search for various threads, and there are a lot of suggestions, one of this is as you propose, some combinations of fields, another is combination of all fields and another is as i do, not throw exception in implementation. I decide that because a primary key isn't clear at specifications, i interpret this like a suggestion, like a future enhancement. I repeat these words of Andrew:

"Just because a method signature declares that a particular exception might be thrown does not mean that you have to throw it. If you cannot find a unique key in the data you have been provided (and from memory you cannot get a unique key in URLyBird) then you may want to just consider it as something intended for future use."

This is my reason for ommit throw DuplicateKeyException, but your suggestion seems good and you must justify this in choices.txt. :roll:

I hope it help you. This help me!!!

[rinke hoekstra]

Hi Gabriel,

You are right, there is no key possible, at least not for urlybird. A record in the database corresponds to a room in a hotel. Why shouldn't a hotel be able to offer more than one room in the urlybird application? It is even very likely that each hotel has various rooms in the offering. So the combination of name/location can for sure not be the primary key. And I cannot think of any other possible combination of keys which could reasonably be a primary key.

[rinke hoekstra]

I found a flaw in my own design.

The question was: Would it be harmful NOT to test for the existence of the record when locking a record?

The answer is yes, there is a possibility that this is harmful.

Consider a thread calling lock on a non-existing record number in a situation where lock doesn't test on existence of records. lets say this is record 42.
Suppose the database has 41 records at the time this happens.

If then, after record 42 (which does not exist) is locked, the create method is called from another thread, and this method appends the new record at the end of the database. Then this new record will be... record 42, which is already locked by the first thread. So the new record will immediately be locked by the other thread, and the other thread has control over the new record. This doesn't seem like a wanted situation.

This is possible because the create method does not make use of the locking system, and can create records without locking them first (yes, Ken, that is no problem as long as I don't reuse deleted records, but only append new records).

So, only for this reason it IS necessary to check on existence of a record which just has been locked.
The other option is of course to demand that the create method also uses the locking system, but as lock is declared to throw RecordNotFoundException, I might better implement it like others have suggested .

So Data.lock looks like this:

public long lock(int recNo) throws RecordNotFoundException {
    long lockCookie = this.lockManager.lock(recNo);
    try {
        this.dataAccess.verifyRecord(recNo); //this is a wrapper around read
    } catch (RecordNotFoundException rnfe) {
        this.lockManager.unlock(recNo, lockCookie);
        throw rnfe;
    }
    return lockCookie;
}

I can't see any reason to have a synchronized block in this method. Isn't it?
[ July 07, 2007: Message edited by: rinke hoekstra ]

[Ken Boyd]

Originally posted by rinke hoekstra:
I found a flaw in my own design.

The question was: Would it be harmful NOT to test for the existence of the record when locking a record?

The answer is yes, there is a possibility that this is harmful.

Consider a thread calling lock on a non-existing record number in a situation where lock doesn't test on existence of records. lets say this is record 42.
Suppose the database has 41 records at the time this happens.

If then, after record 42 (which does not exist) is locked, the create method is called, and this method appends the new record at the end of the database. Then this new record will be... record 42, which is already locked by the other thread. So the new record will immediately be locked by the other thread, and the other thread has control over the new record. This doesn't seem like a wanted situation.

This is possible because the create method does not make use of the locking system, and can create records without locking them first (yes, Ken, that is no problem as long as I don't reuse deleted records, but only append new records).

Rinke,
You can avoid that situation by checking total records in database against pass record number in lock, unlock, update and delete method i.e. -1000 or 1000 record number is not valid if you do something like recordNumber > total_record_available in database.
You can keep create method not using locking and reusing deleted entry (again you are not following spec if you have that in create method)


Ken
[ July 07, 2007: Message edited by: Ken Boyd ]

[rinke hoekstra]

Originally posted by Ken Boyd:

You can avoid that situation by checking total records in database against pass record number in lock, unlock, update and delete method i.e. -1000 or 1000 record number is not valid if you do something like recordNumber > total_record_available in database.

Ken

Yes, that would reduce the code of Data.lock to something like this:

public long lock(int recNo) throws RecordNotFoundException {
    if (recNo <= this.dataManager.getMaxRecNo()) {
        return this.lockManager.lock(recNo);
    }
    throw new RecordNotFoundException("impossible record number");
}    

Basically, any more checking is not needed, because negative or deleted record numbers will be caught by the next method called.

However, this method should be synchronized.

By the way, do you have this line in your assignment:

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

If so, how do you deal with it, as you're not checking on deletions.
[ July 07, 2007: Message edited by: rinke hoekstra ]

[Ken Boyd]

Originally posted by rinke hoekstra:


If so, how do you deal with it, as you're not checking on deletions.

[ July 07, 2007: Message edited by: rinke hoekstra ]

I have the same line in the specification. My lock method won't throw RNFE but update and delete will throw them if locked record is deleted. Also note that it is not must and said exclusively that lock method must throw it. Again lock method is throwing RNFE but in particular scenario not all the time. Just method signature declares that a particular exception might be thrown does not mean that you have to throw it.

I am quite sure you implement RNFE in every method depending on conditions..more description goes to choice.txt

Only DuplicateException is not thrown by create method and description in choice.txt

Ken

[Gabriel Vargas]

Hi Rinke, Ken,

I manage all validations over my data class, with that code:

public long lock(int recNo) throws RecordNotFoundException {
    if (recNo <= this.dataManager.getMaxRecNo()) {
        return this.lockManager.lock(recNo);
    }
    throw new RecordNotFoundException("impossible record number");

I think you manage two similar validations (deleted record and no existent record) in two separate parts, i personally prefer manage this in read method . I think there are a central part where i look if there is a bug.

You are right, there is no key possible, at least not for urlybird. A record in the database corresponds to a room in a hotel. Why shouldn't a hotel be able to offer more than one room in the urlybird application? It is even very likely that each hotel has various rooms in the offering. So the combination of name/location can for sure not be the primary key. And I cannot think of any other possible combination of keys which could reasonably be a primary key.

In my low experience I learned than use a field of data (or combinations of then) can be probabily good, but later problems appear, so i would prefer use a field different of data fields like an id field. For example, normally locations have different names (I think you refer to Smallville, Gotham City, etc), but there are cases where locations have same name and when the same company are in these locations throubles begins... (it happened to me!!! not with the same case but happened to me). My "primary key" is the record position in the datafile begins with zero, this is for internal use only, never showed to the user but it simplifies all operations.

I hope it help.

[Ken Boyd]

may be last post regarding RNFE that I will lock deleted recorded but throw RNFE in case record number is not in database i.e. -1000.

I have to lock deleted entry to create new record at same location and to avoid two threads to do that at same time..

ken