Details

    • Type: Sub-task Sub-task
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.2
    • Fix Version/s: 1.3.3rc1
    • Component/s: JDO
    • Labels:
      None
    • Number of attachments :
      2

      Description

      In current implementation, the transaction will first track an object, the acquire object lock. This is counterintuitive. Because if the transaction is not locked properly, the tracked object in the transaction will not usefu.

      1. patch-C3120-20110625.txt
        5 kB
        Wensheng Dou
      2. patch-C3120-20110705.txt
        14 kB
        Ralf Joachim

        Activity

        Hide
        Wensheng Dou added a comment -

        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.

        Show
        Wensheng Dou added a comment - 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.
        Hide
        Wensheng Dou added a comment -

        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.

        Show
        Wensheng Dou added a comment - 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.
        Hide
        Ralf Joachim added a comment -

        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.

        current
        try {
          track
          try {
            aquire_lock
          } finally {
            confirm_lock
          }
        } catch {
          untrack
        }
        
        exchanged
        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.

        Show
        Ralf Joachim added a comment - 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. current try { track try { aquire_lock } finally { confirm_lock } } catch { untrack } exchanged 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.
        Hide
        Wensheng Dou added a comment - - edited

        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.

        Show
        Wensheng Dou added a comment - - edited 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.
        Hide
        Ralf Joachim added a comment -

        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?

        Show
        Ralf Joachim added a comment - 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?
        Hide
        Ralf Joachim added a comment -

        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.

        Show
        Ralf Joachim added a comment - 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.
        Hide
        Wensheng Dou added a comment -

        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.

        Show
        Wensheng Dou added a comment - 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.
        Hide
        Ralf Joachim added a comment -

        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.

        Show
        Ralf Joachim added a comment - 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.

          People

          • Assignee:
            Wensheng Dou
            Reporter:
            Wensheng Dou
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: