Re: [PATCH 2/2] python-cffi: returned OwnedMessage objects from Message.replies

[Floris Bruynooghe <flub@devork.be>]

On Sat 08 Jan 2022 at 10:03 -0400, David Bremner wrote:

> If we return regular Message objects, python will try to destroy them,
> and the underlying notmuch object, causing e.g. the crash [1].
>
> [1]: id:87sfu6utxg.fsf@tethera.net
> ---
>  bindings/python-cffi/notmuch2/_message.py | 4 ++--
>  test/T392-python-cffi-notmuch.sh          | 2 --
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_message.py b/bindings/python-cffi/notmuch2/_message.py
> index a460d8c1..aa1cb875 100644
> --- a/bindings/python-cffi/notmuch2/_message.py
> +++ b/bindings/python-cffi/notmuch2/_message.py
> @@ -371,14 +371,14 @@ class Message(base.NotmuchObject):
>          This method will only work if the message was created from a
>          thread.  Otherwise it will yield no results.
>  
> -        :returns: An iterator yielding :class:`Message` instances.
> +        :returns: An iterator yielding :class:`OwnedMessage` instances.
>          :rtype: MessageIter
>          """
>          # The notmuch_messages_valid call accepts NULL and this will
>          # become an empty iterator, raising StopIteration immediately.
>          # Hence no return value checking here.
>          msgs_p = capi.lib.notmuch_message_get_replies(self._msg_p)
> -        return MessageIter(self, msgs_p, db=self._db)
> +        return MessageIter(self, msgs_p, db=self._db, msg_cls=OwnedMessage)
>  
>      def __hash__(self):
>          return hash(self.messageid)
> diff --git a/test/T392-python-cffi-notmuch.sh b/test/T392-python-cffi-notmuch.sh
> index 50012c55..15c8fc6b 100755
> --- a/test/T392-python-cffi-notmuch.sh
> +++ b/test/T392-python-cffi-notmuch.sh
> @@ -24,13 +24,11 @@ show_msgs(thread, 0)
>  EOF
>  
>  test_begin_subtest "recursive traversal of replies (no crash)"
> -test_subtest_known_broken
>  test_python < recurse.py
>  error=$?
>  test_expect_equal "${error}" 0
>  
>  test_begin_subtest "recursive traversal of replies (output)"
> -test_subtest_known_broken
>  test_python < recurse.py
>  tail -n 10 < OUTPUT > OUTPUT.sample
>  cat <<EOF > EXPECTED

oh, nice debugging!  And yes, this seems like the right fix, glad the
mechanism was at least already in place.

With respect to the docs, they seem clear enough.  Probably I missed
this or I simply didn't realise that the replies iter is basically a
thread owning it.

Only thing I'd do different is write a pytest test for this as well, but
than I wouldn't have written the notmuch tests. So I don't think this
matters that much.

Cheers,
Floris