From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>,
	Justus Winter <4winter@informatik.uni-hamburg.de>
Subject: Re: [RFC] Split notmuch_database_close into two functions
In-Reply-To: <20120412165744.GF13549@mit.edu>
References:
 <1332291311-28954-1-git-send-email-4winter@informatik.uni-hamburg.de>
	<20120401032323.GH5949@mit.edu>
	<20120412090533.2074.78211@thinkbox.jade-hamburg.de>
	<20120412165744.GF13549@mit.edu>
User-Agent: Notmuch/0.12+110~gbc97b4a (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 17 Apr 2012 09:42:55 +0100
Message-ID: <87mx6a4uls.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: notmuch@notmuchmail.org
Precedence: list

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