Date: Thu, 12 Apr 2012 12:57:44 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Justus Winter <4winter@informatik.uni-hamburg.de>
Subject: Re: [RFC] Split notmuch_database_close into two functions
Message-ID: <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20120412090533.2074.78211@thinkbox.jade-hamburg.de>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

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