Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
To: Austin Clements <amdragon@MIT.EDU>, 
From: Justus Winter <4winter@informatik.uni-hamburg.de>
In-Reply-To: <20120229221754.GD772@mit.edu>
References:
 <1330507157-22859-1-git-send-email-4winter@informatik.uni-hamburg.de>
	<1330507157-22859-2-git-send-email-4winter@informatik.uni-hamburg.de>
	<20120229154833.GB772@mit.edu> <20120229211922.GQ24964@survex.com>
	<20120229221754.GD772@mit.edu>
Message-ID: <20120301065930.5076.72748@thinkbox.jade-hamburg.de>
User-Agent: alot/0.21+
Subject: Re: [PATCH] Actually close the xapian database in
	notmuch_database_close
Date: Thu, 01 Mar 2012 07:59:30 +0100
Cc: Olly Betts <olly@survex.com>, notmuch@notmuchmail.org
Precedence: list

Hi :)

Olly wrote:
>It is hard to say if calling close() is actually useful here from just
>seeing the patch.

Huh? I provided a test case...

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 ;)

>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.

Cheers,
Justus

0: http://xapian.org/docs/apidoc/html/classXapian_1_1Database.html#59f5f8b1=
37723dcaaabdbdccbc0cf1eb