Re: [PATCH] Actually close the xapian database in notmuch_database_close

[Austin Clements <amdragon@MIT.EDU>]

Quoth Justus Winter on Mar 01 at  7:59 am:
> Quoting Austin Clements (2012-02-29 23:17:54)
> >Quoth Olly Betts on Feb 29 at  9:19 pm:
> >> On Wed, Feb 29, 2012 at 10:48:33AM -0500, Austin Clements wrote:
> >> > Quoth Justus Winter on Feb 29 at 10:19 am:
> >> > > Formerly the xapian database object was deleted and closed in its
> >> > > destructor once the object was garbage collected. Explicitly call
> >> > > close() so that the database and the associated lock is released
> >> > > immediately.
> >> > 
> >> > Interesting.  Is this a bug in Xapian?  According to the docs,
> >> > ~Database is supposed to close the database (if there are no other
> >> > copies, which there shouldn't be), so this should be redundant with
> >> > the delete notmuch->xapian_db a few lines down, but your experience
> >> > obviously suggests that it isn't and I can't find the code path in
> >> > Xapian that would close it in the destructor.
> >> 
> >> Most Xapian API classes (including Database and WritableDatabase) just
> >> hold a reference-counted pointer, and so it's the destructor of the
> >> reference-counted object which closes the database.  If "PIMPL" means
> >> anything to you, that's what we have here.
> >> 
> >> Some other API classes objects (such as PostingIterator) internally hold
> >> a reference to the database they are using, so calling close()
> >> explicitly is useful if you don't want to have to worry about such
> >> objects still existing and holding onto references which keep the
> >> database open.
> >
> >Makes sense.  Justus, could you add a comment to your patch explaining
> >that we explicitly close the database because there may be other
> >objects with references to it that would keep it open?
> 
> I thought I did, I'm not a native speaker though, so if you want to
> reword my message be my guest ;)

Sorry, I meant a code comment.  Perhaps something like,

/* Many Xapian objects (and thus notmuch objects) hold references to
 * the database, so merely deleting the database may not suffice to
 * close it.  Thus, we explicitly close it here. */

> >Also, since close could throw an exception, it should get wrapped in a
> >try/catch like flush currently is.
> 
> My interpretation of [0] was that Xapian::Database::close() does not
> throw any exceptions.

Olly mentioned on IRC that it can throw an exception (because, for
example, close calls flush).

> Cheers,
> Justus
> 
> 0: http://xapian.org/docs/apidoc/html/classXapian_1_1Database.html#59f5f8b137723dcaaabdbdccbc0cf1eb