unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* python: Continuing message re-use fix
@ 2020-06-15 20:58 Floris Bruynooghe
  2020-06-15 20:58 ` [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query Floris Bruynooghe
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Floris Bruynooghe @ 2020-06-15 20:58 UTC (permalink / raw)
  To: notmuch

Hi,

This builds on the patch by Anton Khirnov to fix the message re-use
that is possible when accessing messages from a thread.  I started
with just addressing my own comments on this patch, but evolved it
into switching the logic around and leave the normal Message object
untouched.  Instead I created a new OwnedMessage which is used by
the Thread which does not free itself on __del__().  I think this
is preferable because the other iterators, mainly Database.messages(),
do not allow retrieving messages more than once since the query object
is hidden from the API.

I've left the original commit in this patch series to not alter any
contributions.

Cheers,
Floris


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query
  2020-06-15 20:58 python: Continuing message re-use fix Floris Bruynooghe
@ 2020-06-15 20:58 ` Floris Bruynooghe
  2020-06-15 20:58 ` [PATCH 2/2] Make messages returned by Thread objects owned Floris Bruynooghe
  2020-06-16 11:14 ` python: Continuing message re-use fix David Bremner
  2 siblings, 0 replies; 4+ messages in thread
From: Floris Bruynooghe @ 2020-06-15 20:58 UTC (permalink / raw)
  To: notmuch

From: Anton Khirnov <anton@khirnov.net>

Any messages retrieved from a query - either directly via
search_messages() or indirectly via thread objects - are owned by that
query. Retrieving the same message (i.e. corresponding to the same
message ID / database object) several times will always yield the same
C object.

The caller is allowed to destroy message objects owned by a query before
the query itself - which can save memory for long-lived queries.
However, that message must then never be retrieved again from that
query.

The python-notmuch2 bindings will currently destroy every message object
in Message._destroy(), which will lead to an invalid free if the same
message is then retrieved again. E.g. the following python program leads
to libtalloc abort()ing:

import notmuch2
db   = notmuch2.Database(mode = notmuch2.Database.MODE.READ_ONLY)
t    = next(db.threads('*'))
msgs = list(zip(t.toplevel(), t.toplevel()))
msgs = list(zip(t.toplevel(), t.toplevel()))

Fix this issue by creating a subclass of Message, which is used for
"standalone" message which have to be freed by the caller. Message class
is then used only for messages descended from a query, which do not need
to be freed by the caller.
---
 bindings/python-cffi/notmuch2/_database.py |  6 ++--
 bindings/python-cffi/notmuch2/_message.py  | 42 ++++++++++++++--------
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
index 95f59ca0..f14eac78 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -399,7 +399,7 @@ class Database(base.NotmuchObject):
               capi.lib.NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID]
         if ret not in ok:
             raise errors.NotmuchError(ret)
-        msg = message.Message(self, msg_pp[0], db=self)
+        msg = message.StandaloneMessage(self, msg_pp[0], db=self)
         if sync_flags:
             msg.tags.from_maildir_flags()
         return self.AddedMessage(
@@ -468,7 +468,7 @@ class Database(base.NotmuchObject):
         msg_p = msg_pp[0]
         if msg_p == capi.ffi.NULL:
             raise LookupError
-        msg = message.Message(self, msg_p, db=self)
+        msg = message.StandaloneMessage(self, msg_p, db=self)
         return msg
 
     def get(self, filename):
@@ -501,7 +501,7 @@ class Database(base.NotmuchObject):
         msg_p = msg_pp[0]
         if msg_p == capi.ffi.NULL:
             raise LookupError
-        msg = message.Message(self, msg_p, db=self)
+        msg = message.StandaloneMessage(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 c5fdbf6d..416ce7ca 100644
--- a/bindings/python-cffi/notmuch2/_message.py
+++ b/bindings/python-cffi/notmuch2/_message.py
@@ -14,7 +14,7 @@ __all__ = ['Message']
 
 
 class Message(base.NotmuchObject):
-    """An email message stored in the notmuch database.
+    """An email message stored in the notmuch database retrieved via a query.
 
     This should not be directly created, instead it will be returned
     by calling methods on :class:`Database`.  A message keeps a
@@ -61,22 +61,10 @@ class Message(base.NotmuchObject):
 
     @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
 
     @property
     def messageid(self):
@@ -375,6 +363,30 @@ 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.
+    """
+    @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()
+
+    def _destroy(self):
+        if self.alive:
+            capi.lib.notmuch_message_destroy(self._msg_p)
+        self._msg_p = None
 
 class FilenamesIter(base.NotmuchIter):
     """Iterator for binary filenames objects."""
-- 
2.27.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] Make messages returned by Thread objects owned
  2020-06-15 20:58 python: Continuing message re-use fix Floris Bruynooghe
  2020-06-15 20:58 ` [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query Floris Bruynooghe
@ 2020-06-15 20:58 ` Floris Bruynooghe
  2020-06-16 11:14 ` python: Continuing message re-use fix David Bremner
  2 siblings, 0 replies; 4+ messages in thread
From: Floris Bruynooghe @ 2020-06-15 20:58 UTC (permalink / raw)
  To: notmuch

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 f14eac78..95f59ca0 100644
--- a/bindings/python-cffi/notmuch2/_database.py
+++ b/bindings/python-cffi/notmuch2/_database.py
@@ -399,7 +399,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(
@@ -468,7 +468,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):
@@ -501,7 +501,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: <cdata>
-
     :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
-- 
2.27.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: python: Continuing message re-use fix
  2020-06-15 20:58 python: Continuing message re-use fix Floris Bruynooghe
  2020-06-15 20:58 ` [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query Floris Bruynooghe
  2020-06-15 20:58 ` [PATCH 2/2] Make messages returned by Thread objects owned Floris Bruynooghe
@ 2020-06-16 11:14 ` David Bremner
  2 siblings, 0 replies; 4+ messages in thread
From: David Bremner @ 2020-06-16 11:14 UTC (permalink / raw)
  To: Floris Bruynooghe, notmuch

Floris Bruynooghe <flub@devork.be> writes:

> Hi,
>
> This builds on the patch by Anton Khirnov to fix the message re-use
> that is possible when accessing messages from a thread.  I started
> with just addressing my own comments on this patch, but evolved it
> into switching the logic around and leave the normal Message object
> untouched.  Instead I created a new OwnedMessage which is used by
> the Thread which does not free itself on __del__().  I think this
> is preferable because the other iterators, mainly Database.messages(),
> do not allow retrieving messages more than once since the query object
> is hidden from the API.
>
> I've left the original commit in this patch series to not alter any
> contributions.
>
> Cheers,
> Floris

Revised series merged to master and release.

Thanks for your help with these updates.

d

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-06-16 11:15 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-15 20:58 python: Continuing message re-use fix Floris Bruynooghe
2020-06-15 20:58 ` [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query Floris Bruynooghe
2020-06-15 20:58 ` [PATCH 2/2] Make messages returned by Thread objects owned Floris Bruynooghe
2020-06-16 11:14 ` python: Continuing message re-use fix David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).