On Wed 05 Feb 2025 at 20:01 -0700, Lars Kotthoff wrote:

>> 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 :)

No, on Debian Linux...

>> 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.

Patch looks good!  Thanks for this, and thanks for adding the tests!

> From ea42908d4de15c41487470cba0fd7334a4d69fa7 Mon Sep 17 00:00:00 2001
> From: Lars Kotthoff <lars@larsko.org>
> 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')
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org