Solr
  1. Solr
  2. SOLR-1538

Solr possible deadlock source (FindBugs report)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 1.4.1, 1.5, 3.1, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Environment:

      platform independent

      Description

      The code to get the latest accessed items in ConcurrentLRUCache looks like

      ConcurrentLRUCache.java
       public Map<K, V> getOldestAccessedItems(int n) {
          markAndSweepLock.lock();
          Map<K, V> result = new LinkedHashMap<K, V>();
          TreeSet<CacheEntry> tree = new TreeSet<CacheEntry>();
          try {
         ...
          } finally {
            markAndSweepLock.unlock();
          }
      

      (this method is apparently unused though) and in

         public Map<K,V> getLatestAccessedItems(int n) {
           // we need to grab the lock since we are changing lastAccessedCopy
           markAndSweepLock.lock();
           Map<K,V> result = new LinkedHashMap<K,V>();
           TreeSet<CacheEntry> tree = new TreeSet<CacheEntry>();
           try {
      ...
      

      The impression is that if an OOM situation occurs on the allocation of the local LinkedHashMap and TreeSet the lock would not be unlocked anymore.
      The quick fix would be to move the lock() call after the allocations, and this does not seem to imply any problem.

      1. SOLR-1538.patch
        1 kB
        gabriele renzi

        Activity

        Hide
        gabriele renzi added a comment -

        simply switch order of ollections allocation and lock aquisition, all tests still pass and it makes the locked zone smaller

        Show
        gabriele renzi added a comment - simply switch order of ollections allocation and lock aquisition, all tests still pass and it makes the locked zone smaller
        Hide
        Hoss Man added a comment -

        I'm not a threading/locking expert, but this seems straight forward enough.

        All the docs i've seen definitely say that a "try" block (with "unlock" in the finally) should be the very next line after any lock() call.

        Show
        Hoss Man added a comment - I'm not a threading/locking expert, but this seems straight forward enough. All the docs i've seen definitely say that a "try" block (with "unlock" in the finally) should be the very next line after any lock() call.
        Hide
        Hoss Man added a comment -

        Committed revision 898119.

        Show
        Hoss Man added a comment - Committed revision 898119.
        Hide
        Hoss Man added a comment -

        Correcting Fix Version based on CHANGES.txt, see this thread for more details...

        http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

        Show
        Hoss Man added a comment - Correcting Fix Version based on CHANGES.txt, see this thread for more details... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E
        Hide
        Hoss Man added a comment -

        Committed revision 949885.

        merged to branch-1.4 for 1.4.1

        Show
        Hoss Man added a comment - Committed revision 949885. merged to branch-1.4 for 1.4.1
        Hide
        Hoss Man added a comment -

        Committed revision 949885.

        merged to branch-1.4 for 1.4.1

        Show
        Hoss Man added a comment - Committed revision 949885. merged to branch-1.4 for 1.4.1

          People

          • Assignee:
            Hoss Man
            Reporter:
            gabriele renzi
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 10m
              10m
              Remaining:
              Remaining Estimate - 10m
              10m
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development