[GitHub] eolivelli closed pull request #990: Fix tight loop in getFileInfo

[GitBox <gi...@apache.org>]

eolivelli closed pull request #990: Fix tight loop in getFileInfo
URL: https://github.com/apache/bookkeeper/pull/990
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
index 8cdadc79c..e4cb18392 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
@@ -52,7 +52,8 @@ CachedFileInfo loadFileInfo(long ledgerId, byte[] masterKey) throws IOException
                 // have been able to get a reference to it here.
                 // The caller of loadFileInfo owns the refence, and is
                 // responsible for calling the corresponding #release().
-                assert(fi.tryRetain());
+                boolean retained = fi.tryRetain();
+                assert(retained);
                 return fi;
             }
         } finally {
@@ -67,7 +68,8 @@ CachedFileInfo loadFileInfo(long ledgerId, byte[] masterKey) throws IOException
             fileInfos.put(ledgerId, fi);
 
             // see comment above for why we assert
-            assert(fi.tryRetain());
+            boolean retained = fi.tryRetain();
+            assert(retained);
             return fi;
         } finally {
             lock.writeLock().unlock();
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexPersistenceMgr.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexPersistenceMgr.java
index 0945506b4..05b5eb570 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexPersistenceMgr.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexPersistenceMgr.java
@@ -220,7 +220,21 @@ CachedFileInfo getFileInfo(final Long ledger, final byte masterKey[]) throws IOE
                 } else {
                     fi = readFileInfoCache.get(ledger, loader);
                 }
-            } while (!fi.tryRetain());
+                if (!fi.tryRetain()) {
+                    // defensively ensure that dead fileinfo objects don't exist in the
+                    // cache. They shouldn't if refcounting is correct, but if someone
+                    // does a double release, the fileinfo will be cleaned up, while
+                    // remaining in the cache, which could cause a tight loop in this method.
+                    boolean inWriteMap = writeFileInfoCache.asMap().remove(ledger, fi);
+                    boolean inReadMap = readFileInfoCache.asMap().remove(ledger, fi);
+                    if (inWriteMap || inReadMap) {
+                        LOG.error("Dead fileinfo({}) forced out of cache (write:{}, read:{}). "
+                                  + "It must have been double-released somewhere.",
+                                  fi, inWriteMap, inReadMap);
+                    }
+                    fi = null;
+                }
+            } while (fi == null);
 
             return fi;
         } catch (ExecutionException | UncheckedExecutionException ee) {


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services