Re: [RFC] Split notmuch_database_close into two functions

[Mark Walters <markwalters1009@gmail.com>]

On Thu, 12 Apr 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> 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.

Can I just check that there is no way to reopen the Xapian database
readonly? (I may be using the wrong term: I mean is there a way of
switching an open read-write database to read-only without losing the
attached structures/messages/threads etc) If I understand it this would
be sufficient as it would free the lock, but could be more generally
useful for long lived notmuch processes.

Best wishes

Mark