> Hum, it seems *all* iterators are destroyed too early? I think it is > just luck that this works for threads, also the 2nd for loop in the > above code seems like just luck.
Yes — looks like the memory management doesn't necessarily free everything immediately; that's what must be causing it to work sometimes. > (FWIW I get a SIGABRT instead of SIGSEGV from these.) Ah, you must be on a Mac :) > Try removing the `self.destroy()` line from > `_base.NotmuchIter.__next__()`. I'm not sure why that destroy was ever > there, I think it is a bug to call free on the iterator so early. Done that; seems to work now. > I can do a proper patch in the next few days probably if no one beats me > to it. Patch attached — I removed a similarly troublesome _destroy() in another place and added unit tests to check this. FWIW, I wasn't able to get it to segfault with the threads or properties iterator, even though it should. I suspect notmuch's memory management; added the tests just in case. Cheers, Lars
From ea42908d4de15c41487470cba0fd7334a4d69fa7 Mon Sep 17 00:00:00 2001 From: Lars Kotthoff <[email protected]> Date: Wed, 5 Feb 2025 19:52:51 -0700 Subject: [PATCH] fix segfaults in Python cFFI API and add tests --- bindings/python-cffi/notmuch2/_base.py | 3 +- bindings/python-cffi/notmuch2/_message.py | 3 +- bindings/python-cffi/notmuch2/_query.py | 2 +- bindings/python-cffi/tests/test_database.py | 31 +++++++++++++++++++++ bindings/python-cffi/tests/test_message.py | 14 ++++++++++ 5 files changed, 48 insertions(+), 5 deletions(-) diff --git a/bindings/python-cffi/notmuch2/_base.py b/bindings/python-cffi/notmuch2/_base.py index 1cf03c88..c83abd01 100644 --- 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 d4b34e91..abe37db6 100644 --- 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 1db6ec96..169fcf27 100644 --- 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 f1d12ea6..1557235d 100644 --- 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 56701d05..3b103530 100644 --- 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') -- 2.48.1
_______________________________________________ notmuch mailing list -- [email protected] To unsubscribe send an email to [email protected]
