[PATCH] Use the Xapian::DB_RETRY_LOCK flag when available

[PATCH] Use the Xapian::DB_RETRY_LOCK flag when available

[Istvan Marko]

Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug
275). Detect it in configure and use it if available. With this flag
commands that need the write lock will wait for their turn instead of
aborting when it's not immediately available.
---
 configure       | 25 ++++++++++++++++++++++++-
 lib/database.cc |  5 +++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 6231d2b..0c1d5bc 100755
--- a/configure
+++ b/configure
@@ -371,6 +371,21 @@ if [ ${have_xapian} = "1" ]; then
     esac
 fi
 
+# DB_RETRY_LOCK is only supported on Xapian > 1.3.2
+have_xapian_db_retry_lock=0
+if [ ${have_xapian} = "1" ]; then
+    printf "Checking for Xapian lock retry support... "
+    case "${xapian_version}" in
+	0.*|1.[012].*|1.3.[0-2])
+	    printf "No (only available with Xapian > 1.3.2).\n" ;;
+	[1-9]*.[0-9]*.[0-9]*)
+	    have_xapian_db_retry_lock=1
+	    printf "Yes.\n" ;;
+	*)
+	    printf "Unknown version.\n" ;;
+    esac
+fi
+
 default_xapian_backend=""
 if [ ${have_xapian} = "1" ]; then
     printf "Testing default Xapian backend... "
@@ -998,6 +1013,9 @@ HAVE_D_TYPE = ${have_d_type}
 # Whether the Xapian version in use supports compaction
 HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
 
+# Whether the Xapian version in use supports DB_RETRY_LOCK
+HAVE_XAPIAN_DB_RETRY_LOCK = ${have_xapian_db_retry_lock}
+
 # Whether the getpwuid_r function is standards-compliant
 # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
 # to enable the standards-compliant version -- needed for Solaris)
@@ -1072,6 +1090,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
 		   -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\
 		   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)	 \\
+		   -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\
 		   -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
 
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
@@ -1086,6 +1105,7 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
 		     -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\
 		     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)       \\
+                     -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\
 		     -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
 
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(ZLIB_LDFLAGS) \$(XAPIAN_LDFLAGS)
@@ -1097,7 +1117,10 @@ cat > sh.config <<EOF
 # script of notmuch.
 
 # Whether the Xapian version in use supports compaction
-NOTMUCH_HAVE_XAPIAN_COMPACT=${have_xapian_compact}
+NOTMUCH_HAVE_XAPIAN_  =${have_xapian_compact}
+
+# Whether the Xapian version in use supports DB_RETRY_LOCK
+NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${have_xapian_db_retry_lock}
 
 # Which backend will Xapian use by default?
 NOTMUCH_DEFAULT_XAPIAN_BACKEND=${default_xapian_backend}
diff --git a/lib/database.cc b/lib/database.cc
index c8c5e26..4b503a2 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -929,8 +929,13 @@ notmuch_database_open_verbose (const char *path,
 	string last_mod;
 
 	if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) {
+	    #if HAVE_XAPIAN_DB_RETRY_LOCK
+	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
+							       Xapian::DB_CREATE_OR_OPEN|Xapian::DB_RETRY_LOCK);
+	    #else
 	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
 							       Xapian::DB_CREATE_OR_OPEN);
+	    #endif
 	} else {
 	    notmuch->xapian_db = new Xapian::Database (xapian_path);
 	}
-- 
2.4.10


-- 
	Istvan

Re: [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available

[Jani Nikula]

On Tue, 03 May 2016, Istvan Marko <notmuch@kismala.com> wrote:
> Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug
> 275). Detect it in configure and use it if available. With this flag
> commands that need the write lock will wait for their turn instead of
> aborting when it's not immediately available.
> ---
>  configure       | 25 ++++++++++++++++++++++++-
>  lib/database.cc |  5 +++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 6231d2b..0c1d5bc 100755
> --- a/configure
> +++ b/configure
> @@ -371,6 +371,21 @@ if [ ${have_xapian} = "1" ]; then
>      esac
>  fi
>  
> +# DB_RETRY_LOCK is only supported on Xapian > 1.3.2
> +have_xapian_db_retry_lock=0
> +if [ ${have_xapian} = "1" ]; then
> +    printf "Checking for Xapian lock retry support... "
> +    case "${xapian_version}" in
> +	0.*|1.[012].*|1.3.[0-2])
> +	    printf "No (only available with Xapian > 1.3.2).\n" ;;
> +	[1-9]*.[0-9]*.[0-9]*)
> +	    have_xapian_db_retry_lock=1
> +	    printf "Yes.\n" ;;
> +	*)
> +	    printf "Unknown version.\n" ;;
> +    esac
> +fi
> +
>  default_xapian_backend=""
>  if [ ${have_xapian} = "1" ]; then
>      printf "Testing default Xapian backend... "
> @@ -998,6 +1013,9 @@ HAVE_D_TYPE = ${have_d_type}
>  # Whether the Xapian version in use supports compaction
>  HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
>  
> +# Whether the Xapian version in use supports DB_RETRY_LOCK
> +HAVE_XAPIAN_DB_RETRY_LOCK = ${have_xapian_db_retry_lock}
> +
>  # Whether the getpwuid_r function is standards-compliant
>  # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
>  # to enable the standards-compliant version -- needed for Solaris)
> @@ -1072,6 +1090,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
>  		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
>  		   -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\
>  		   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)	 \\
> +		   -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\
>  		   -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
>  
>  CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
> @@ -1086,6 +1105,7 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
>  		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
>  		     -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\
>  		     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)       \\
> +                     -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\
>  		     -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
>  
>  CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(ZLIB_LDFLAGS) \$(XAPIAN_LDFLAGS)
> @@ -1097,7 +1117,10 @@ cat > sh.config <<EOF
>  # script of notmuch.
>  
>  # Whether the Xapian version in use supports compaction
> -NOTMUCH_HAVE_XAPIAN_COMPACT=${have_xapian_compact}
> +NOTMUCH_HAVE_XAPIAN_  =${have_xapian_compact}

Ooops?

> +
> +# Whether the Xapian version in use supports DB_RETRY_LOCK
> +NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${have_xapian_db_retry_lock}

You don't need to update sh.config unless you need this in a shell
script, typically in the tests. Which brings us to the question, how can
you test this?

>  
>  # Which backend will Xapian use by default?
>  NOTMUCH_DEFAULT_XAPIAN_BACKEND=${default_xapian_backend}
> diff --git a/lib/database.cc b/lib/database.cc
> index c8c5e26..4b503a2 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -929,8 +929,13 @@ notmuch_database_open_verbose (const char *path,
>  	string last_mod;
>  
>  	if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) {
> +	    #if HAVE_XAPIAN_DB_RETRY_LOCK
> +	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
> +							       Xapian::DB_CREATE_OR_OPEN|Xapian::DB_RETRY_LOCK);
> +	    #else
>  	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
>  							       Xapian::DB_CREATE_OR_OPEN);
> +	    #endif

Please no #ifdefs like this in the middle of the code.

A better alternative is to define something like this above the
function:

#if HAVE_XAPIAN_DB_RETRY_LOCK
#define DB_ACTION	(Xapian::DB_CREATE_OR_OPEN | Xapian::DB_RETRY_LOCK)
#else
#define DB_ACTION	Xapian::DB_CREATE_OR_OPEN
#endif

and use that in new Xapian::WritableDatabase().


BR,
Jani.


>  	} else {
>  	    notmuch->xapian_db = new Xapian::Database (xapian_path);
>  	}
> -- 
> 2.4.10
>
>
> -- 
> 	Istvan
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available

[David Bremner]

Istvan Marko <notmuch@kismala.com> writes:

> Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug
> 275). Detect it in configure and use it if available. With this flag
> commands that need the write lock will wait for their turn instead of
> aborting when it's not immediately available.
> ---
>  configure       | 25 ++++++++++++++++++++++++-
>  lib/database.cc |  5 +++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 6231d2b..0c1d5bc 100755
> --- a/configure
> +++ b/configure
> @@ -371,6 +371,21 @@ if [ ${have_xapian} = "1" ]; then
>      esac
>  fi
>  
> +# DB_RETRY_LOCK is only supported on Xapian > 1.3.2
> +have_xapian_db_retry_lock=0
> +if [ ${have_xapian} = "1" ]; then
> +    printf "Checking for Xapian lock retry support... "
> +    case "${xapian_version}" in
> +	0.*|1.[012].*|1.3.[0-2])
> +	    printf "No (only available with Xapian > 1.3.2).\n" ;;
> +	[1-9]*.[0-9]*.[0-9]*)
> +	    have_xapian_db_retry_lock=1
> +	    printf "Yes.\n" ;;
> +	*)
> +	    printf "Unknown version.\n" ;;
> +    esac
> +fi

First, thanks for writing the patch. I'd completely forgotten about this
feature, and it's something pretty important for many notmuch users. I
saw you got some good feedback from Jani. As a less urgent comment, what
about writing the test in the style of

      http://article.gmane.org/gmane.mail.notmuch.general/22329

I don't know if there's complete concensus on this, but I personally
much prefer testing features to versions when possible.

d

[PATCH v2] Use the Xapian::DB_RETRY_LOCK flag when available

[Istvan Marko]

Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug
275). Detect it in configure and use it if available. With this flag
commands that need the write lock will wait for their turn instead of
aborting when it's not immediately available.
---

Updated based on Jani's feedback.

Regarding tests, I couldn't think of a meaningful test for this. We get
different behaviors depending on the Xapian version and if we try do
some kind of a concurrent locking test we'd really be just testing
Xapian's locking code rather than notmuch.

 configure       | 20 ++++++++++++++++++++
 lib/database.cc |  8 +++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 6231d2b..8ef96b4 100755
--- a/configure
+++ b/configure
@@ -371,6 +371,21 @@ if [ ${have_xapian} = "1" ]; then
     esac
 fi
 
+# DB_RETRY_LOCK is only supported on Xapian > 1.3.2
+have_xapian_db_retry_lock=0
+if [ ${have_xapian} = "1" ]; then
+    printf "Checking for Xapian lock retry support... "
+    case "${xapian_version}" in
+	0.*|1.[012].*|1.3.[0-2])
+	    printf "No (only available with Xapian > 1.3.2).\n" ;;
+	[1-9]*.[0-9]*.[0-9]*)
+	    have_xapian_db_retry_lock=1
+	    printf "Yes.\n" ;;
+	*)
+	    printf "Unknown version.\n" ;;
+    esac
+fi
+
 default_xapian_backend=""
 if [ ${have_xapian} = "1" ]; then
     printf "Testing default Xapian backend... "
@@ -998,6 +1013,9 @@ HAVE_D_TYPE = ${have_d_type}
 # Whether the Xapian version in use supports compaction
 HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
 
+# Whether the Xapian version in use supports DB_RETRY_LOCK
+HAVE_XAPIAN_DB_RETRY_LOCK = ${have_xapian_db_retry_lock}
+
 # Whether the getpwuid_r function is standards-compliant
 # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
 # to enable the standards-compliant version -- needed for Solaris)
@@ -1072,6 +1090,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
 		   -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\
 		   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)	 \\
+		   -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\
 		   -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
 
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
@@ -1086,6 +1105,7 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
 		     -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\
 		     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)       \\
+                     -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\
 		     -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
 
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(ZLIB_LDFLAGS) \$(XAPIAN_LDFLAGS)
diff --git a/lib/database.cc b/lib/database.cc
index c8c5e26..c0fcb29 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -48,6 +48,12 @@ typedef struct {
 #define STRINGIFY(s) _SUB_STRINGIFY(s)
 #define _SUB_STRINGIFY(s) #s
 
+#if HAVE_XAPIAN_DB_RETRY_LOCK
+#define DB_ACTION (Xapian::DB_CREATE_OR_OPEN | Xapian::DB_RETRY_LOCK)
+#else
+#define DB_ACTION Xapian::DB_CREATE_OR_OPEN
+#endif
+
 /* Here's the current schema for our database (for NOTMUCH_DATABASE_VERSION):
  *
  * We currently have three different types of documents (mail, ghost,
@@ -930,7 +936,7 @@ notmuch_database_open_verbose (const char *path,
 
 	if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) {
 	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
-							       Xapian::DB_CREATE_OR_OPEN);
+							       DB_ACTION);
 	} else {
 	    notmuch->xapian_db = new Xapian::Database (xapian_path);
 	}
-- 
2.4.10


-- 
	Istvan

Re: [PATCH v2] Use the Xapian::DB_RETRY_LOCK flag when available

[David Bremner]

Istvan Marko <notmuch@kismala.com> writes:

> Regarding tests, I couldn't think of a meaningful test for this. We get
> different behaviors depending on the Xapian version and if we try do
> some kind of a concurrent locking test we'd really be just testing
> Xapian's locking code rather than notmuch.

We would be testing the build/configuration part to make sure we are
enabling the right flags, but more importantly testing the error
handling code, since we have to catch exceptions and pass them back
through the C library interface as error codes. One thing that would
make this easier to test is adding some finer grain errors than
"NOTMUCH_STATUS_XAPIAN_EXCEPTION" for notmuch_database_open_verbose to
return, to distinguish between the case of immediate failure and
timeout.  Although it's not clear to me that Xapian actually provides a
timeout when DB_RETRY_LOCK is used.

v3 of DB_RETRY_LOCK

[David Bremner]

[PATCH 1/4] Use the Xapian::DB_RETRY_LOCK flag when available

This is the original patch, rebased on master. I took the liberty of
converting the configure test to a test compile.

[PATCH 2/4] test: factor out some boilerplate from C tests
[PATCH 3/4] test: initial tests for locking retry

Add a simple test of locking retry.

[PATCH 4/4] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK

Make it visible to the user whether their notmuch supports locking
retry (blocking open).

The remaining question for me is if we are happy with defaulting to
blocking (w/o timeout) on open. I'm not sure how you could deadlock
without writing C code, but it's pretty easy with fork. As I
discovered writing my test case.

[PATCH 1/4] Use the Xapian::DB_RETRY_LOCK flag when available

[David Bremner]

From: Istvan Marko <notmuch@kismala.com>

Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug
275). Detect it in configure and use it if available. With this flag
commands that need the write lock will wait for their turn instead of
aborting when it's not immediately available.
---
 configure       | 20 ++++++++++++++++++++
 lib/database.cc |  8 +++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index ce1d698..e9de9c6 100755
--- a/configure
+++ b/configure
@@ -392,6 +392,22 @@ EOF
     rm -f _field_processor.o _field_processor.cc
 
     default_xapian_backend=""
+    # DB_RETRY_LOCK is only supported on Xapian > 1.3.2
+    have_xapian_db_retry_lock=0
+    printf "Checking for Xapian lock retry support... "
+    cat>_retry.cc<<EOF
+#include <xapian.h>
+int flag = Xapian::DB_RETRY_LOCK;
+EOF
+    if ${CXX} ${CXXFLAGS_for_sh} ${xapian_cxxflags} -c _retry.cc -o _retry.o > /dev/null 2>&1
+    then
+	have_xapian_db_retry_lock=1
+	printf "Yes.\n"
+    else
+	printf "No. (optional)\n"
+    fi
+    rm -f _retry.o _retry.cc
+
     printf "Testing default Xapian backend... "
     cat >_default_backend.cc <<EOF
 #include <xapian.h>
@@ -1022,6 +1038,9 @@ HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
 # Whether the Xapian version in use supports field processors
 HAVE_XAPIAN_FIELD_PROCESSOR = ${have_xapian_field_processor}
 
+# Whether the Xapian version in use supports DB_RETRY_LOCK
+HAVE_XAPIAN_DB_RETRY_LOCK = ${have_xapian_db_retry_lock}
+
 # Whether the getpwuid_r function is standards-compliant
 # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
 # to enable the standards-compliant version -- needed for Solaris)
@@ -1097,6 +1116,7 @@ COMMON_CONFIGURE_CFLAGS = \\
 	-DSTD_ASCTIME=\$(STD_ASCTIME)				\\
 	-DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)		\\
 	-DHAVE_XAPIAN_FIELD_PROCESSOR=\$(HAVE_XAPIAN_FIELD_PROCESSOR) \\
+	-DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\
 	-DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
 
 CONFIGURE_CFLAGS = \$(COMMON_CONFIGURE_CFLAGS)
diff --git a/lib/database.cc b/lib/database.cc
index 9630000..e4f3ab5 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -49,6 +49,12 @@ typedef struct {
 #define STRINGIFY(s) _SUB_STRINGIFY(s)
 #define _SUB_STRINGIFY(s) #s
 
+#if HAVE_XAPIAN_DB_RETRY_LOCK
+#define DB_ACTION (Xapian::DB_CREATE_OR_OPEN | Xapian::DB_RETRY_LOCK)
+#else
+#define DB_ACTION Xapian::DB_CREATE_OR_OPEN
+#endif
+
 /* Here's the current schema for our database (for NOTMUCH_DATABASE_VERSION):
  *
  * We currently have three different types of documents (mail, ghost,
@@ -931,7 +937,7 @@ notmuch_database_open_verbose (const char *path,
 
 	if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) {
 	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
-							       Xapian::DB_CREATE_OR_OPEN);
+							       DB_ACTION);
 	} else {
 	    notmuch->xapian_db = new Xapian::Database (xapian_path);
 	}
-- 
2.8.1

[PATCH 2/4] test: factor out some boilerplate from C tests

[David Bremner]

The trick of having a common header file doesn't work to share between
test scripts, so make an include file in the test directory.
---
 test/T590-libconfig.sh | 39 ++++++++++++++-------------------------
 test/notmuch-test.h    | 15 +++++++++++++++
 test/test-lib.sh       |  2 +-
 3 files changed, 30 insertions(+), 26 deletions(-)
 create mode 100644 test/notmuch-test.h

diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh
index 9c1e566..e8c078d 100755
--- a/test/T590-libconfig.sh
+++ b/test/T590-libconfig.sh
@@ -6,20 +6,9 @@ test_description="library config API"
 add_email_corpus
 
 cat <<EOF > c_head
-#include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
-#include <notmuch.h>
-
-void run(int line, notmuch_status_t ret)
-{
-   if (ret) {
-	fprintf (stderr, "line %d: %s\n", line, ret);
-	exit (1);
-   }
-}
-
-#define RUN(v)  run(__LINE__, v);
+#include <notmuch-test.h>
 
 int main (int argc, char** argv)
 {
@@ -27,23 +16,23 @@ int main (int argc, char** argv)
    char *val;
    notmuch_status_t stat;
 
-   RUN(notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db));
+   EXPECT0(notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db));
 
 EOF
 
 cat <<EOF > c_tail
-   RUN(notmuch_database_destroy(db));
+   EXPECT0(notmuch_database_destroy(db));
 }
 EOF
 
 test_begin_subtest "notmuch_database_{set,get}_config"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
-   RUN(notmuch_database_set_config (db, "testkey1", "testvalue1"));
-   RUN(notmuch_database_set_config (db, "testkey2", "testvalue2"));
-   RUN(notmuch_database_get_config (db, "testkey1", &val));
+   EXPECT0(notmuch_database_set_config (db, "testkey1", "testvalue1"));
+   EXPECT0(notmuch_database_set_config (db, "testkey2", "testvalue2"));
+   EXPECT0(notmuch_database_get_config (db, "testkey1", &val));
    printf("testkey1 = %s\n", val);
-   RUN(notmuch_database_get_config (db, "testkey2", &val));
+   EXPECT0(notmuch_database_get_config (db, "testkey2", &val));
    printf("testkey2 = %s\n", val);
 }
 EOF
@@ -60,7 +49,7 @@ test_begin_subtest "notmuch_database_get_config_list: empty list"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
    notmuch_config_list_t *list;
-   RUN(notmuch_database_get_config_list (db, "nonexistent", &list));
+   EXPECT0(notmuch_database_get_config_list (db, "nonexistent", &list));
    printf("valid = %d\n", notmuch_config_list_valid (list));
    notmuch_config_list_destroy (list);
 }
@@ -77,9 +66,9 @@ test_begin_subtest "notmuch_database_get_config_list: all pairs"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
    notmuch_config_list_t *list;
-   RUN(notmuch_database_set_config (db, "zzzafter", "afterval"));
-   RUN(notmuch_database_set_config (db, "aaabefore", "beforeval"));
-   RUN(notmuch_database_get_config_list (db, "", &list));
+   EXPECT0(notmuch_database_set_config (db, "zzzafter", "afterval"));
+   EXPECT0(notmuch_database_set_config (db, "aaabefore", "beforeval"));
+   EXPECT0(notmuch_database_get_config_list (db, "", &list));
    for (; notmuch_config_list_valid (list); notmuch_config_list_move_to_next (list)) {
       printf("%s %s\n", notmuch_config_list_key (list), notmuch_config_list_value(list));
    }
@@ -100,7 +89,7 @@ test_begin_subtest "notmuch_database_get_config_list: one prefix"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
    notmuch_config_list_t *list;
-   RUN(notmuch_database_get_config_list (db, "testkey", &list));
+   EXPECT0(notmuch_database_get_config_list (db, "testkey", &list));
    for (; notmuch_config_list_valid (list); notmuch_config_list_move_to_next (list)) {
       printf("%s %s\n", notmuch_config_list_key (list), notmuch_config_list_value(list));
    }
@@ -118,7 +107,7 @@ test_expect_equal_file EXPECTED OUTPUT
 test_begin_subtest "dump config"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
-    RUN(notmuch_database_set_config (db, "key with spaces", "value, with, spaces!"));
+    EXPECT0(notmuch_database_set_config (db, "key with spaces", "value, with, spaces!"));
 }
 EOF
 notmuch dump --include=config >OUTPUT
@@ -136,7 +125,7 @@ test_begin_subtest "restore config"
 notmuch dump --include=config >EXPECTED
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
 {
-    RUN(notmuch_database_set_config (db, "testkey1", "mutatedvalue"));
+    EXPECT0(notmuch_database_set_config (db, "testkey1", "mutatedvalue"));
 }
 EOF
 notmuch restore --include=config <EXPECTED
diff --git a/test/notmuch-test.h b/test/notmuch-test.h
new file mode 100644
index 0000000..e6e1d6c
--- /dev/null
+++ b/test/notmuch-test.h
@@ -0,0 +1,15 @@
+#ifndef _NOTMUCH_TEST_H
+#define _NOTMUCH_TEST_H
+#include <stdio.h>
+#include <notmuch.h>
+
+void expect0(int line, notmuch_status_t ret)
+{
+   if (ret) {
+	fprintf (stderr, "line %d: %s\n", line, ret);
+	exit (1);
+   }
+}
+
+#define EXPECT0(v)  expect0(__LINE__, v);
+#endif
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 201d0eb..442d293 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1199,7 +1199,7 @@ test_C () {
     test_file="${exec_file}.c"
     cat > ${test_file}
     export LD_LIBRARY_PATH=${TEST_DIRECTORY}/../lib
-    ${TEST_CC} ${TEST_CFLAGS} -I${TEST_DIRECTORY}/../lib -o ${exec_file} ${test_file} -L${TEST_DIRECTORY}/../lib/ -lnotmuch -ltalloc
+    ${TEST_CC} ${TEST_CFLAGS} -I${TEST_DIRECTORY} -I${TEST_DIRECTORY}/../lib -o ${exec_file} ${test_file} -L${TEST_DIRECTORY}/../lib/ -lnotmuch -ltalloc
     echo "== stdout ==" > OUTPUT.stdout
     echo "== stderr ==" > OUTPUT.stderr
     ./${exec_file} "$@" 1>>OUTPUT.stdout 2>>OUTPUT.stderr
-- 
2.8.1

[PATCH 3/4] test: initial tests for locking retry

[David Bremner]

Currently there's not much to test, so we simulate contention, and check
that the modifications to the database are serialized.
---
 test/T620-lock.sh | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100755 test/T620-lock.sh

diff --git a/test/T620-lock.sh b/test/T620-lock.sh
new file mode 100755
index 0000000..f46475e
--- /dev/null
+++ b/test/T620-lock.sh
@@ -0,0 +1,75 @@
+#!/usr/bin/env bash
+test_description="locking"
+. ./test-lib.sh || exit 1
+
+if [ "${NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK}" = "0" ]; then
+    test_subtest_missing_external_prereq_["lock retry support"]=t
+fi
+
+add_email_corpus
+
+test_begin_subtest "blocking open"
+test_C ${MAIL_DIR} <<'EOF'
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <notmuch-test.h>
+
+void
+taggit (notmuch_database_t *db, const char *tag)
+{
+    notmuch_message_t *message;
+
+    EXPECT0 (notmuch_database_find_message (db, "4EFC743A.3060609@april.org", &message));
+    if (message == NULL) {
+	fprintf (stderr, "unable to find message");
+	exit (1);
+    }
+
+    EXPECT0 (notmuch_message_add_tag (message, tag));
+    notmuch_message_destroy (message);
+}
+
+int
+main (int argc, char **argv)
+{
+    pid_t child;
+    const char *path = argv[1];
+
+    child = fork ();
+    if (child == -1) {
+	fprintf (stderr, "fork failed\n");
+	exit (1);
+    }
+
+    if (child == 0) {
+	notmuch_database_t *db2;
+
+	sleep (1);
+	EXPECT0 (notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &db2));
+	taggit (db2, "child");
+	EXPECT0 (notmuch_database_close (db2));
+    } else {
+	notmuch_database_t *db;
+
+	EXPECT0 (notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &db));
+	taggit (db, "parent");
+	sleep (2);
+	EXPECT0 (notmuch_database_close (db));
+	wait (NULL);
+    }
+}
+
+EOF
+notmuch search --output=tags id:4EFC743A.3060609@april.org >> OUTPUT
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+child
+inbox
+parent
+unread
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_done
-- 
2.8.1

[PATCH 4/4] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK

[David Bremner]

Currently building with DB_RETRY_LOCK is dependent on xapian version,
but I can imagine a configure switch to turn it off.
---
 lib/built-with.c | 2 ++
 notmuch-config.c | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/lib/built-with.c b/lib/built-with.c
index 7ea1d7f..9dd9e2d 100644
--- a/lib/built-with.c
+++ b/lib/built-with.c
@@ -28,6 +28,8 @@ notmuch_built_with (const char *name)
 	return HAVE_XAPIAN_COMPACT;
     } else if (STRNCMP_LITERAL (name, "field_processor") == 0) {
 	return HAVE_XAPIAN_FIELD_PROCESSOR;
+    } else if (STRNCMP_LITERAL (name, "retry_lock") == 0) {
+	return HAVE_XAPIAN_DB_RETRY_LOCK;
     } else {
 	return FALSE;
     }
diff --git a/notmuch-config.c b/notmuch-config.c
index c618f2c..16eaac9 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -911,6 +911,9 @@ _notmuch_config_list_built_with ()
     printf("%sfield_processor=%s\n",
 	   BUILT_WITH_PREFIX,
 	   notmuch_built_with ("field_processor") ? "true" : "false");
+    printf("%sretry_lock=%s\n",
+	   BUILT_WITH_PREFIX,
+	   notmuch_built_with ("retry_lock") ? "true" : "false");
 }
 
 static int
-- 
2.8.1

[PATCH] fixup! test: factor out some boilerplate from C tests

[David Bremner]

---
 test/notmuch-test.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/notmuch-test.h b/test/notmuch-test.h
index e6e1d6c..d39febb 100644
--- a/test/notmuch-test.h
+++ b/test/notmuch-test.h
@@ -3,7 +3,8 @@
 #include <stdio.h>
 #include <notmuch.h>
 
-void expect0(int line, notmuch_status_t ret)
+inline static void
+expect0(int line, notmuch_status_t ret)
 {
    if (ret) {
 	fprintf (stderr, "line %d: %s\n", line, ret);
-- 
2.8.1

[PATCH] fixup! lib: add built_with handling for XAPIAN_DB_RETRY_LOCK

[David Bremner]

---
 test/T030-config.sh | 1 +
 test/T040-setup.sh  | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/test/T030-config.sh b/test/T030-config.sh
index b8d5a86..0915abd 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -59,6 +59,7 @@ foo.string=this is another string value
 foo.list=this;is another;list value;
 built_with.compact=something
 built_with.field_processor=something
+built_with.retry_lock=something
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
diff --git a/test/T040-setup.sh b/test/T040-setup.sh
index be2f0db..021f2d0 100755
--- a/test/T040-setup.sh
+++ b/test/T040-setup.sh
@@ -31,6 +31,7 @@ search.exclude_tags=baz;
 maildir.synchronize_flags=true
 crypto.gpg_path=gpg
 built_with.compact=something
-built_with.field_processor=something"
+built_with.field_processor=something
+built_with.retry_lock=something"
 
 test_done
-- 
2.8.1

Re: [PATCH 2/4] test: factor out some boilerplate from C tests

[David Bremner]

David Bremner <david@tethera.net> writes:

> The trick of having a common header file doesn't work to share between
> test scripts, so make an include file in the test directory.

I pushed this one, and the fixup, because it was starting to get
confusing with various feature branches depending on it, or conflicting
with it.

d