From 2d895a0119b423b117d10e890c9e0eb5d2a9cdf8 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Mon, 15 Jun 2020 22:58:50 +0200 Subject: [PATCH] Make messages returned by Thread objects owned This reverses the logic of StandaloneMessage to instead create a OwnedMessage. Only the Thread class allows retrieving messages more then once so it can explicitly create such messages. The added test fails with SIGABRT without the fix for the message re-use in threads being present. --- bindings/python-cffi/notmuch2/_database.py | 6 +-- bindings/python-cffi/notmuch2/_message.py | 55 ++++++++++++--------- bindings/python-cffi/notmuch2/_thread.py | 8 ++- bindings/python-cffi/tests/test_database.py | 11 +++++ 4 files changed, 51 insertions(+), 29 deletions(-) diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py index fc55fea8..3c06402d 100644 --- a/bindings/python-cffi/notmuch2/_database.py +++ b/bindings/python-cffi/notmuch2/_database.py @@ -400,7 +400,7 @@ class Database(base.NotmuchObject): capi.lib.NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID] if ret not in ok: raise errors.NotmuchError(ret) - msg = message.StandaloneMessage(self, msg_pp[0], db=self) + msg = message.Message(self, msg_pp[0], db=self) if sync_flags: msg.tags.from_maildir_flags() return self.AddedMessage( @@ -469,7 +469,7 @@ class Database(base.NotmuchObject): msg_p = msg_pp[0] if msg_p == capi.ffi.NULL: raise LookupError - msg = message.StandaloneMessage(self, msg_p, db=self) + msg = message.Message(self, msg_p, db=self) return msg def get(self, filename): @@ -502,7 +502,7 @@ class Database(base.NotmuchObject): msg_p = msg_pp[0] if msg_p == capi.ffi.NULL: raise LookupError - msg = message.StandaloneMessage(self, msg_p, db=self) + msg = message.Message(self, msg_p, db=self) return msg @property diff --git a/bindings/python-cffi/notmuch2/_message.py b/bindings/python-cffi/notmuch2/_message.py index 416ce7ca..02de50ad 100644 --- a/bindings/python-cffi/notmuch2/_message.py +++ b/bindings/python-cffi/notmuch2/_message.py @@ -47,9 +47,7 @@ class Message(base.NotmuchObject): :type db: Database :param msg_p: The C pointer to the ``notmuch_message_t``. :type msg_p: - :param dup: Whether the message was a duplicate on insertion. - :type dup: None or bool """ _msg_p = base.MemoryPointer() @@ -61,10 +59,22 @@ class Message(base.NotmuchObject): @property def alive(self): - return self._parent.alive + if not self._parent.alive: + return False + try: + self._msg_p + except errors.ObjectDestroyedError: + return False + else: + return True + + def __del__(self): + self._destroy() def _destroy(self): - pass + if self.alive: + capi.lib.notmuch_message_destroy(self._msg_p) + self._msg_p = None @property def messageid(self): @@ -363,30 +373,26 @@ class Message(base.NotmuchObject): if isinstance(other, self.__class__): return self.messageid == other.messageid -class StandaloneMessage(Message): - """An email message stored in the notmuch database. - This subclass of Message is used for messages that are retrieved from the - database directly and are not owned by a query. +class OwnedMessage(Message): + """An email message owned by parent thread object. + + This subclass of Message is used for messages that are retrieved + from the notmuch database via a parent :class:`notmuch2.Thread` + object, which "owns" this message. This means that when this + message object is destroyed, by calling :func:`del` or + :meth:`_destroy` directly or indirectly, the message is not freed + in the notmuch API and the parent :class:`notmuch2.Thread` object + can return the same object again when needed. """ + @property def alive(self): - if not self._parent.alive: - return False - try: - self._msg_p - except errors.ObjectDestroyedError: - return False - else: - return True - - def __del__(self): - self._destroy() + return self._parent.alive def _destroy(self): - if self.alive: - capi.lib.notmuch_message_destroy(self._msg_p) - self._msg_p = None + pass + class FilenamesIter(base.NotmuchIter): """Iterator for binary filenames objects.""" @@ -690,8 +696,9 @@ collections.abc.ValuesView.register(PropertiesValuesView) class MessageIter(base.NotmuchIter): - def __init__(self, parent, msgs_p, *, db): + def __init__(self, parent, msgs_p, *, db, msg_cls=Message): self._db = db + self._msg_cls = msg_cls super().__init__(parent, msgs_p, fn_destroy=capi.lib.notmuch_messages_destroy, fn_valid=capi.lib.notmuch_messages_valid, @@ -700,4 +707,4 @@ class MessageIter(base.NotmuchIter): def __next__(self): msg_p = super().__next__() - return Message(self, msg_p, db=self._db) + return self._msg_cls(self, msg_p, db=self._db) diff --git a/bindings/python-cffi/notmuch2/_thread.py b/bindings/python-cffi/notmuch2/_thread.py index bb76f2dc..e883f308 100644 --- a/bindings/python-cffi/notmuch2/_thread.py +++ b/bindings/python-cffi/notmuch2/_thread.py @@ -62,7 +62,9 @@ class Thread(base.NotmuchObject, collections.abc.Iterable): :raises ObjectDestroyedError: if used after destroyed. """ msgs_p = capi.lib.notmuch_thread_get_toplevel_messages(self._thread_p) - return message.MessageIter(self, msgs_p, db=self._db) + return message.MessageIter(self, msgs_p, + db=self._db, + msg_cls=message.OwnedMessage) def __iter__(self): """Return an iterator over all the messages in the thread. @@ -72,7 +74,9 @@ class Thread(base.NotmuchObject, collections.abc.Iterable): :raises ObjectDestroyedError: if used after destroyed. """ msgs_p = capi.lib.notmuch_thread_get_messages(self._thread_p) - return message.MessageIter(self, msgs_p, db=self._db) + return message.MessageIter(self, msgs_p, + db=self._db, + msg_cls=message.OwnedMessage) @property def matched(self): diff --git a/bindings/python-cffi/tests/test_database.py b/bindings/python-cffi/tests/test_database.py index e3a8344d..55069b5e 100644 --- a/bindings/python-cffi/tests/test_database.py +++ b/bindings/python-cffi/tests/test_database.py @@ -324,3 +324,14 @@ class TestQuery: threads = db.threads('*') thread = next(threads) assert isinstance(thread, notmuch2.Thread) + + def test_use_threaded_message_twice(self, db): + thread = next(db.threads('*')) + for msg in thread.toplevel(): + assert isinstance(msg, notmuch2.Message) + assert msg.alive + del msg + for msg in thread: + assert isinstance(msg, notmuch2.Message) + assert msg.alive + del msg