fix segfaults in Python cFFI API and add tests

Several iterators in the Python cFFI API destroyed the objects they iterated
over too early (when the iterator was exhausted), causing subsequent segfaults
in common cases like creating a list from the iterator. This patch fixes the
segfaults and add tests to ensure that they don't happen again.

diff --git a/bindings/python-cffi/notmuch2/_base.py b/bindings/python-cffi/notmuch2/_base.py
index 1cf03c8813e5b03dffbc069fda525aad411a7eba..c83abd013c420b25f9c92cdee5043880675969d0 100644 (file)
--- a/bindings/python-cffi/notmuch2/_base.py
+++ b/bindings/python-cffi/notmuch2/_base.py
@@ -167,7 +167,7 @@ class NotmuchIter(NotmuchObject, collections.abc.Iterator):
     It is tempting to use a generator function instead, but this would
     not correctly respect the :class:`NotmuchObject` memory handling
     protocol and in some unsuspecting cornercases cause memory
-    trouble.  You probably want to sublcass this in order to wrap the
+    trouble.  You probably want to subclass this in order to wrap the
     value returned by :meth:`__next__`.
 
     :param parent: The parent object.
@@ -223,7 +223,6 @@ class NotmuchIter(NotmuchObject, collections.abc.Iterator):
 
     def __next__(self):
         if not self._fn_valid(self._iter_p):
-            self._destroy()
             raise StopIteration()
         obj_p = self._fn_get(self._iter_p)
         self._fn_next(self._iter_p)

diff --git a/bindings/python-cffi/notmuch2/_message.py b/bindings/python-cffi/notmuch2/_message.py
index d4b34e91e2f88ff367a73cfa5b311d11cbfa6b9a..abe37db6c915480a8a2522d5caf97f272b04920a 100644 (file)
--- a/bindings/python-cffi/notmuch2/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -610,7 +610,7 @@ class PropertiesMap(base.NotmuchObject, collections.abc.MutableMapping):
     def getall(self, prefix='', *, exact=False):
         """Return an iterator yielding all properties for a given key prefix.
 
-        The returned iterator yields all peroperties which start with
+        The returned iterator yields all properties which start with
         a given key prefix as ``(key, value)`` namedtuples.  If called
         with ``exact=True`` then only properties which exactly match
         the prefix are returned, those a key longer than the prefix
@@ -655,7 +655,6 @@ class PropertiesIter(base.NotmuchIter):
 
     def __next__(self):
         if not self._fn_valid(self._iter_p):
-            self._destroy()
             raise StopIteration
         key = capi.lib.notmuch_message_properties_key(self._iter_p)
         value = capi.lib.notmuch_message_properties_value(self._iter_p)

diff --git a/bindings/python-cffi/notmuch2/_query.py b/bindings/python-cffi/notmuch2/_query.py
index 1db6ec966b79859618b327094feea4fb921fe59f..169fcf272dcef15e7879aa92166f4b2195506b85 100644 (file)
--- a/bindings/python-cffi/notmuch2/_query.py
+++ b/bindings/python-cffi/notmuch2/_query.py
@@ -52,7 +52,7 @@ class Query(base.NotmuchObject):
         This executes the query and returns an iterator over the
         :class:`Message` objects found.
         """
-        msgs_pp = capi.ffi.new('notmuch_messages_t**')
+        msgs_pp = capi.ffi.new('notmuch_messages_t **')
         ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp)
         if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
             raise errors.NotmuchError(ret)

diff --git a/bindings/python-cffi/tests/test_database.py b/bindings/python-cffi/tests/test_database.py
index f1d12ea6c2f046186377a7ce48b104d3a5855873..1557235d76ea7b354dcc53042964a1ad2b7f2368 100644 (file)
--- a/bindings/python-cffi/tests/test_database.py
+++ b/bindings/python-cffi/tests/test_database.py
@@ -303,6 +303,18 @@ class TestQuery:
         msgs = db.messages('*')
         assert isinstance(msgs, collections.abc.Iterator)
 
+    def test_messages_iterator(self, db):
+        for msg in db.messages('*'):
+            assert isinstance(msg, notmuch2.Message)
+            assert isinstance(msg.messageid, str)
+
+    def test_messages_iterator_list(self, db):
+        msgs = list(db.messages('*'))
+        assert len(msgs) == 3
+        for msg in msgs:
+            assert isinstance(msg, notmuch2.Message)
+            assert isinstance(msg.messageid, str)
+
     def test_message_no_results(self, db):
         msgs = db.messages('not_a_matching_query')
         with pytest.raises(StopIteration):
@@ -320,6 +332,25 @@ class TestQuery:
         threads = db.threads('*')
         assert isinstance(threads, collections.abc.Iterator)
 
+    def test_threads_iterator(self, db):
+        for t in db.threads('*'):
+            assert isinstance(t, notmuch2.Thread)
+            assert isinstance(t.threadid, str)
+            for msg in t:
+                assert isinstance(msg, notmuch2.Message)
+                assert isinstance(msg.messageid, str)
+
+    def test_threads_iterator_list(self, db):
+        threads = list(db.threads('*'))
+        assert len(threads) == 2
+        for t in threads:
+            assert isinstance(t, notmuch2.Thread)
+            assert isinstance(t.threadid, str)
+            msgs = list(t)
+            for msg in msgs:
+                assert isinstance(msg, notmuch2.Message)
+                assert isinstance(msg.messageid, str)
+
     def test_threads_no_match(self, db):
         threads = db.threads('not_a_matching_query')
         with pytest.raises(StopIteration):

diff --git a/bindings/python-cffi/tests/test_message.py b/bindings/python-cffi/tests/test_message.py
index 56701d056de5ff6d1cda5e7fc0c53b1cdd8a6de5..3b103530922e8adac28a7902fe58114ee4f5309d 100644 (file)
--- a/bindings/python-cffi/tests/test_message.py
+++ b/bindings/python-cffi/tests/test_message.py
@@ -218,6 +218,20 @@ class TestProperties:
         props.add('foo', 'a')
         assert set(props.getall('foo')) == {('foo', 'a')}
 
+    def test_getall_iter(self, props):
+        props.add('foo', 'a')
+        props.add('foobar', 'b')
+        for prop in props.getall('foo'):
+            assert isinstance(prop.value, str)
+
+    def test_getall_iter_list(self, props):
+        props.add('foo', 'a')
+        props.add('foobar', 'b')
+        res = list(props.getall('foo'))
+        assert len(res) == 2
+        for prop in res:
+            assert isinstance(prop.value, str)
+
     def test_getall_prefix(self, props):
         props.add('foo', 'a')
         props.add('foobar', 'b')