Skip to content

Commit

Permalink
Allow weakref callback to be a no-op during iterator cleanup
Browse files Browse the repository at this point in the history
When running the tests on Pypy, sometimes this error occurs:

    Exception ignored in: weakref callback:
    <cyfunction __init__.<locals>.<lambda> at 0x227aec0>

      Traceback (most recent call last):
        File "plyvel/_plyvel.pyx", line 695, in plyvel._plyvel.BaseIterator.__init__.lambda
          lambda wr: ref_dict.pop(iterator_id))
      KeyError: 140185035039968

This issue manifests only sporadically, and may be related to cleanup
order (garbage collection etc.) being different on CPython and Pypy.

LevelDB requires that iterators are closed before a database is closed,
so plyvel tracks ‘active iterators’ using weak references (‘ref_dict’ in
the traceback above). Active iterators are cleaned up when a database is
closed. Apparently the weakref callback sometimes triggers after the
weakref has already been removed from the collection, and this raises
the KeyError. Ignoring this should be fine since it means the iterator
is closed already.

Simplify a bit by using a set instead of a dict to track the weakrefs,
and use set.discard() in the callback, which is a no-op (no exception)
for missing items.

See #140.
  • Loading branch information
wbolster committed Jan 9, 2022
1 parent 30e59b4 commit 30055cd
Showing 1 changed file with 8 additions and 12 deletions.
20 changes: 8 additions & 12 deletions plyvel/_plyvel.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ cdef class DB:
cdef Options options
cdef readonly object name
cdef object lock
cdef dict iterators
cdef set iterator_weakrefs

def __init__(self, name, *, bool create_if_missing=False,
bool error_if_exists=False, paranoid_checks=None,
Expand Down Expand Up @@ -252,17 +252,17 @@ cdef class DB:
# leveldb/db.h). We don't use weakref.WeakValueDictionary here
# for performance reasons.
self.lock = threading.Lock()
self.iterators = dict()
self.iterator_weakrefs = set()

cpdef close(self):
# If the constructor raised an exception (and hence never
# completed), self.iterators can be None. In that case no
# iterators need to be cleaned anyway.
# completed), self.iterator_weakrefs can be None. In that
# case no iterators need to be cleaned anyway.
cdef BaseIterator iterator
if self.iterators is not None:
if self.iterator_weakrefs is not None:
with self.lock:
while self.iterators:
iterator = self.iterators.popitem()[1]()
while self.iterator_weakrefs:
iterator = self.iterator_weakrefs.pop()()
if iterator is not None:
iterator.close()

Expand Down Expand Up @@ -688,11 +688,7 @@ cdef class BaseIterator:
self._iter = db._db.NewIterator(read_options)

# Store a weak reference on the db (needed when closing db)
iterator_id = id(self)
ref_dict = db.iterators
ref_dict[iterator_id] = weakref_ref(
self,
lambda wr: ref_dict.pop(iterator_id))
db.iterator_weakrefs.add(weakref_ref(self, db.iterator_weakrefs.discard))

cpdef close(self):
if self._iter is not NULL:
Expand Down

0 comments on commit 30055cd

Please sign in to comment.