[RFC] Split notmuch_database_close into two functions
Austin Clements
amdragon at MIT.EDU
Thu Apr 12 09:57:44 PDT 2012
Quoth Justus Winter on Apr 12 at 11:05 am:
> Quoting Austin Clements (2012-04-01 05:23:23)
> >Quoth Justus Winter on Mar 21 at 1:55 am:
> >> I propose to split the function notmuch_database_close into
> >> notmuch_database_close and notmuch_database_destroy so that long
> >> running processes like alot can close the database while still using
> >> data obtained from queries to that database.
> >
> >Is this actually safe? My understanding of Xapian::Database::close is
> >that, once you've closed the database, basically anything can throw a
> >Xapian exception. A lot of data is retrieved lazily, both by notmuch
> >and by Xapian, so simply having, say, a notmuch_message_t object isn't
> >enough to guarantee that you'll be able to get data out of it after
> >closing the database. Hence, I don't see how this interface could be
> >used correctly.
>
> I do not know how, but both alot and afew (and occasionally the
> notmuch binary) are somehow safely using this interface on my box for
> the last three weeks.
I see. TL;DR: This isn't safe, but that's okay if we document it.
The bug report [0] you pointed to was quite informative. At its core,
this is really a memory management issue. To sum up for the record
(and to check my own thinking): It sounds like alot is careful not to
use any notmuch objects after closing the database. The problem is
that, currently, closing the database also talloc_free's it, which
recursively free's everything derived from it. Python later GCs the
wrapper objects, which *also* try to free their underlying objects,
resulting in a double free.
Before the change to expose notmuch_database_close, the Python
bindings would only talloc_free from destructors. Furthermore, they
prevented the library from recursively freeing things at other times
by internally maintaining a reverse reference for every library talloc
reference (e.g., message is a sub-allocation of query, so the bindings
keep a reference from each message to its query to ensure the query
doesn't get freed). The ability to explicitly talloc_free the
database subverts this mechanism.
So, I've come around to thinking that splitting notmuch_database_close
and _destroy is okay. It certainly parallels the rest of the API
better. However, notmuch_database_close needs a big warning similar
to Xapian::Database::close's warning that retrieving information from
objects derived from this database may not work after calling close.
notmuch_database_close is really a specialty interface, and about the
only thing you can guarantee after closing the database is that you
can destroy other objects. This is also going to require a SONAME
major version bump, as mentioned by others. Which, to be fair, would
be a good opportunity to fix some other issues, too, like how
notmuch_database_open can't return errors and how
notmuch_database_get_directory is broken on read-only databases. The
actual bump should be done at release time, but maybe we should drop a
note somewhere (NEWS?) so we don't forget.
[0] https://github.com/pazz/alot/issues/413
More information about the notmuch
mailing list