* 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ messages in thread
* [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query @ 2020-05-09 5:05 Anton Khirnov 2020-05-21 7:08 ` Anton Khirnov 2020-05-21 22:17 ` Floris Bruynooghe 0 siblings, 2 replies; 9+ messages in thread From: Anton Khirnov @ 2020-05-09 5:05 UTC (permalink / raw) To: notmuch 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.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query 2020-05-09 5:05 [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query Anton Khirnov @ 2020-05-21 7:08 ` Anton Khirnov 2020-05-21 11:13 ` David Bremner 2020-06-10 21:02 ` Tomi Ollila 2020-05-21 22:17 ` Floris Bruynooghe 1 sibling, 2 replies; 9+ messages in thread From: Anton Khirnov @ 2020-05-21 7:08 UTC (permalink / raw) To: notmuch ping -- Anton Khirnov ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query 2020-05-21 7:08 ` Anton Khirnov @ 2020-05-21 11:13 ` David Bremner 2020-06-10 21:02 ` Tomi Ollila 1 sibling, 0 replies; 9+ messages in thread From: David Bremner @ 2020-05-21 11:13 UTC (permalink / raw) To: Anton Khirnov, notmuch Anton Khirnov <anton@khirnov.net> writes: > ping I'm hoping Floris will find a chance to review your patches. d ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query 2020-05-21 7:08 ` Anton Khirnov 2020-05-21 11:13 ` David Bremner @ 2020-06-10 21:02 ` Tomi Ollila 1 sibling, 0 replies; 9+ messages in thread From: Tomi Ollila @ 2020-06-10 21:02 UTC (permalink / raw) To: Anton Khirnov, notmuch Hi Anton, On Thu, May 21 2020, Anton Khirnov wrote: > ping Great (an important) stuff in this series ! ...just that... Floris did good review on your changes the next day. What I remember was there (looked 30 mins ago) - style: somewhat important (I press more there). Easy to get better, good and consistent style makes it better - docs in code: like Floris mentioned, the docs in commit messages are are not seen by user. The relevant documentation is to be with the code. commit message tells what was done and why - tests: since pytest is used to test the current notmuch2 cffi python tests, all new code that can be tested pytest code to do so is to be added And these have to be done now -- postponing such a things into future will just mean that it never happens, and the messines and entropy of the notmuch code gets even greater in such a case... > > -- > Anton Khirnov Tomi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query 2020-05-09 5:05 [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query Anton Khirnov 2020-05-21 7:08 ` Anton Khirnov @ 2020-05-21 22:17 ` Floris Bruynooghe 1 sibling, 0 replies; 9+ messages in thread From: Floris Bruynooghe @ 2020-05-21 22:17 UTC (permalink / raw) To: Anton Khirnov, notmuch Hi Anton, Thanks for improving the bindings! Any my apologies for the late response, I failed to spot this mail the first time round. Also, this is a pretty serious bug, thanks for finding it. This looks pretty solid, a few small style comments that aren't very important notwithstanding. Though I do think you should be able to add a test for this in the pytest tests. I think just triggering the use-after-free you demonstrate in the commit message is fine as it will work with your patch. The fact it will crash rather then fail without is unfortunate but probably fine. On Sat 09 May 2020 at 07:05 +0200, Anton Khirnov wrote: > 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 I feel like some more description from the commit message could live here to explain why this isn't destroying the message rather than having to find it in the commit message. > > @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): Minor style thing, between top-level code one usually leaves 2 blank lines per PEP8. Nowadays perhaps we should think about adopting an automatic formatter but hey. (This applies to lots of places in both patches btw) > + """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. > + """ Likewise I'd be tempted to add here that this message is destroyed as soon as it is garbage collected pointing out that this is not the case for Message. And probably update the Message docstring with the inverse. > + @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.""" ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-16 11:15 UTC | newest] Thread overview: 9+ 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 -- strict thread matches above, loose matches on Subject: below -- 2020-05-09 5:05 [PATCH 1/2] python/notmuch2: do not destroy messages owned by a query Anton Khirnov 2020-05-21 7:08 ` Anton Khirnov 2020-05-21 11:13 ` David Bremner 2020-06-10 21:02 ` Tomi Ollila 2020-05-21 22:17 ` Floris Bruynooghe
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).