exchange of locking/tracking sequence

After the patch for 3199 (20110623), the test4 will pass.
If exchange the lock and track sequence, the deadlock will show. I don't know why yet.

I think I know why.
If the exchange the lock and track sequence, then when deadlock happened, the object has not been tracked, so AbstractTransactionContext.load() which unregister the object will cause the NullPointException.

So I know that, we can't exchange the lock and track sequence. This task is not useful any more.

Your observation is correct but your conclusion isn't.

If you look at this in more deatil you will recognize that AbstractTransactionContext.load() has a try catch block around the call to LockEngine.load() that guaranties that ObjectTracker is left in a consistent state when anything goes wrong in LockEngine.load(). If you look at LockEngine.load() you will see another try catch block that takes care that ObjectLock is left in a consistent state if anything goes wrong inside of this block. What you have to do if you like to exchange sequence of tracking and locking is to also exchange these try catch blocks.

try {
  track
  try {
    aquire_lock
  } finally {
    confirm_lock
  }
} catch {
  untrack
}

try {
  aquire_lock
  try {
    track
  } catch {
    untrack
  }
} finally {
  confirm_lock
}

I don't know if this has impacts on anything else but at least that is how it should work in theory.

I think the LockEngine.load should take care of the track/untrack, and the AbstractTransactionContext.load should not take care of untrack.

So I invetigate who uses the tracked object, and then I shrink the track/untrack to ClassMolder.mold.

Now I have simplify the track/untrack in patchpatch-C3120-20110625.txt.

As exception handling seams not to be correct at previous patch I created a new one which now does untrack and confirm at exceptions correctly. Still there is space for improvements. What do you think Wensheng?

Please take into account that we have only a very small number of tests that test fail situations. Therefore we have to take special care on them at refactoring.

Yes, your patch is correct, because your patch keeps the same meaning with the old code exactly, I think it is absolutely right.

I also think there is still space for improvements for track/untrack. In the current implementation, if the object is extended, we need the track/untrack the old object, then track the right object. This is ugly.

In patch-C3120-20110625.txt, firstly, I carefully investigate which code throws which exception, so I can take care of them seperately (in fact, I found that the excetpion handling can be simplified). Secondly, I carefully read the code, who will use/affect the tracked object, I found that, in the LockEngine.load method, before the molder.mold(), the tracked object is not useful.

So I think we just need to track the object before it is used. So we just need to track the right object (no matter extended or not).

So I think in patch-C3120-20110625.txt, the code is simplified, and the same meaning is stayed.

You are correct but I prefer to commit latest patch as is and omit multiple track/untrack calls in a separate issue. In addition exception handling should be improved.