ObjectNotFoundException during load() appears to not release in-memory read lock

I reproduced the problem with a simple class and the MAX generator.

The sequence is

• LockEngine.load calls typeInfo.acquire to create the lock
• ObjectNotFoundException is thrown
• LockEngine.load calls lock.confirm( tx, false ) => the lock is
  no longer active, but still in the locks Map
• When the new Object is created, its temporary OID (before key
  generation) must be converted to the new one, which happen to be
  equals to the one generated by the load call
  => crash in TypeInfo.rename

I first tried to remove the lock when catching the first exception,
but some test cases are broken by this.
The next thing to try is to use a more precise test in TypeInfo.rename, I
tried :

if ( newentry != null && !newentry.isDisposable())
throw new LockNotGrantedException("Lock is already existed for the new oid.");

All tests are OK with this one but I'm not yet sure of the validity of this
patch... Some more work on this is needed.

Michael, you wouldn't be able to attach a patch for the solution outlined below ?

Stein, is this bug about the same problem as bug 1519 ?

Yes, it's the same as #1519.

In this case, can we close one and mark it as duplicate of the other ? E.g.
close this one and leave 1519 as valid accepted ? Or the other way around ?

Keep the original and mark bug 1519 as duplicate.

• • • Bug 1519 has been marked as a duplicate of this bug. ***

Created an attachment (id=504)
Demonstration patch

In my original bug 1519 I didn't have a problem with the key-generator, I just
observed the item in locks was not removed.

Perhaps we shouldn't have made it a dup?

The patch I attached prints true or false if the item is in the locks, do we
agree that this is the problem?

> The patch I attached prints true or false if the item is in the
> locks, do we agree that this is the problem?
Yes, I think this is the problem.

Can we attach a small JUnit test case that e.g. insert a couple of recores to a
table, then tries to load an object instance that does not exist, etc? Just for
the sake of this discussion .. .

Gregory, just wondering whether this sounds familiar to you ?
ObjectNotFoundExceptions not releasing a lock ... or am I confusing two
completely unrelated things ?

No, feels the same to me. I've just got gut feelings about this...

The issue, if I'm going to make a guess, is going to be the same issue I have with
persist.TransactionManager's explicit unlock()ing....

It would be really, really nice if locks were always safe to call release on, and release() was always
called 100% of the time, regardless of reason, and all statements in TransactionManager did a catch
Throwable to ensure that those releases took place appropriately, regardless of the exception or
throwable in the underlying code, and preventing the locks from getting into a complete state.

Again, I don't know enough about this code to be sure - but I'm familiar enough with locking semantics
to get a pretty good feel for what looks like pretty bad behavior. Not only do we not call release() often
enough, but rollback() doesn't look like it will behave properly if a problem happens during
revertObject() - the lock never gets released, and all we do is print the exception. (And that shouldn't
be an except.printStackTrace(), either)

I'd like to see better safety in that class.

> It would be really, really nice if locks were always safe to call
> release on, and release() was always called 100% of the time, regardless
> of reason, and all statements in TransactionManager did a catch Throwable
> to ensure that those releases took place appropriately, regardless of
> the exception or throwable in the underlying code, and preventing the
> locks from getting into a complete state.
100% agreed. Most definitely. all exception handling should be converted to
catch Throwable, and if special behaviour is required, instanceof statements
should be used within the catch clause to selectively throw different
exceptions as a result of the original exception. Next, we should try to use
finally clauses wherever possible, making sure that locks are being released
properly despite exceptions being thrown elsewhere.

I'll come up with a to-do list as part of this bug, and let me see how much
spare time I can dedicate to this... .

To-do list:

• replace all occurences of e.printStackTrace with calls to logging API.
• Amend code to catch Throwable in all places.
• Use finally clause wherever possible, making sure that a lock is released
  properly despite exceptions being thropwn elsewhere.
• If special exception handling is required, implement this as part of the
  catch clause (using instanceof).

And I assume that we are talking about refactoring 'TransactionContext',
correct ?

Throwable's a bit of a funny one - because I think it catches out of memory exceptions as well, which, if
not thrown all the way, will result in weak referenced objects not getting garbage collected. Anything
that's a VirtualMachineError needs to be rethrown if you do that. Still good to release the lock before
rethrowing, of course.

Demonstration patch reattached from bugzilla.

Okay, looking at his again .. . I think if somebody provided me with a complete test case (as set out in src/bugs/jdo/template), this would be absolutely great. The above to-do list still looks like quite a well-written recipe to me ... any volunteers (but Ralf and me) ? Greg ? Given your apparent familiarity in this area, this should be quite straight-forward to you .. .

Added test to verify that ObjectNotFoundException during load() releases in-memory read lock. As the test does not show any failure i'll commit this as is.