Mercurial > hgsubversion
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