NX: Unlock before throw Exception

[Terry Martinson]

In my database manipulation methods, I'm adding a call to my unlock method before throwing an exception (thanks to a suggestion made on this forum). I'm debating whether to just add it to the exception catch, or to make a finally clause.
If I add it to the finally, the delete/update methods would ALWAYS be responsible for unlocking the record, rather than expecting whoever originally got the lock to unlock it. This would definitely ensure that the record gets unlocked but it seems like the delete/update might be doing too much here.
If I put it in the exception catch, the database manipulation method would only unlock it in the case of an error, which is needed because the callers unlock functionality would probably not get executed because of the thrown error. In this case, I am expecting that the normal way of doing things would be that someone would 1) get the lock on the record, 2) call the update/delete/whatever, and 3) unlock the record. I would only be doing the special unlock in an error situation.
Thoughts???
TJ

[Philippe Maquet]

Hi Terry,
What about this ?

try {
    lock();
    update(); // or delete()
} finally {
    unlock();
}

Best,
Phil.

[Terry Martinson]

Hi Phil. Thanks. That is basically what I am doing in my book method.
I'm thinking about the situation where the Sun auto-testing or some other program that might use my API is doing the calling to my Data manipulation methods. I wouldn't want any record locks left out there in an error situation, and I don't think I can really depend on them to remove the locks.
Do you handle this type of situation in your assignment, or do you depend on the caller of the delete/update method to do the unlocking in all situations?
TJ

[Philippe Maquet]

Hi Terry,

I'm thinking about the situation where the Sun auto-testing or some other program that might use my API is doing the calling to my Data manipulation methods. I wouldn't want any record locks left out there in an error situation, and I don't think I can really depend on them to remove the locks.

As the Data class writer, you're not responsible for badly written clients of your class, including some possible auto-testing software from SUN .
It's all about responsabilities : update() and delete() are *not* responsible for getting a lock, right ? Just for checking that the client has been granted the proper lock before calling them. So why should they be responsible for unlocking the record in the case their clients fail to do it ?
Regards,
Phil.

[Terry Martinson]

Good point. Thanks Phil. (I have auto-failure paranoia )
TJ

[Terry Martinson]

One last thing on this that has me thinking:
In this posting from someone who passed with very high score, they stress the importance of releasing a lock before calling exception. Exact words were: "It is vital that any exception that occurs during processing is caught and rethrown after the Contractor has been unlocked."
My current plan for the update/delete methods is:
1. Follow Phil's suggestion and not do unlocking in the normal situation. So, if everything goes fine, the update and delete methods are not responsible for the unlock and I expect that clients use the locking API I provide.
2. If exception is encountered within Data manipulation code, follow Ken's suggestion in the thread referenced above to unlock the record just to be safe since some big bad error must have occurred.
3. I do NOT plan to unlock in the case of a SecurityException, since security exceptions mean that someone else must have a lock on a particular record. In this case I would not want to allow someone coming in with a bad cookie to remove a lock on a record locked by someone else.
TJ

[Philippe Maquet]

Hi Terry,
I agree with everything you wrote above, though I think that the try..finally construct may give the same results/benefits, with less code to write.
If you compare this :

try {
    data.lock(recNo);
    try {
        data.someMethod();
        data.unlock(recNo);
    } catch (SomeException e) { // needs some special processing
        data.specialProcess();
        data.unlock(recNo):
        throw e;
    } catch (RuntimeException e) { // any other exception must be catched just for unlocking
        data.unlock(recNo);
        throw e;
    }
}

to this :

try {
    data.lock(recNo);
    try {
        data.someMethod();
    } catch (SomeException e) { // needs some special processing
        data.specialProcess();
        throw e;
    }
} finally {
    data.unlock(recNo);
}

it's hard to hesitate between both ways of coding. Both code snippets give the same behaviour, and the second one is just simpler to code and later read.
Now it's right that SecurityException may be a special case, especially because unlock() may throw it. But if you add some boolean isLocked() method (or even a more specific boolean canUnlock() method), it's easy to put the data.unlock(recNo) lines into if (canUnlock(recNo)) blocks. But this is true with both coding styles, in such a way that if you use canUnlock() to avoid a possible issue with unlock(), not only is the try..finally solution still simpler, but even more :
Solution 1 becomes very hard to read :

try {
    data.lock(recNo);
    try {
        data.someMethod();
        if (data.canUnlock(recNo)) {
            data.unlock(recNo);
        }
    } catch (SomeException e) { // needs some special processing
        data.specialProcess();
        if (data.canUnlock(recNo)) {
            data.unlock(recNo);
        }
        throw e;
    } catch (RuntimeException e) { // any other exception must be catched just for unlocking
        if (data.canUnlock(recNo)) {
            data.unlock(recNo);
        }
        throw e;
    }
}

while solution 2 is hardly more complex :

try {
    data.lock(recNo);
    try {
        data.someMethod();
    } catch (SomeException e) { // needs some special processing
        data.specialProcess();
        throw e;
    }
} finally {
    if (data.canUnlock(recNo)) {
        data.unlock(recNo);
    }
}

Regards,
Phil.

[Terry Martinson]

Thanks Phil. I do the finally approach (similar to what you posted above) in my book business method, which calls the lock-process-unlock functionality.
Regarding my quote from above:

2. If exception is encountered within Data manipulation code, follow Ken's suggestion in the thread referenced above to unlock the record just to be safe since some big bad error must have occurred.

This is referring to the actual Data.update or Data.delete method, which expects me to already have the lock cookie. So, I'm not calling the lock method from within update or delete. So, my delete method looks something like this pseudo-code:

1. verifyLockCookie (throws SecurityException if needed, no unlocking done)
2. check room to see if deleted or not found - if so, unlock and throw RecordNotFoundException
3. do the IO stuff to delete the record and if IOException, unlock and throw DatabaseError

The finally works great for my business method that invokes locking, but I don't see where it would be needed in the Data.update or delete.
Does this make sense, or am I still missing something?
TJ

[Philippe Maquet]

Hi Terry,
I think you're still mixing methods/classes responsabilities, which makes your design harder to explain and understand.
Compare both designs :

A possible one : Update/Delete are *not* responsible for unlocking. Period.

Yours : Update/Delete are *not* responsible for unlocking, *except* if an exception is thrown, *except* if this possible exception is SecurityException.

Are you still hesitating ?
Best regards,
Phil.
PS: I love such logical discussions, so thank you for this one.

[Terry Martinson]

Phil - I love your design 1, which is how I originally coded my update/delete. My hesitation with this is still that statement from Ken in the posting listed above that deals with it being vital to release a lock in the case of an error during processing.
If I encounter an error during either my Data.update or Data.delete and I do not unlock anything, I'm totally disregarding what Ken says is "vital".
I'm a bit stuck here, because I agree with you that it is not the update/delete method's responsibility to lock/unlock - any user of my API should use my locking API correctly. I know that MY business book method does it correctly, but I can't guarantee others (like Sun's auto-test) will use it correctly.
What do you think Ken was getting at here? I just don't want to overlook some vital thing that could cause auto-failure (my worst fear right now )
TJ

[Bill Robertson]

Phil's comments:

A possible one : Update/Delete are *not* responsible for unlocking.
Period.

I disagree. In update if you run across RecordNotFoundException you
do not want that record kept in your Map or whatever collection you
use for locking. So if you do not call unlock how would you do this?!?
This is just one of many examples.

[Ken Krebs]

In my ServicesImpl book method, which can throw RecordNotFoundException or IOException, I lock the Contractor record and then invoke my Data update method in a try block to perform the booking. If an exception is thrown during the Data update invocation, I catch it and then rethrow it after unlocking the Contractor in the finally block.

My point about being "vital" refers to the client's design-by-contract responsibility to make sure that the unlock part of the sequence is performed. I totally agree that since the client (my ServicesImpl class, not the gui or rmi proxy) does the locking, that same client should do the unlocking whether an exception is thrown or not or else the record can become permanently locked, requiring a restart of the server.

[Bill Robertson]

I am now in full agreement with Ken and Phil. My only question is lets say
you get a RecordNotFoundException. How can you then call unlock when
unlock also throws RecordNotFoundException. Unlock is just going to turn
around and throw it.

[Terry Martinson]

Ken -
Thank you for clarifying your comments from the famous "389 posting" .
You rock!
My book method is already following my locking API rules and does the unlock as suggested above, and I will assume that any Sun auto-test that would be directly calling my Data.update or delete methods will also follow the rules. Thus, no unlocking in the specific Data.update or delete methods.
I don't know much about Bill's final question though regarding the RecordNotFoundException. My unlock is different - mine only throws SecurityException and not RecordNotFoundException.
Thanks Bill, Phil and Ken!!!
TJ

[Ken Krebs]

Bill,

My only question is lets say
you get a RecordNotFoundException. How can you then call unlock when
unlock also throws RecordNotFoundException. Unlock is just going to turn
around and throw it.

There is no need to call unlock if the lock call fails because of a RecordNotFoundException. NOTE: I did not put my lock call in the try block so that if the lock call causes a RecordNotFoundException, it will cause the book method to throw the exception up the calling hierarchy immediately.

[Bill Robertson]

awesome, thanks. The only thing with not unlocking when a record not found exception occurs the record stays in my lock collection. so if a new record is created and takes the place of the record not found it will be created as locked. unless you remove from the delete call but i don't know
how you would do that!
make sense?

[George Marinkovich]

Bill,

    try {
        controller.lock(recNo);
    } catch (ControllerException e) {
        throw e;
    }
    try {
        String[] record = controller.read(recNo);
        controller.update(recNo, record);
    } catch (ControllerException e) {
        throw e;
    } finally {
        controller.unlock(recNo);
    }

You might try two try/catch blocks as above. The first obtains a lock or
it throws an exception. In the second try/catch/finally the lock has been obtained so it is always appropriate to unlock in the finally block.
I hope this helps, or at least I hope it makes sense.
Anyway, good bye '03, hello '04 -- let the celebration begin,
George

[Bill Robertson]

hi george,
i see your point, but i do not see how this would prevent placing a new record into a position that is locked in my lock collection. because remember whenever you enter create you have no idea where yor new record is going to go.
bill

[George Marinkovich]

In your create method when you're searching for deleted records you might want to skip over those that are locked. That is, a suitable candidate location for the new record is one in which the record is both unlocked and marked for deletion.
My original design had a LOCK_DATABASE flag that could be passed to the lock method to lock the entire database. So, one could lock the entire database before the create call. I'm having second thoughts about this now, so I would currently lean toward the solution outlined in the first paragraph.
- George

[james airey]

I like the try {} finally {} approach.
I did once find a compiler that wouldn't allow it without a catch clause though!

[George Marinkovich]

Originally posted by james airey:
I like the try {} finally {} approach.
I did once find a compiler that wouldn't allow it without a catch clause though!

try/finally should be fine for the j2sdk which is what we're required to use for the SCJD. You're right about the code quoted above. As quoted, it would have been better to use try/finally rather than try/catch/finally. I edited my code to remove extraneous code (like exception logging in the exception handler) and didn't take it a step further and omit the catch block entirely.