fix notmuch_database_close

fix notmuch_database_close

[Justus Winter]

Pazz mentioned a problem wrt reopening a notmuch database immediately
after it has been closed. The problem can be reproduced with this test
case:

~~~ snip ~~~
import os
import notmuch

db_path = os.path.expanduser('~/Maildir')

for i in range(2):
    with notmuch.Database(db_path, mode=notmuch.Database.MODE.READ_WRITE) as db:
        query = db.create_query('tag:inbox AND NOT tag:killed')
        print(len(list(query.search_messages())))
~~~ snap ~~~

Without this fix, the second loop iteration fails because the xapian
lock is not released in time.

Cheers,
Justus

[PATCH] Actually close the xapian database in notmuch_database_close

[Justus Winter]

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.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 lib/database.cc |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 5efa85e..79cf375 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -726,6 +726,10 @@ notmuch_database_close (notmuch_database_t *notmuch)
 	}
     }
 
+    if (notmuch->xapian_db != NULL) {
+	notmuch->xapian_db->close();
+    }
+
     delete notmuch->term_gen;
     delete notmuch->query_parser;
     delete notmuch->xapian_db;
-- 
1.7.9

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

[Austin Clements]

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.

Olly?

> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  lib/database.cc |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/database.cc b/lib/database.cc
> index 5efa85e..79cf375 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -726,6 +726,10 @@ notmuch_database_close (notmuch_database_t *notmuch)
>  	}
>      }
>  
> +    if (notmuch->xapian_db != NULL) {
> +	notmuch->xapian_db->close();
> +    }
> +
>      delete notmuch->term_gen;
>      delete notmuch->query_parser;
>      delete notmuch->xapian_db;

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

[Olly Betts]

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.

The main motivation for adding close() was the bindings though - e.g. in
Python the wrapped Database object gets destroyed when the GC gets run,
which is at some essentially arbitrary time after you remove the last
reference to it.

It is hard to say if calling close() is actually useful here from just
seeing the patch.

Cheers,
    Olly

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

[Austin Clements]

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?

Also, since close could throw an exception, it should get wrapped in a
try/catch like flush currently is.

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

[Justus Winter]

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#59f5f8b137723dcaaabdbdccbc0cf1eb

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

[Austin Clements]

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

[PATCH] Actually close the xapian database in notmuch_database_close

[Justus Winter]

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.

The comment is a courtesy of Austin Clements.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 lib/database.cc |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 5efa85e..8f8df1a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -726,6 +726,17 @@ notmuch_database_close (notmuch_database_t *notmuch)
 	}
     }
 
+    /* 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. */
+    if (notmuch->xapian_db != NULL) {
+	try {
+	    notmuch->xapian_db->close();
+	} catch (const Xapian::Error &error) {
+	    /* do nothing */
+	}
+    }
+
     delete notmuch->term_gen;
     delete notmuch->query_parser;
     delete notmuch->xapian_db;
-- 
1.7.9.1

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

[David Bremner]

On Fri,  2 Mar 2012 15:58:39 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> 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.

pushed.

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

[Olly Betts]

On Thu, Mar 01, 2012 at 07:59:30AM +0100, Justus Winter wrote:
> 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...

I only saw the part of the patch Austin quoted in the mail he cc-ed to
me.

> Quoting Austin Clements (2012-02-29 23:17:54)
> >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.

Sadly there's not full documentation of exceptions which can be thrown 
by a particular method.

Cheers,
    Olly