changeset 1550:67b28d657f62

sqliterevmap: break ".hashes()" cycle in a safer way The `fromsvn()` revset implementation could cause weakref error when using sqliterevmap like: File "hgsubversion/util.py", line 357, in <lambda> return subset.filter(lambda r: tonode(r) in hashes) File "hgsubversion/maps.py", line 542, in __contains__ return self.get(key) != None File "hgsubversion/maps.py", line 533, in get for row in self.revmap._query( ReferenceError: weakly-referenced object no longer exists Basically the seemingly harmless assignment could break surprisingly: # dangerous: `hashes` does not have a reference of `meta.revmap` and may # become unavailable after `meta`, `revmap` being released by refcount. hashes = meta.revmap.hashes() The above syntax is nice to support while avoiding cycles is also nice. This patch removes `revmap._hashes` so the revmap no longer owns a reference of a `ReverseRevMap` object so the `ReverseRevMap` object no longer needs to use weakref for `self.revmap`. This could actually be caught by `comprehensive/test_sqlite_revmap.py`. I was not careful enough verifying the "fromsvn()" patch.
author Jun Wu <quark@fb.com>
date Thu, 21 Dec 2017 17:39:52 -0800
parents 8410a978c650
children 22137e94411f
files hgsubversion/maps.py
diffstat 1 files changed, 10 insertions(+), 11 deletions(-) [+]
line wrap: on
line diff
--- a/hgsubversion/maps.py
+++ b/hgsubversion/maps.py
@@ -523,9 +523,9 @@ class SqliteRevMap(collections.MutableMa
     class ReverseRevMap(object):
         # collections.Mapping is not suitable since we don't want 2/3 of
         # its required interfaces: __iter__, __len__.
-        def __init__(self, revmap):
-            self.revmap = weakref.proxy(revmap)
-            self._cache = {}
+        def __init__(self, revmap, cache):
+            self.revmap = revmap
+            self._cache = cache
 
         def get(self, key, default=None):
             if key not in self._cache:
@@ -565,18 +565,17 @@ class SqliteRevMap(collections.MutableMa
         self._lastpulledpath = lastpulled_path
 
         self._db = None
-        self._hashes = None
         self._sqlitepragmas = sqlitepragmas
         self.firstpulled = 0
         self._updatefirstlastpulled()
         # __iter__ is expensive and thus disabled by default
         # it should only be enabled for testing
         self._allowiter = False
+        # used by self.hashes(), {hash: (rev, branch)}
+        self._hashescache = {}
 
     def hashes(self):
-        if self._hashes is None:
-            self._hashes = self.ReverseRevMap(self)
-        return self._hashes
+        return self.ReverseRevMap(self, self._hashescache)
 
     def branchedits(self, branch, revnum):
         return [((r[0], r[1] or None), bytes(r[2])) for r in
@@ -609,7 +608,7 @@ class SqliteRevMap(collections.MutableMa
         hgutil.unlinkpath(self._dbpath, ignoremissing=True)
         hgutil.unlinkpath(self._rowcountpath, ignoremissing=True)
         self._db = None
-        self._hashes = None
+        self._hashescache = {}
         self._firstpull = None
         self._lastpull = None
 
@@ -646,15 +645,15 @@ class SqliteRevMap(collections.MutableMa
             self.firstpulled = revnum
         if revnum > self.lastpulled or not self.lastpulled:
             self.lastpulled = revnum
-        if self._hashes is not None:
-            self._hashes._cache[binha] = key
+        if self._hashescache:
+            self._hashescache[binha] = key
 
     def __delitem__(self, key):
         for row in self._querybykey('DELETE', key):
             if self.rowcount > 0:
                 self.rowcount -= 1
             return
-        # For performance reason, self._hashes is not updated
+        # For performance reason, self._hashescache is not updated
         raise KeyError(key)
 
     @contextlib.contextmanager