You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by eo...@apache.org on 2018/01/17 15:06:53 UTC
[bookkeeper] branch branch-4.6 updated: Fix tight loop in
getFileInfo
This is an automated email from the ASF dual-hosted git repository.
eolivelli pushed a commit to branch branch-4.6
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/branch-4.6 by this push:
new 569a482 Fix tight loop in getFileInfo
569a482 is described below
commit 569a4822595d31e1929d096588290e5719723dbc
Author: Ivan Kelly <iv...@apache.org>
AuthorDate: Wed Jan 17 16:06:22 2018 +0100
Fix tight loop in getFileInfo
The argument to assertions is not evaluated if assertions is disabled,
which was messing up the refcounting for the fileinfo backing
cache. We ended up with dead fileinfo objects in the guava caches,
which triggered an infinite loop.
This patch fixes that by moving the tryRetain() out of the assert. It
also adds defensive code to getFileInfo to ensure that if a fileinfo
object is dead, that it doesn't exist any longer in the caches.
Author: Ivan Kelly <iv...@apache.org>
Reviewers: Enrico Olivelli <eo...@gmail.com>, Yiming Zang <yz...@gmail.com>, Sijie Guo <si...@apache.org>
This closes #990 from ivankelly/inf-loop
(cherry picked from commit 26c3379c9fdd812ef36da6176d83e26179705535)
Signed-off-by: Enrico Olivelli <eo...@apache.org>
---
.../apache/bookkeeper/bookie/FileInfoBackingCache.java | 6 ++++--
.../apache/bookkeeper/bookie/IndexPersistenceMgr.java | 16 +++++++++++++++-
2 files changed, 19 insertions(+), 3 deletions(-)
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 8cdadc7..e4cb183 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 @@ class FileInfoBackingCache {
// 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 @@ class FileInfoBackingCache {
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 0945506..05b5eb5 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 @@ public class IndexPersistenceMgr {
} 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) {
--
To stop receiving notification emails like this one, please contact
['"commits@bookkeeper.apache.org" <co...@bookkeeper.apache.org>'].