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 <[email protected]>
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 <[email protected]>
Reviewers: Enrico Olivelli <[email protected]>, Yiming Zang
<[email protected]>, Sijie Guo <[email protected]>
This closes #990 from ivankelly/inf-loop
(cherry picked from commit 26c3379c9fdd812ef36da6176d83e26179705535)
Signed-off-by: Enrico Olivelli <[email protected]>
---
.../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
['"[email protected]" <[email protected]>'].
