[RFC] Split notmuch_database_close into two functions

[RFC] Split notmuch_database_close into two functions

[Justus Winter]

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.

I've updated the tools, the go, ruby and python bindings to use
notmuch_database_destroy instead of notmuch_database_close to destroy
database objects.

Cheers,
Justus

[PATCH 1/7] Split notmuch_database_close into two functions

[Justus Winter]

Formerly notmuch_database_close closed the xapian database and
destroyed the talloc structure associated with the notmuch database
object. Split notmuch_database_close into notmuch_database_close and
notmuch_database_destroy.

This makes it possible for long running programs to close the xapian
database and thus release the lock associated with it without
destroying the data structures obtained from it.

This also makes the api more consistent since every other data
structure has a destructor function.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 lib/database.cc |   14 ++++++++++++--
 lib/notmuch.h   |   15 +++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 16c4354..2fefcad 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -642,7 +642,7 @@ notmuch_database_open (const char *path,
 			 "       read-write mode.\n",
 			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
 		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
-		notmuch_database_close (notmuch);
+		notmuch_database_destroy (notmuch);
 		notmuch = NULL;
 		goto DONE;
 	    }
@@ -702,7 +702,7 @@ notmuch_database_open (const char *path,
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
 		 error.get_msg().c_str());
-	notmuch_database_close (notmuch);
+	notmuch_database_destroy (notmuch);
 	notmuch = NULL;
     }
 
@@ -738,9 +738,19 @@ notmuch_database_close (notmuch_database_t *notmuch)
     }
 
     delete notmuch->term_gen;
+    notmuch->term_gen = NULL;
     delete notmuch->query_parser;
+    notmuch->query_parser = NULL;
     delete notmuch->xapian_db;
+    notmuch->xapian_db = NULL;
     delete notmuch->value_range_processor;
+    notmuch->value_range_processor = NULL;
+}
+
+void
+notmuch_database_destroy (notmuch_database_t *notmuch)
+{
+    notmuch_database_close (notmuch);
     talloc_free (notmuch);
 }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index babd208..6114f36 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -133,7 +133,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  *
  * After a successful call to notmuch_database_create, the returned
  * database will be open so the caller should call
- * notmuch_database_close when finished with it.
+ * notmuch_database_destroy when finished with it.
  *
  * The database will not yet have any data in it
  * (notmuch_database_create itself is a very cheap function). Messages
@@ -165,7 +165,7 @@ typedef enum {
  * An existing notmuch database can be identified by the presence of a
  * directory named ".notmuch" below 'path'.
  *
- * The caller should call notmuch_database_close when finished with
+ * The caller should call notmuch_database_destroy when finished with
  * this database.
  *
  * In case of any failure, this function returns NULL, (after printing
@@ -175,11 +175,18 @@ notmuch_database_t *
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode);
 
-/* Close the given notmuch database, freeing all associated
- * resources. See notmuch_database_open. */
+/* Close the given notmuch database.
+ *
+ * This function is called by notmuch_database_destroyed and can be
+ * called multiple times. */
 void
 notmuch_database_close (notmuch_database_t *database);
 
+/* Destroy the notmuch database freeing all associated
+ * resources */
+void
+notmuch_database_destroy (notmuch_database_t *database);
+
 /* Return the database path of the given database.
  *
  * The return value is a string owned by notmuch so should not be
-- 
1.7.9.1

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[Mark Walters]

Justus Winter <4winter@informatik.uni-hamburg.de> writes:

> Formerly notmuch_database_close closed the xapian database and
> destroyed the talloc structure associated with the notmuch database
> object. Split notmuch_database_close into notmuch_database_close and
> notmuch_database_destroy.
>
> This makes it possible for long running programs to close the xapian
> database and thus release the lock associated with it without
> destroying the data structures obtained from it.
>
> This also makes the api more consistent since every other data
> structure has a destructor function.

I like the idea of this series but have two queries before reviewing it.

The first is a concern that if we change the library functions we should
update the library version otherwise out of tree users won't know which
to call. (I don't actually know how versioning is done but I think we
should at least be able to make new out-of-tree code cope with the old
or new version). It might be worth keeping notmuch_database_close as it
is for now and adding something like notmuch_database_weak_close with
the new functionality. Then whenever the library version is next going
to get bumped we could move to the destroy/close nomenclature.

Secondly, I think the patch series could be made clearer and easier to
review. If you do it in three steps

1) change of notmuch_database_close to notmuch_database_destroy (just
   the function name change)
2) split the new notmuch_database_destroy into two as in the current
   first patch
3) Make any changes (if there are any) of notmuch_database_destroy to
   notmuch_database_close.

The advantage is that the first change is easy to test (essentially does
it build) and then changes from notmuch_database_destroy to
notmuch_database_close in step 3 are explicit rather than the current
situation where we need to grep the code to see if some instances of
notmuch_database_close were not changed to notmuch_database_destroy.

Of course if the decision is to go via the weak_close version then you
just need to do the analogues of 2 and 3.

Best wishes

Mark

>
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  lib/database.cc |   14 ++++++++++++--
>  lib/notmuch.h   |   15 +++++++++++----
>  2 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 16c4354..2fefcad 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -642,7 +642,7 @@ notmuch_database_open (const char *path,
>  			 "       read-write mode.\n",
>  			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
>  		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> -		notmuch_database_close (notmuch);
> +		notmuch_database_destroy (notmuch);
>  		notmuch = NULL;
>  		goto DONE;
>  	    }
> @@ -702,7 +702,7 @@ notmuch_database_open (const char *path,
>      } catch (const Xapian::Error &error) {
>  	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
>  		 error.get_msg().c_str());
> -	notmuch_database_close (notmuch);
> +	notmuch_database_destroy (notmuch);
>  	notmuch = NULL;
>      }
>  
> @@ -738,9 +738,19 @@ notmuch_database_close (notmuch_database_t *notmuch)
>      }
>  
>      delete notmuch->term_gen;
> +    notmuch->term_gen = NULL;
>      delete notmuch->query_parser;
> +    notmuch->query_parser = NULL;
>      delete notmuch->xapian_db;
> +    notmuch->xapian_db = NULL;
>      delete notmuch->value_range_processor;
> +    notmuch->value_range_processor = NULL;
> +}
> +
> +void
> +notmuch_database_destroy (notmuch_database_t *notmuch)
> +{
> +    notmuch_database_close (notmuch);
>      talloc_free (notmuch);
>  }
>  
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index babd208..6114f36 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -133,7 +133,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>   *
>   * After a successful call to notmuch_database_create, the returned
>   * database will be open so the caller should call
> - * notmuch_database_close when finished with it.
> + * notmuch_database_destroy when finished with it.
>   *
>   * The database will not yet have any data in it
>   * (notmuch_database_create itself is a very cheap function). Messages
> @@ -165,7 +165,7 @@ typedef enum {
>   * An existing notmuch database can be identified by the presence of a
>   * directory named ".notmuch" below 'path'.
>   *
> - * The caller should call notmuch_database_close when finished with
> + * The caller should call notmuch_database_destroy when finished with
>   * this database.
>   *
>   * In case of any failure, this function returns NULL, (after printing
> @@ -175,11 +175,18 @@ notmuch_database_t *
>  notmuch_database_open (const char *path,
>  		       notmuch_database_mode_t mode);
>  
> -/* Close the given notmuch database, freeing all associated
> - * resources. See notmuch_database_open. */
> +/* Close the given notmuch database.
> + *
> + * This function is called by notmuch_database_destroyed and can be
> + * called multiple times. */
>  void
>  notmuch_database_close (notmuch_database_t *database);
>  
> +/* Destroy the notmuch database freeing all associated
> + * resources */
> +void
> +notmuch_database_destroy (notmuch_database_t *database);
> +
>  /* Return the database path of the given database.
>   *
>   * The return value is a string owned by notmuch so should not be
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> The first is a concern that if we change the library functions we should
> update the library version otherwise out of tree users won't know which
> to call. 

This is not such a big deal. We can update the SONAME of the library so
that old code will continute to dynamically link to the old version of
the library. Nothing [1] will magically make the old code work with the
new library if we change the API; that is just the way things go,
occasionally APIs and/or ABIs change.

[1] Well maybe symbol versioning. I don't fully understand that, but I
suspect it is more trouble than it is worth.

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[Justus Winter]

Quoting Mark Walters (2012-03-31 19:17:15)
> Secondly, I think the patch series could be made clearer and easier to
> review. If you do it in three steps
> 
> 1) change of notmuch_database_close to notmuch_database_destroy (just
>    the function name change)
> 2) split the new notmuch_database_destroy into two as in the current
>    first patch
> 3) Make any changes (if there are any) of notmuch_database_destroy to
>    notmuch_database_close.
> 
> The advantage is that the first change is easy to test (essentially does
> it build) and then changes from notmuch_database_destroy to
> notmuch_database_close in step 3 are explicit rather than the current
> situation where we need to grep the code to see if some instances of
> notmuch_database_close were not changed to notmuch_database_destroy.

I don't buy it. The patch series first touches the library and
documentation and the lib compiles fine. The next patch updates the
cli tools, all of them compile fine afterwards.

Every patch addresses the issue component wise, this seems rather
natural for me.

Cheers,
Justus

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[Mark Walters]

On Mon, 16 Apr 2012, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Quoting Mark Walters (2012-03-31 19:17:15)
>> Secondly, I think the patch series could be made clearer and easier to
>> review. If you do it in three steps
>> 
>> 1) change of notmuch_database_close to notmuch_database_destroy (just
>>    the function name change)
>> 2) split the new notmuch_database_destroy into two as in the current
>>    first patch
>> 3) Make any changes (if there are any) of notmuch_database_destroy to
>>    notmuch_database_close.
>> 
>> The advantage is that the first change is easy to test (essentially does
>> it build) and then changes from notmuch_database_destroy to
>> notmuch_database_close in step 3 are explicit rather than the current
>> situation where we need to grep the code to see if some instances of
>> notmuch_database_close were not changed to notmuch_database_destroy.
>
> I don't buy it. The patch series first touches the library and
> documentation and the lib compiles fine. The next patch updates the
> cli tools, all of them compile fine afterwards.
>
> Every patch addresses the issue component wise, this seems rather
> natural for me.

I will try an explain my concern better. I assume that the patch
actually introduces a functional change : that is something somewhere in
the code calls the new notmuch_database_close instead of
notmuch_database_destroy [1]. In your current patch series someone
reading the patches alone can't see the functional change: it comes from
the occurrences of notmuch_database_close that you *don't* change to
notmuch_database_destroy.

Indeed, if the only change is to allow out-of-tree code access to the
new notmuch_database_close function then doing the patch series as

rename notmuch_database_close to notmuch_database_destroy

Then split notmuch_database_destroy make it clearer. (And if the code
compiles after step 1 then I know *all* occurrences of
notmuch_database_close have been changed to notmuch_database_destroy).


Best wishes

Mark

[1] Apart, of course, from notmuch_database_destroy.

[PATCH 2/7] NEWS: Document the notmuch_database_close split

[Justus Winter]

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

diff --git a/NEWS b/NEWS
index ed5e3c5..26fce67 100644
--- a/NEWS
+++ b/NEWS
@@ -52,6 +52,18 @@ Reply improvement using the JSON format
   reply body, and it will quote HTML parts if no text/plain parts are
   available.
 
+Library changes
+---------------
+
+API changes
+
+  The function notmuch_database_close has been split into
+  notmuch_database_close and notmuch_database_destroy.
+
+  This makes it possible for long running programs to close the xapian
+  database and thus release the lock associated with it without
+  destroying the data structures obtained from it.
+
 Notmuch 0.12 (2012-03-20)
 =========================
 
-- 
1.7.9.1

[PATCH 3/7] Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Adapt the notmuch binaries source to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 notmuch-count.c   |    2 +-
 notmuch-dump.c    |    2 +-
 notmuch-new.c     |    2 +-
 notmuch-reply.c   |    2 +-
 notmuch-restore.c |    2 +-
 notmuch-search.c  |    2 +-
 notmuch-show.c    |    2 +-
 notmuch-tag.c     |    2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/notmuch-count.c b/notmuch-count.c
index 46b76ae..ade9138 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -100,7 +100,7 @@ notmuch_count_command (void *ctx, int argc, char *argv[])
     }
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     return 0;
 }
diff --git a/notmuch-dump.c b/notmuch-dump.c
index a735875..71ab0ea 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -116,7 +116,7 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
 	fclose (output);
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     return 0;
 }
diff --git a/notmuch-new.c b/notmuch-new.c
index 4f13535..f078753 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -1017,7 +1017,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 		notmuch_status_to_string (ret));
     }
 
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     if (run_hooks && !ret && !interrupted)
 	ret = notmuch_run_hook (db_path, "post-new");
diff --git a/notmuch-reply.c b/notmuch-reply.c
index e2b6c25..1684b20 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -804,7 +804,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 	return 1;
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     if (params.cryptoctx)
 	g_object_unref(params.cryptoctx);
diff --git a/notmuch-restore.c b/notmuch-restore.c
index 87d9772..60efc52 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -184,7 +184,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     if (line)
 	free (line);
 
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
     if (input != stdin)
 	fclose (input);
 
diff --git a/notmuch-search.c b/notmuch-search.c
index f6061e4..0b41573 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -531,7 +531,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     }
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     return ret;
 }
diff --git a/notmuch-show.c b/notmuch-show.c
index ff9d427..c2eaa44 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1119,7 +1119,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
 
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     if (params.cryptoctx)
 	g_object_unref(params.cryptoctx);
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 36b9b09..142005c 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -227,7 +227,7 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     }
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     return interrupted;
 }
-- 
1.7.9.1

[PATCH 4/7] Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Adapt notmuch-deliver to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 contrib/notmuch-deliver/src/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/notmuch-deliver/src/main.c b/contrib/notmuch-deliver/src/main.c
index 6f32f73..37d2919 100644
--- a/contrib/notmuch-deliver/src/main.c
+++ b/contrib/notmuch-deliver/src/main.c
@@ -455,7 +455,7 @@ main(int argc, char **argv)
 	g_strfreev(opt_rtags);
 	g_free(mail);
 
-	notmuch_database_close(db);
+	notmuch_database_destroy(db);
 
 	return 0;
 }
-- 
1.7.9.1

[PATCH 5/7] go: Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Adapt the go bindings to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 bindings/go/pkg/notmuch.go |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/bindings/go/pkg/notmuch.go b/bindings/go/pkg/notmuch.go
index c6844ef..d32901d 100644
--- a/bindings/go/pkg/notmuch.go
+++ b/bindings/go/pkg/notmuch.go
@@ -114,7 +114,7 @@ func NewDatabase(path string) *Database {
  * An existing notmuch database can be identified by the presence of a
  * directory named ".notmuch" below 'path'.
  *
- * The caller should call notmuch_database_close when finished with
+ * The caller should call notmuch_database_destroy when finished with
  * this database.
  *
  * In case of any failure, this function returns NULL, (after printing
@@ -140,7 +140,7 @@ func OpenDatabase(path string, mode DatabaseMode) *Database {
 /* Close the given notmuch database, freeing all associated
  * resources. See notmuch_database_open. */
 func (self *Database) Close() {
-	C.notmuch_database_close(self.db)
+	C.notmuch_database_destroy(self.db)
 }
 
 /* Return the database path of the given database.
-- 
1.7.9.1

[PATCH 6/7] ruby: Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Adapt the ruby bindings to the notmuch_database_close split.

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

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 982fd59..ba9a139 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -110,7 +110,7 @@ notmuch_rb_database_close (VALUE self)
     notmuch_database_t *db;
 
     Data_Get_Notmuch_Database (self, db);
-    notmuch_database_close (db);
+    notmuch_database_destroy (db);
     DATA_PTR (self) = NULL;
 
     return Qnil;
-- 
1.7.9.1

[PATCH 7/7] python: wrap and use notmuch_database_destroy as destructor

[Justus Winter]

Adapt the go bindings to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 bindings/python/notmuch/database.py |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index 44d40fd..9a1896b 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -218,9 +218,22 @@ class Database(object):
     _close.restype = None
 
     def close(self):
-        """Close and free the notmuch database if needed"""
-        if self._db is not None:
+        '''
+        Closes the notmuch database.
+        '''
+        if self._db:
             self._close(self._db)
+
+    _destroy = nmlib.notmuch_database_destroy
+    _destroy.argtypes = [NotmuchDatabaseP]
+    _destroy.restype = None
+
+    def destroy(self):
+        '''
+        Destroys the notmuch database.
+        '''
+        if self._db:
+            self._destroy(self._db)
             self._db = None
 
     def __enter__(self):
-- 
1.7.9.1

Re: [PATCH 7/7] python: wrap and use notmuch_database_destroy as destructor

[Austin Clements]

Quoth Justus Winter on Mar 21 at  1:55 am:
> Adapt the go bindings to the notmuch_database_close split.

Typo.

> 
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  bindings/python/notmuch/database.py |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
> index 44d40fd..9a1896b 100644
> --- a/bindings/python/notmuch/database.py
> +++ b/bindings/python/notmuch/database.py
> @@ -218,9 +218,22 @@ class Database(object):
>      _close.restype = None
>  
>      def close(self):
> -        """Close and free the notmuch database if needed"""
> -        if self._db is not None:
> +        '''
> +        Closes the notmuch database.
> +        '''
> +        if self._db:
>              self._close(self._db)
> +
> +    _destroy = nmlib.notmuch_database_destroy
> +    _destroy.argtypes = [NotmuchDatabaseP]
> +    _destroy.restype = None
> +
> +    def destroy(self):

Should this be __del__?  The existing __del__ is certainly wrong with
this change, since it only closes the database and doesn't free it.  I
think this function is exactly what you want __del__ to be now.

(I think it also doesn't make sense to expose notmuch_database_destroy
as a general, public method since it will free all of the other C
objects out from under the bindings, resulting in exactly the double
free-type crashes that you're trying to avoid.  It appears that none
of the other Python classes have a destroy method.)

> +        '''
> +        Destroys the notmuch database.
> +        '''
> +        if self._db:
> +            self._destroy(self._db)
>              self._db = None
>  
>      def __enter__(self):

Re: [PATCH 7/7] python: wrap and use notmuch_database_destroy as destructor

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

Austin Clements <amdragon@MIT.EDU> writes:
> (I think it also doesn't make sense to expose notmuch_database_destroy
> as a general, public method since it will free all of the other C
> objects out from under the bindings, resulting in exactly the double
> free-type crashes that you're trying to avoid.  It appears that none
> of the other Python classes have a destroy method.)

Agreed, that sounds right to me.

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 7/7] python: wrap and use notmuch_database_destroy as destructor

[Justus Winter]

Quoting Austin Clements (2012-04-12 19:02:49)
> Quoth Justus Winter on Mar 21 at  1:55 am:
> > Adapt the go bindings to the notmuch_database_close split.
> 
> Typo.

Duh >,<

> > Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> > ---
> >  bindings/python/notmuch/database.py |   17 +++++++++++++++--
> >  1 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
> > index 44d40fd..9a1896b 100644
> > --- a/bindings/python/notmuch/database.py
> > +++ b/bindings/python/notmuch/database.py
> > @@ -218,9 +218,22 @@ class Database(object):
> >      _close.restype = None
> >  
> >      def close(self):
> > -        """Close and free the notmuch database if needed"""
> > -        if self._db is not None:
> > +        '''
> > +        Closes the notmuch database.
> > +        '''
> > +        if self._db:
> >              self._close(self._db)
> > +
> > +    _destroy = nmlib.notmuch_database_destroy
> > +    _destroy.argtypes = [NotmuchDatabaseP]
> > +    _destroy.restype = None
> > +
> > +    def destroy(self):
> 
> Should this be __del__?  The existing __del__ is certainly wrong with
> this change, since it only closes the database and doesn't free it.  I
> think this function is exactly what you want __del__ to be now.
> 
> (I think it also doesn't make sense to expose notmuch_database_destroy
> as a general, public method since it will free all of the other C
> objects out from under the bindings, resulting in exactly the double
> free-type crashes that you're trying to avoid.  It appears that none
> of the other Python classes have a destroy method.)

Yes, of course...

I'm going to send an rebased and updated patch series as a follow up
to this message. Thanks for the input everyone :)

Justus

[PATCH 1/7] Split notmuch_database_close into two functions

[Justus Winter]

Formerly notmuch_database_close closed the xapian database and
destroyed the talloc structure associated with the notmuch database
object. Split notmuch_database_close into notmuch_database_close and
notmuch_database_destroy.

This makes it possible for long running programs to close the xapian
database and thus release the lock associated with it without
destroying the data structures obtained from it.

This also makes the api more consistent since every other data
structure has a destructor function.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 lib/database.cc |   14 ++++++++++++--
 lib/notmuch.h   |   15 +++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 16c4354..2fefcad 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -642,7 +642,7 @@ notmuch_database_open (const char *path,
 			 "       read-write mode.\n",
 			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
 		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
-		notmuch_database_close (notmuch);
+		notmuch_database_destroy (notmuch);
 		notmuch = NULL;
 		goto DONE;
 	    }
@@ -702,7 +702,7 @@ notmuch_database_open (const char *path,
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
 		 error.get_msg().c_str());
-	notmuch_database_close (notmuch);
+	notmuch_database_destroy (notmuch);
 	notmuch = NULL;
     }
 
@@ -738,9 +738,19 @@ notmuch_database_close (notmuch_database_t *notmuch)
     }
 
     delete notmuch->term_gen;
+    notmuch->term_gen = NULL;
     delete notmuch->query_parser;
+    notmuch->query_parser = NULL;
     delete notmuch->xapian_db;
+    notmuch->xapian_db = NULL;
     delete notmuch->value_range_processor;
+    notmuch->value_range_processor = NULL;
+}
+
+void
+notmuch_database_destroy (notmuch_database_t *notmuch)
+{
+    notmuch_database_close (notmuch);
     talloc_free (notmuch);
 }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 673c423..84c9265 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -133,7 +133,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  *
  * After a successful call to notmuch_database_create, the returned
  * database will be open so the caller should call
- * notmuch_database_close when finished with it.
+ * notmuch_database_destroy when finished with it.
  *
  * The database will not yet have any data in it
  * (notmuch_database_create itself is a very cheap function). Messages
@@ -165,7 +165,7 @@ typedef enum {
  * An existing notmuch database can be identified by the presence of a
  * directory named ".notmuch" below 'path'.
  *
- * The caller should call notmuch_database_close when finished with
+ * The caller should call notmuch_database_destroy when finished with
  * this database.
  *
  * In case of any failure, this function returns NULL, (after printing
@@ -175,11 +175,18 @@ notmuch_database_t *
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode);
 
-/* Close the given notmuch database, freeing all associated
- * resources. See notmuch_database_open. */
+/* Close the given notmuch database.
+ *
+ * This function is called by notmuch_database_destroy and can be
+ * called multiple times. */
 void
 notmuch_database_close (notmuch_database_t *database);
 
+/* Destroy the notmuch database freeing all associated
+ * resources */
+void
+notmuch_database_destroy (notmuch_database_t *database);
+
 /* Return the database path of the given database.
  *
  * The return value is a string owned by notmuch so should not be
-- 
1.7.10

[PATCH 2/7] NEWS: Document the notmuch_database_close split

[Justus Winter]

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 NEWS |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/NEWS b/NEWS
index c1704e0..a2cd080 100644
--- a/NEWS
+++ b/NEWS
@@ -73,6 +73,18 @@ leaving Mutt.  notmuch-mutt, formerly distributed under the name
 "mutt-notmuch" by Stefano Zacchiroli, will be maintained as a notmuch
 contrib/ from now on.
 
+Library changes
+---------------
+
+API changes
+
+  The function notmuch_database_close has been split into
+  notmuch_database_close and notmuch_database_destroy.
+
+  This makes it possible for long running programs to close the xapian
+  database and thus release the lock associated with it without
+  destroying the data structures obtained from it.
+
 Notmuch 0.12 (2012-03-20)
 =========================
 
-- 
1.7.10

Re: [PATCH 2/7] NEWS: Document the notmuch_database_close split

[Felipe Contreras]

On Sun, Apr 22, 2012 at 3:07 PM, Justus Winter
<4winter@informatik.uni-hamburg.de> wrote:
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  NEWS |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index c1704e0..a2cd080 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -73,6 +73,18 @@ leaving Mutt.  notmuch-mutt, formerly distributed under the name
>  "mutt-notmuch" by Stefano Zacchiroli, will be maintained as a notmuch
>  contrib/ from now on.
>
> +Library changes
> +---------------
> +
> +API changes
> +
> +  The function notmuch_database_close has been split into
> +  notmuch_database_close and notmuch_database_destroy.
> +
> +  This makes it possible for long running programs to close the xapian
> +  database and thus release the lock associated with it without
> +  destroying the data structures obtained from it.
> +

I haven't following this change. Who can an application take advantage
of this? I call _close(), now what do I do to use the database again?

Cheers.

-- 
Felipe Contreras

[PATCH 3/7] Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Adapt the notmuch binaries source to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 notmuch-count.c   |    2 +-
 notmuch-dump.c    |    2 +-
 notmuch-new.c     |    2 +-
 notmuch-reply.c   |    2 +-
 notmuch-restore.c |    2 +-
 notmuch-search.c  |    2 +-
 notmuch-show.c    |    2 +-
 notmuch-tag.c     |    2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/notmuch-count.c b/notmuch-count.c
index b76690c..9c2ad7b 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -107,7 +107,7 @@ notmuch_count_command (void *ctx, int argc, char *argv[])
     }
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     return 0;
 }
diff --git a/notmuch-dump.c b/notmuch-dump.c
index a735875..71ab0ea 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -116,7 +116,7 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
 	fclose (output);
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     return 0;
 }
diff --git a/notmuch-new.c b/notmuch-new.c
index 4f13535..f078753 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -1017,7 +1017,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 		notmuch_status_to_string (ret));
     }
 
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     if (run_hooks && !ret && !interrupted)
 	ret = notmuch_run_hook (db_path, "post-new");
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 0949d9f..da99a13 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -755,7 +755,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 	return 1;
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     if (params.cryptoctx)
 	g_object_unref(params.cryptoctx);
diff --git a/notmuch-restore.c b/notmuch-restore.c
index d3b9246..02b563c 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -192,7 +192,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     if (line)
 	free (line);
 
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
     if (input != stdin)
 	fclose (input);
 
diff --git a/notmuch-search.c b/notmuch-search.c
index 1cc8430..7dfd270 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -545,7 +545,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
     }
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     return ret;
 }
diff --git a/notmuch-show.c b/notmuch-show.c
index da4a797..3b6667c 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1117,7 +1117,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
     }
 
     notmuch_query_destroy (query);
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     if (params.cryptoctx)
 	g_object_unref(params.cryptoctx);
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 05feed3..bd56fd1 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -238,7 +238,7 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
 
     ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
 
-    notmuch_database_close (notmuch);
+    notmuch_database_destroy (notmuch);
 
     return ret;
 }
-- 
1.7.10

[PATCH 4/7] Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Adapt notmuch-deliver to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 contrib/notmuch-deliver/src/main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/notmuch-deliver/src/main.c b/contrib/notmuch-deliver/src/main.c
index 6f32f73..37d2919 100644
--- a/contrib/notmuch-deliver/src/main.c
+++ b/contrib/notmuch-deliver/src/main.c
@@ -455,7 +455,7 @@ main(int argc, char **argv)
 	g_strfreev(opt_rtags);
 	g_free(mail);
 
-	notmuch_database_close(db);
+	notmuch_database_destroy(db);
 
 	return 0;
 }
-- 
1.7.10

[PATCH 5/7] go: Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Adapt the go bindings to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 bindings/go/pkg/notmuch.go |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/bindings/go/pkg/notmuch.go b/bindings/go/pkg/notmuch.go
index c6844ef..d32901d 100644
--- a/bindings/go/pkg/notmuch.go
+++ b/bindings/go/pkg/notmuch.go
@@ -114,7 +114,7 @@ func NewDatabase(path string) *Database {
  * An existing notmuch database can be identified by the presence of a
  * directory named ".notmuch" below 'path'.
  *
- * The caller should call notmuch_database_close when finished with
+ * The caller should call notmuch_database_destroy when finished with
  * this database.
  *
  * In case of any failure, this function returns NULL, (after printing
@@ -140,7 +140,7 @@ func OpenDatabase(path string, mode DatabaseMode) *Database {
 /* Close the given notmuch database, freeing all associated
  * resources. See notmuch_database_open. */
 func (self *Database) Close() {
-	C.notmuch_database_close(self.db)
+	C.notmuch_database_destroy(self.db)
 }
 
 /* Return the database path of the given database.
-- 
1.7.10

[PATCH 6/7] ruby: Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Adapt the ruby bindings to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 bindings/ruby/database.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 982fd59..ba9a139 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -110,7 +110,7 @@ notmuch_rb_database_close (VALUE self)
     notmuch_database_t *db;
 
     Data_Get_Notmuch_Database (self, db);
-    notmuch_database_close (db);
+    notmuch_database_destroy (db);
     DATA_PTR (self) = NULL;
 
     return Qnil;
-- 
1.7.10

Re: [PATCH 6/7] ruby: Use notmuch_database_destroy instead of notmuch_database_close

[Felipe Contreras]

Hi,

On Sun, Apr 22, 2012 at 3:07 PM, Justus Winter
<4winter@informatik.uni-hamburg.de> wrote:
> Adapt the ruby bindings to the notmuch_database_close split.
>
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  bindings/ruby/database.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
> index 982fd59..ba9a139 100644
> --- a/bindings/ruby/database.c
> +++ b/bindings/ruby/database.c
> @@ -110,7 +110,7 @@ notmuch_rb_database_close (VALUE self)
>     notmuch_database_t *db;
>
>     Data_Get_Notmuch_Database (self, db);
> -    notmuch_database_close (db);
> +    notmuch_database_destroy (db);
>     DATA_PTR (self) = NULL;
>
>     return Qnil;
> --

I don't think this is the right approach. If database_destroy truly
destroys the object, then we would want to do it only at garbage
collection, when it's not accessible any more. What if I want to
re-use the database from the Ruby code?

This would probably be better:

-- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -20,10 +20,16 @@

 #include "defs.h"

+static void
+database_free (void *p)
+{
+    notmuch_database_destroy (p);
+}
+
 VALUE
 notmuch_rb_database_alloc (VALUE klass)
 {
-    return Data_Wrap_Struct (klass, NULL, NULL, NULL);
+    return Data_Wrap_Struct (klass, NULL, database_free, NULL);
 }

 /*

-- 
Felipe Contreras

Re: [PATCH 6/7] ruby: Use notmuch_database_destroy instead of notmuch_database_close

[Justus Winter]

Quoting Felipe Contreras (2012-04-23 14:36:33)
> I don't think this is the right approach. If database_destroy truly
> destroys the object, then we would want to do it only at garbage
> collection, when it's not accessible any more. What if I want to
> re-use the database from the Ruby code?
> 
> This would probably be better:
> 
>[...]

You're probably right, I don't know the ruby bindings at all, I just
wanted to preserve the old behavior. You are welcome to refine the
ruby bindings later (or maintain them, I *believe* they are
unmaintained, the last change was back in october 2011), but let's get
this patch series in first.

Cheers,
Justus

Re: [PATCH 6/7] ruby: Use notmuch_database_destroy instead of notmuch_database_close

[Austin Clements]

On Mon, 23 Apr 2012, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Quoting Felipe Contreras (2012-04-23 14:36:33)
>> I don't think this is the right approach. If database_destroy truly
>> destroys the object, then we would want to do it only at garbage
>> collection, when it's not accessible any more. What if I want to
>> re-use the database from the Ruby code?
>> 
>> This would probably be better:
>> 
>>[...]
>
> You're probably right, I don't know the ruby bindings at all, I just
> wanted to preserve the old behavior. You are welcome to refine the
> ruby bindings later (or maintain them, I *believe* they are
> unmaintained, the last change was back in october 2011), but let's get
> this patch series in first.

I just marked this series ready, but I wasn't clear on what the
conclusion here was.  Feel free to mark it moreinfo again if it's not
actually ready (maybe mark just this patch?  it's a bit confusing since
the patches aren't quite all together.)

[PATCH 7/7] python: wrap and use notmuch_database_destroy as destructor

[Justus Winter]

Adapt the python bindings to the notmuch_database_close split.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 bindings/python/notmuch/database.py |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index 44d40fd..268e952 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -161,8 +161,13 @@ class Database(object):
         else:
             self.create(path)
 
+    _destroy = nmlib.notmuch_database_destroy
+    _destroy.argtypes = [NotmuchDatabaseP]
+    _destroy.restype = None
+
     def __del__(self):
-        self.close()
+        if self._db:
+            self._destroy(self._db)
 
     def _assert_db_is_initialized(self):
         """Raises :exc:`NotInitializedError` if self._db is `None`"""
@@ -218,10 +223,11 @@ class Database(object):
     _close.restype = None
 
     def close(self):
-        """Close and free the notmuch database if needed"""
-        if self._db is not None:
+        '''
+        Closes the notmuch database.
+        '''
+        if self._db:
             self._close(self._db)
-            self._db = None
 
     def __enter__(self):
         '''
-- 
1.7.10

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[Austin Clements]

Quoth Justus Winter on Apr 22 at  2:07 pm:
> Formerly notmuch_database_close closed the xapian database and
> destroyed the talloc structure associated with the notmuch database
> object. Split notmuch_database_close into notmuch_database_close and
> notmuch_database_destroy.
> 
> This makes it possible for long running programs to close the xapian
> database and thus release the lock associated with it without
> destroying the data structures obtained from it.
> 
> This also makes the api more consistent since every other data
> structure has a destructor function.
> 
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
> ---
>  lib/database.cc |   14 ++++++++++++--
>  lib/notmuch.h   |   15 +++++++++++----
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/database.cc b/lib/database.cc
> index 16c4354..2fefcad 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -642,7 +642,7 @@ notmuch_database_open (const char *path,
>  			 "       read-write mode.\n",
>  			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
>  		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> -		notmuch_database_close (notmuch);
> +		notmuch_database_destroy (notmuch);
>  		notmuch = NULL;
>  		goto DONE;
>  	    }
> @@ -702,7 +702,7 @@ notmuch_database_open (const char *path,
>      } catch (const Xapian::Error &error) {
>  	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
>  		 error.get_msg().c_str());
> -	notmuch_database_close (notmuch);
> +	notmuch_database_destroy (notmuch);
>  	notmuch = NULL;
>      }
>  
> @@ -738,9 +738,19 @@ notmuch_database_close (notmuch_database_t *notmuch)
>      }
>  
>      delete notmuch->term_gen;
> +    notmuch->term_gen = NULL;
>      delete notmuch->query_parser;
> +    notmuch->query_parser = NULL;
>      delete notmuch->xapian_db;
> +    notmuch->xapian_db = NULL;
>      delete notmuch->value_range_processor;
> +    notmuch->value_range_processor = NULL;
> +}
> +
> +void
> +notmuch_database_destroy (notmuch_database_t *notmuch)
> +{
> +    notmuch_database_close (notmuch);
>      talloc_free (notmuch);
>  }
>  
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 673c423..84c9265 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -133,7 +133,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>   *
>   * After a successful call to notmuch_database_create, the returned
>   * database will be open so the caller should call
> - * notmuch_database_close when finished with it.
> + * notmuch_database_destroy when finished with it.
>   *
>   * The database will not yet have any data in it
>   * (notmuch_database_create itself is a very cheap function). Messages
> @@ -165,7 +165,7 @@ typedef enum {
>   * An existing notmuch database can be identified by the presence of a
>   * directory named ".notmuch" below 'path'.
>   *
> - * The caller should call notmuch_database_close when finished with
> + * The caller should call notmuch_database_destroy when finished with
>   * this database.
>   *
>   * In case of any failure, this function returns NULL, (after printing
> @@ -175,11 +175,18 @@ notmuch_database_t *
>  notmuch_database_open (const char *path,
>  		       notmuch_database_mode_t mode);
>  
> -/* Close the given notmuch database, freeing all associated
> - * resources. See notmuch_database_open. */
> +/* Close the given notmuch database.
> + *
> + * This function is called by notmuch_database_destroy and can be
> + * called multiple times. */

This needs a comment explaining the implications of closing the
database.  Perhaps something like this (modeled on
Xapian::Database::close's documentation)

/* Close the given notmuch database.
 *
 * After notmuch_database_close has been called, calls to other
 * functions on objects derived from this database may either behave
 * as if the database had not been closed (e.g., if the required data
 * has been cached) or may fail with a
 * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
 *
 * notmuch_database_close can be called multiple times.  Later calls
 * have no affect. 
 */

>  void
>  notmuch_database_close (notmuch_database_t *database);
>  
> +/* Destroy the notmuch database freeing all associated
> + * resources */

This should mention that this also closes the database (currently you
mention this in the doc for notmuch_database_close, but it's a reverse
reference there; that could probably even be omitted).  Perhaps

/* Destroy the notmuch database, closing it if necessary and freeing
 * all associated resources. */

> +void
> +notmuch_database_destroy (notmuch_database_t *database);
> +
>  /* Return the database path of the given database.
>   *
>   * The return value is a string owned by notmuch so should not be

[PATCH 1/7] Split notmuch_database_close into two functions

[Justus Winter]

Formerly notmuch_database_close closed the xapian database and
destroyed the talloc structure associated with the notmuch database
object. Split notmuch_database_close into notmuch_database_close and
notmuch_database_destroy.

This makes it possible for long running programs to close the xapian
database and thus release the lock associated with it without
destroying the data structures obtained from it.

This also makes the api more consistent since every other data
structure has a destructor function.

The comments in notmuch.h are a courtesy of Austin Clements.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 lib/database.cc |   14 ++++++++++++--
 lib/notmuch.h   |   22 ++++++++++++++++++----
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 16c4354..2fefcad 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -642,7 +642,7 @@ notmuch_database_open (const char *path,
 			 "       read-write mode.\n",
 			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
 		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
-		notmuch_database_close (notmuch);
+		notmuch_database_destroy (notmuch);
 		notmuch = NULL;
 		goto DONE;
 	    }
@@ -702,7 +702,7 @@ notmuch_database_open (const char *path,
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
 		 error.get_msg().c_str());
-	notmuch_database_close (notmuch);
+	notmuch_database_destroy (notmuch);
 	notmuch = NULL;
     }
 
@@ -738,9 +738,19 @@ notmuch_database_close (notmuch_database_t *notmuch)
     }
 
     delete notmuch->term_gen;
+    notmuch->term_gen = NULL;
     delete notmuch->query_parser;
+    notmuch->query_parser = NULL;
     delete notmuch->xapian_db;
+    notmuch->xapian_db = NULL;
     delete notmuch->value_range_processor;
+    notmuch->value_range_processor = NULL;
+}
+
+void
+notmuch_database_destroy (notmuch_database_t *notmuch)
+{
+    notmuch_database_close (notmuch);
     talloc_free (notmuch);
 }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 673c423..7d9e092 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -133,7 +133,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  *
  * After a successful call to notmuch_database_create, the returned
  * database will be open so the caller should call
- * notmuch_database_close when finished with it.
+ * notmuch_database_destroy when finished with it.
  *
  * The database will not yet have any data in it
  * (notmuch_database_create itself is a very cheap function). Messages
@@ -165,7 +165,7 @@ typedef enum {
  * An existing notmuch database can be identified by the presence of a
  * directory named ".notmuch" below 'path'.
  *
- * The caller should call notmuch_database_close when finished with
+ * The caller should call notmuch_database_destroy when finished with
  * this database.
  *
  * In case of any failure, this function returns NULL, (after printing
@@ -175,11 +175,25 @@ notmuch_database_t *
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode);
 
-/* Close the given notmuch database, freeing all associated
- * resources. See notmuch_database_open. */
+/* Close the given notmuch database.
+ *
+ * After notmuch_database_close has been called, calls to other
+ * functions on objects derived from this database may either behave
+ * as if the database had not been closed (e.g., if the required data
+ * has been cached) or may fail with a
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
+ *
+ * notmuch_database_close can be called multiple times.  Later calls
+ * have no effect.
+ */
 void
 notmuch_database_close (notmuch_database_t *database);
 
+/* Destroy the notmuch database, closing it if necessary and freeing
+* all associated resources. */
+void
+notmuch_database_destroy (notmuch_database_t *database);
+
 /* Return the database path of the given database.
  *
  * The return value is a string owned by notmuch so should not be
-- 
1.7.10

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[Austin Clements]

LGTM.

On Wed, 25 Apr 2012, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Formerly notmuch_database_close closed the xapian database and
> destroyed the talloc structure associated with the notmuch database
> object. Split notmuch_database_close into notmuch_database_close and
> notmuch_database_destroy.
>
> This makes it possible for long running programs to close the xapian
> database and thus release the lock associated with it without
> destroying the data structures obtained from it.
>
> This also makes the api more consistent since every other data
> structure has a destructor function.
>
> The comments in notmuch.h are a courtesy of Austin Clements.
>
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[David Bremner]

Justus Winter <4winter@informatik.uni-hamburg.de> writes:

> Formerly notmuch_database_close closed the xapian database and
> destroyed the talloc structure associated with the notmuch database
> object. Split notmuch_database_close into notmuch_database_close and
> notmuch_database_destroy.

Series pushed.

d

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[Austin Clements]

On Sun, 22 Apr 2012, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
> Formerly notmuch_database_close closed the xapian database and
> destroyed the talloc structure associated with the notmuch database
> object. Split notmuch_database_close into notmuch_database_close and
> notmuch_database_destroy.
>
> This makes it possible for long running programs to close the xapian
> database and thus release the lock associated with it without
> destroying the data structures obtained from it.
>
> This also makes the api more consistent since every other data
> structure has a destructor function.
>
> Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>

Other than my comments comment, this series LGTM.  I think adding those
comments should also address Felipe's question.

Re: [RFC] Split notmuch_database_close into two functions

[Patrick Totzke]

Hi all,

I can confirm that this fixes a rather nasty core dump in a branch of alot
that closes and re-opens the database more frequently.
/p

Re: [RFC] Split notmuch_database_close into two functions

[Tomi Ollila]

Justus Winter <4winter@informatik.uni-hamburg.de> writes:

> 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.
>
> I've updated the tools, the go, ruby and python bindings to use
> notmuch_database_destroy instead of notmuch_database_close to destroy
> database objects.

This looks like a good idea. grep _destroy *.c outputs plenty of
other matches so this is not a new term here. I wonder what (backward)
compability issues there are, though.

In message id:"1332291311-28954-2-git-send-email-4winter@informatik.uni-hamburg.de"
there was typo in comment: notmuch_database_destroyed ?

>
> Cheers,
> Justus

Tomi

Re: [RFC] Split notmuch_database_close into two functions

[Justus Winter]

You're right of course, updated patch sent as a follow up.

Justus

[PATCH 1/7] Split notmuch_database_close into two functions

[Justus Winter]

Formerly notmuch_database_close closed the xapian database and
destroyed the talloc structure associated with the notmuch database
object. Split notmuch_database_close into notmuch_database_close and
notmuch_database_destroy.

This makes it possible for long running programs to close the xapian
database and thus release the lock associated with it without
destroying the data structures obtained from it.

This also makes the api more consistent since every other data
structure has a destructor function.

Signed-off-by: Justus Winter <4winter@informatik.uni-hamburg.de>
---
 lib/database.cc |   14 ++++++++++++--
 lib/notmuch.h   |   15 +++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 16c4354..2fefcad 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -642,7 +642,7 @@ notmuch_database_open (const char *path,
 			 "       read-write mode.\n",
 			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
 		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
-		notmuch_database_close (notmuch);
+		notmuch_database_destroy (notmuch);
 		notmuch = NULL;
 		goto DONE;
 	    }
@@ -702,7 +702,7 @@ notmuch_database_open (const char *path,
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
 		 error.get_msg().c_str());
-	notmuch_database_close (notmuch);
+	notmuch_database_destroy (notmuch);
 	notmuch = NULL;
     }
 
@@ -738,9 +738,19 @@ notmuch_database_close (notmuch_database_t *notmuch)
     }
 
     delete notmuch->term_gen;
+    notmuch->term_gen = NULL;
     delete notmuch->query_parser;
+    notmuch->query_parser = NULL;
     delete notmuch->xapian_db;
+    notmuch->xapian_db = NULL;
     delete notmuch->value_range_processor;
+    notmuch->value_range_processor = NULL;
+}
+
+void
+notmuch_database_destroy (notmuch_database_t *notmuch)
+{
+    notmuch_database_close (notmuch);
     talloc_free (notmuch);
 }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index babd208..2fb4e70 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -133,7 +133,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  *
  * After a successful call to notmuch_database_create, the returned
  * database will be open so the caller should call
- * notmuch_database_close when finished with it.
+ * notmuch_database_destroy when finished with it.
  *
  * The database will not yet have any data in it
  * (notmuch_database_create itself is a very cheap function). Messages
@@ -165,7 +165,7 @@ typedef enum {
  * An existing notmuch database can be identified by the presence of a
  * directory named ".notmuch" below 'path'.
  *
- * The caller should call notmuch_database_close when finished with
+ * The caller should call notmuch_database_destroy when finished with
  * this database.
  *
  * In case of any failure, this function returns NULL, (after printing
@@ -175,11 +175,18 @@ notmuch_database_t *
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode);
 
-/* Close the given notmuch database, freeing all associated
- * resources. See notmuch_database_open. */
+/* Close the given notmuch database.
+ *
+ * This function is called by notmuch_database_destroy and can be
+ * called multiple times. */
 void
 notmuch_database_close (notmuch_database_t *database);
 
+/* Destroy the notmuch database freeing all associated
+ * resources */
+void
+notmuch_database_destroy (notmuch_database_t *database);
+
 /* Return the database path of the given database.
  *
  * The return value is a string owned by notmuch so should not be
-- 
1.7.9.1

Re: [RFC] Split notmuch_database_close into two functions

[Austin Clements]

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.

Maybe you could describe your use case in more detail?

Re: [RFC] Split notmuch_database_close into two functions

[Justus Winter]

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.

>Maybe you could describe your use case in more detail?

Uh, okay. There are two long running processes (alot and afew in my
case) competing for a resource ("writable access to the notmuch
database"). This access is serialized by a lock maintained by
libxapian. This is a rather classic cs problem.

In order to avoid starvation of one process, both processes must
release the lock after a finite amount of time. Now for all practical
purposes the requirement is usually that both processes should
minimize the time spent while they aquired the lock.

The only way to ensure that the lock is released is to ask notmuch to
release the lock. When Patrick asked me for a way to release the lock
I exposed the function notmuch_database_close [0].

I made a mistake. The mistake was to assume that a function named
notmuch_database_close would actually do as the name implies. But that
wasn't the case. I should have been more suspicious, but being
somewhat naive I patched notmuch_database_close to actually close the
database [1].

I'm basically trying to correct that mistake. One way to fix that is
this patchset, the other one is to rename notmuch_database_close to
notmuch_database_destroy and to remove the ability to call this
function from the python bindings.

There is a branch of alot [2] that needs this functionality and hence
this patchset to avoid crashes (the ticket contains more information
why this leads to crashes). Incidentally this branch also fixes
another bug in alot [3]. Patrick spoke up in this thread stating that
this patchset is useful for alot. There is a unpublished branch of
afew that requires this functionality.

I think it very much boils down to the question whether or not
libnotmuch and its users are second class citizens. This issue is not
a problem for the notmuch binary since it is short lived in nature.

In fact I feel reminded of [4] when I pointed out problems in the code
that are problematic for libnotmuch users. You - Austin - suggested to
whip up patches instead of raising concerns. That's a valid response,
after all, talk is cheap ;)

But now that I have a patchset that is rather small (yes, it changes
the API but any program can be adjusted automatically using sed...)
and I kind of thought that it would be accepted more easily. And
although the changes are trivial it took me quite some time to track
it down.

The thread [4] dried out without someone like you or David stating
that he cares about libnotmuch and its users as much as the users of
the notmuch binary. These kind of problems require invasive code and
API changes, I asked in that very same thread how to do this properly
but noone answered.

I think I'm just not willing to devote my time to fix these problems
if these issues aren't even perceived as problems by the majority of
notmuch users and developers b/c they are using the emacs interface
which just calls the notmuch binary that has no such problems.

Cheers,
Justus

0: b2734519db78fdec76eeafc5fe8f5631a6436cf6
1: cfc5f1059aa16753cba610c41601cacc97260e08
2: https://github.com/pazz/alot/issues/413
3: https://github.com/pazz/alot/issues/414
4: 20120221002921.8534.57091@thinkbox.jade-hamburg.de

Re: [RFC] Split notmuch_database_close into two functions

[Austin Clements]

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

Re: [RFC] Split notmuch_database_close into two functions

[Justus Winter]

Quoting Austin Clements (2012-04-12 18:57:44)
>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.

Exactly.

>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.

Yes, but then again one should always expect function calls to fail
and most APIs have mechanisms to communicate failures.

OTOH this might be an indication that the notmuch API should be
redesigned. Both alot and afew have their own wrappers around the
notmuch API to work around some limitations (e.g. changes to messages
are enqueued and executed at some point, with some kind of mechanism
to cope with the notmuch database temporarily not being available,
message objects have to be re-fetched if they got outdated (IIRC,
whatever that means)).

Justus

Re: [RFC] Split notmuch_database_close into two functions

[Justus Winter]

Hi everyone,

Quoting Patrick Totzke (2012-04-13 10:33:58)
> Quoting Austin Clements (2012-04-01 04:23:23)
> >Maybe you could describe your use case in more detail?
> 
> Quoting Austin Clements (2012-04-12 17:57:44)
> >Quoth Justus Winter on Apr 12 at 11:05 am:
> ...
> >I see.  TL;DR
> 
> .. which should pretty much settle Austins opinion on
> libnotmuch users being second class citizens.

Na, I think you misinterpreted Austin here, I think he summarized his
position. Looking back at my mail I think I came across a lot harsher
than necessary, I'm sorry for that. Let's get back to the issue at
hand, shall we?

Both Austin and Mark seem to support the split, any other opinions?

Cheers,
Justus

Re: [RFC] Split notmuch_database_close into two functions

[Tomi Ollila]

On Tue, Apr 17 2012, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:

> Hi everyone,
>
> Quoting Patrick Totzke (2012-04-13 10:33:58)
>> Quoting Austin Clements (2012-04-01 04:23:23)
>> >Maybe you could describe your use case in more detail?
>> 
>> Quoting Austin Clements (2012-04-12 17:57:44)
>> >Quoth Justus Winter on Apr 12 at 11:05 am:
>> ...
>> >I see.  TL;DR
>> 
>> .. which should pretty much settle Austins opinion on
>> libnotmuch users being second class citizens.
>
> Na, I think you misinterpreted Austin here, I think he summarized his
> position. Looking back at my mail I think I came across a lot harsher
> than necessary, I'm sorry for that. Let's get back to the issue at
> hand, shall we?
>
> Both Austin and Mark seem to support the split, any other opinions?

I support it too (as did earlyer). Do we already have pushable series
or is there a need for a new one ? It would be good to have this done
so that application writers can start relying this being available
in "official" notmuch (in 0.13 -- SONAME updated -- any other patches
pending requiring SONAME bump to be reviewed ?).

>
> Cheers,
> Justus

Tomi

Re: [RFC] Split notmuch_database_close into two functions

[Mark Walters]

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

Re: [RFC] Split notmuch_database_close into two functions

[Austin Clements]

Quoth Mark Walters on Apr 17 at  9:42 am:
> 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.

That would be handy and perfect for this situation, but no (I
double-checked with Olly on IRC, which you probably saw).  We might be
able to lobby for this capability if it seems more generally useful.
On the other hand, I think it would probably mix poorly with Xapian's
optimistic snapshot isolation if we tried to use it for anything
non-trivial (combined with real snapshot isolation it would be
awesome).