Derby
  1. Derby
  2. DERBY-4928

Deadlock-prone synchronization in BasicDependencyManager

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.8.1.2
    • Fix Version/s: 10.8.1.2
    • Component/s: SQL
    • Labels:
      None

      Description

      The synchronization in BasicDependencyManager is prone to deadlock, because database locks are obtained while holding the monitor on "this".
      Problem observed when testing the automatic index statistics update prototype.

      There are comments in the file suggesting that in-memory dependencies should be accessed while holding the Java monitor, but the monitor should not be held when accessing stored dependencies. The implementation is breaking this rule/suggestion.

        Issue Links

          Activity

          Hide
          Kristian Waagan added a comment -

          Attaching patch 1a.

          This patch primarily addresses incorrect synchronization in BasicDependencyManager. The rule was already described in the class, but the implementation didn't obey them:
          o use synchronized (this) when accessing in-memory dependencies (deps stored in BasicDependencyManager)
          o don't use synchronized (this) when accessing stored dependencies, but rely on database locking (deps stored in the data dictionary / a system table)

          I haven't come across cases where we need to synchronize over both sets of dependencies (at the same time).

          Main changes in BasicDependecyManager:
          o split body of addDepencendy into addInMemoryDependency and addStoredDependency
          o corrected JavaDoc for coreInvalidateFor (the flag 'forSync' doesn't exist)
          o fixed synchronization in clearDependencies (this is where the automatic index stats update feature got stuck in a deadlock)
          o reformatted (+tiny refactoring) clearInMemoryDependencies
          o deleted commented out methods getAllProviders and getAllProvidersInfos
          o removed synchronized for getPersistentProvidersInfos and rewrote method
          o removed synchronized for copyDependencies
          o fixed synchronization in countDependencies
          o deleted unused method dumpDependencies (and then bubbleSort)
          o rewrote methods getProviders and getDependents for new sync rules
          o replaced HashTable with Map
          o removed methods newSList

          Changes in ViewsTest and BaseJDBCTestCase:
          o added method assertStatementErrorUnordered, as the ordering of in-memory and stored dependencies changed to to the changes described above (this can easily be switched back, but I don't see why - decided to make the test more resilient to varying ordering instead).
          o used the new assert method were the test failed

          Re-running regression tests.
          Patch ready for review.

          Show
          Kristian Waagan added a comment - Attaching patch 1a. This patch primarily addresses incorrect synchronization in BasicDependencyManager. The rule was already described in the class, but the implementation didn't obey them: o use synchronized (this) when accessing in-memory dependencies (deps stored in BasicDependencyManager) o don't use synchronized (this) when accessing stored dependencies, but rely on database locking (deps stored in the data dictionary / a system table) I haven't come across cases where we need to synchronize over both sets of dependencies (at the same time). Main changes in BasicDependecyManager: o split body of addDepencendy into addInMemoryDependency and addStoredDependency o corrected JavaDoc for coreInvalidateFor (the flag 'forSync' doesn't exist) o fixed synchronization in clearDependencies (this is where the automatic index stats update feature got stuck in a deadlock) o reformatted (+tiny refactoring) clearInMemoryDependencies o deleted commented out methods getAllProviders and getAllProvidersInfos o removed synchronized for getPersistentProvidersInfos and rewrote method o removed synchronized for copyDependencies o fixed synchronization in countDependencies o deleted unused method dumpDependencies (and then bubbleSort) o rewrote methods getProviders and getDependents for new sync rules o replaced HashTable with Map o removed methods newSList Changes in ViewsTest and BaseJDBCTestCase: o added method assertStatementErrorUnordered, as the ordering of in-memory and stored dependencies changed to to the changes described above (this can easily be switched back, but I don't see why - decided to make the test more resilient to varying ordering instead). o used the new assert method were the test failed Re-running regression tests. Patch ready for review.
          Hide
          Kristian Waagan added a comment -

          Regression tests passed.
          I haven't seen the hang with the automatic index statistics update feature using this patch either,although I haven't run the tests that many times yet.

          Show
          Kristian Waagan added a comment - Regression tests passed. I haven't seen the hang with the automatic index statistics update feature using this patch either,although I haven't run the tests that many times yet.
          Hide
          Knut Anders Hatlen added a comment -

          Looks like a good cleanup to me. +1
          (One small typo in a comment in BaseJDBCTestCase: "Run throgh" -> "Run through")

          Show
          Knut Anders Hatlen added a comment - Looks like a good cleanup to me. +1 (One small typo in a comment in BaseJDBCTestCase: "Run throgh" -> "Run through")
          Hide
          Kristian Waagan added a comment -

          Thanks for the review, Knut.
          I fixed the comment before committing patch 1a to trunk with revision 1043367.

          I don't plan to backport this fix, at least not now. We don't have any reports for this problem for earlier versions, so I'd rather see that the changes go through more testing before backporting.
          For the record, it was the automatic index statistics update feature that triggered the bug.

          Show
          Kristian Waagan added a comment - Thanks for the review, Knut. I fixed the comment before committing patch 1a to trunk with revision 1043367. I don't plan to backport this fix, at least not now. We don't have any reports for this problem for earlier versions, so I'd rather see that the changes go through more testing before backporting. For the record, it was the automatic index statistics update feature that triggered the bug.
          Hide
          Kristian Waagan added a comment -

          Closing issue.

          Show
          Kristian Waagan added a comment - Closing issue.

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Kristian Waagan
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development