lib: thread-safe s-expression query parser

Follow-up of 6273966d, now that sfsexp 1.4.1 doesn't rely on globals
anymore by default (https://github.com/mjsottile/sfsexp/issues/21).

This simply defers the initial query generation to use the thread-safe
helper (xapian_query_match_all) instead of Xapian::Query::MatchAll.

diff --git a/lib/parse-sexp.cc b/lib/parse-sexp.cc
index 9cadbc1359d3fcb2d3813bab7eae5d0a91d6f096..930888e9535ed2271936e9c74653343d182c4a5a 100644 (file)
--- a/lib/parse-sexp.cc
+++ b/lib/parse-sexp.cc
@@ -3,6 +3,7 @@
 #if HAVE_SFSEXP
 #include "sexp.h"
 #include "unicode-util.h"
+#include "xapian-extra.h"
 
 /* _sexp is used for file scope symbols to avoid clashing with
  * definitions from sexp.h */
@@ -39,82 +40,99 @@ typedef enum {
 /*
  * define bitwise operators to hide casts */
 
-inline _sexp_flag_t
+static inline _sexp_flag_t
 operator| (_sexp_flag_t a, _sexp_flag_t b)
 {
     return static_cast<_sexp_flag_t>(
        static_cast<unsigned>(a) | static_cast<unsigned>(b));
 }
 
-inline _sexp_flag_t
+static inline _sexp_flag_t
 operator& (_sexp_flag_t a, _sexp_flag_t b)
 {
     return static_cast<_sexp_flag_t>(
        static_cast<unsigned>(a) & static_cast<unsigned>(b));
 }
 
+typedef enum {
+    SEXP_INITIAL_MATCH_ALL,
+    SEXP_INITIAL_MATCH_NOTHING,
+} _sexp_initial_t;
+
+static inline Xapian::Query
+_sexp_initial_query (_sexp_initial_t initial)
+{
+    switch (initial) {
+    case SEXP_INITIAL_MATCH_ALL:
+       return xapian_query_match_all ();
+    case SEXP_INITIAL_MATCH_NOTHING:
+       return Xapian::Query::MatchNothing;
+    }
+    INTERNAL_ERROR ("invalid initial sexp value %d", initial);
+}
+
 typedef struct  {
     const char *name;
     Xapian::Query::op xapian_op;
-    Xapian::Query initial;
+    _sexp_initial_t initial;
     _sexp_flag_t flags;
 } _sexp_prefix_t;
 
 static _sexp_prefix_t prefixes[] =
 {
-    { "and",            Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "and",            Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_NONE },
-    { "attachment",     Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "attachment",     Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
-    { "body",           Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "body",           Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD },
-    { "date",           Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "date",           Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_RANGE },
-    { "from",           Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "from",           Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "folder",         Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "folder",         Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND |
       SEXP_FLAG_PATHNAME },
-    { "id",             Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "id",             Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX },
-    { "infix",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "infix",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_ORPHAN },
-    { "is",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "is",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "lastmod",           Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "lastmod",           Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_RANGE },
-    { "matching",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "matching",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_DO_EXPAND },
-    { "mid",            Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "mid",            Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX },
-    { "mimetype",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "mimetype",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
-    { "not",            Xapian::Query::OP_AND_NOT,      Xapian::Query::MatchAll,
+    { "not",            Xapian::Query::OP_AND_NOT,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_NONE },
-    { "of",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "of",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_DO_EXPAND },
-    { "or",             Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "or",             Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_NONE },
-    { "path",           Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "path",           Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX |
       SEXP_FLAG_PATHNAME },
-    { "property",       Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "property",       Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "query",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchNothing,
+    { "query",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_SINGLE | SEXP_FLAG_ORPHAN },
-    { "regex",          Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "regex",          Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_DO_REGEX },
-    { "rx",             Xapian::Query::OP_INVALID,      Xapian::Query::MatchAll,
+    { "rx",             Xapian::Query::OP_INVALID,      SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE | SEXP_FLAG_DO_REGEX },
-    { "starts-with",    Xapian::Query::OP_WILDCARD,     Xapian::Query::MatchAll,
+    { "starts-with",    Xapian::Query::OP_WILDCARD,     SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_SINGLE },
-    { "subject",        Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "subject",        Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "tag",            Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "tag",            Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "thread",         Xapian::Query::OP_OR,           Xapian::Query::MatchNothing,
+    { "thread",         Xapian::Query::OP_OR,           SEXP_INITIAL_MATCH_NOTHING,
       SEXP_FLAG_FIELD | SEXP_FLAG_BOOLEAN | SEXP_FLAG_WILDCARD | SEXP_FLAG_REGEX | SEXP_FLAG_EXPAND },
-    { "to",             Xapian::Query::OP_AND,          Xapian::Query::MatchAll,
+    { "to",             Xapian::Query::OP_AND,          SEXP_INITIAL_MATCH_ALL,
       SEXP_FLAG_FIELD | SEXP_FLAG_WILDCARD | SEXP_FLAG_EXPAND },
     { }
 };
@@ -318,7 +336,8 @@ _sexp_expand_query (notmuch_database_t *notmuch,
        return NOTMUCH_STATUS_BAD_QUERY_SYNTAX;
     }
 
-    status = _sexp_combine_query (notmuch, NULL, NULL, prefix->xapian_op, prefix->initial, sx,
+    status = _sexp_combine_query (notmuch, NULL, NULL, prefix->xapian_op,
+                                 _sexp_initial_query (prefix->initial), sx,
                                  subquery);
     if (status)
        return status;
@@ -370,7 +389,8 @@ _sexp_parse_header (notmuch_database_t *notmuch, const _sexp_prefix_t *parent,
 
     parent = &user_prefix;
 
-    return _sexp_combine_query (notmuch, parent, env, Xapian::Query::OP_AND, Xapian::Query::MatchAll,
+    return _sexp_combine_query (notmuch, parent, env, Xapian::Query::OP_AND,
+                               xapian_query_match_all (),
                                sx->list->next, output);
 }
 
@@ -520,7 +540,7 @@ _sexp_parse_range (notmuch_database_t *notmuch,  const _sexp_prefix_t *prefix,
 
     /* empty range matches everything */
     if (! sx) {
-       output = Xapian::Query::MatchAll;
+       output = xapian_query_match_all ();
        return NOTMUCH_STATUS_SUCCESS;
     }
 
@@ -628,7 +648,7 @@ _sexp_to_xapian_query (notmuch_database_t *notmuch, const _sexp_prefix_t *parent
 
     /* Empty list */
     if (! sx->list) {
-       output = Xapian::Query::MatchAll;
+       output = xapian_query_match_all ();
        return NOTMUCH_STATUS_SUCCESS;
     }
 
@@ -704,7 +724,8 @@ _sexp_to_xapian_query (notmuch_database_t *notmuch, const _sexp_prefix_t *parent
                return _sexp_expand_query (notmuch, prefix, parent, env, sx->list->next, output);
            }
 
-           return _sexp_combine_query (notmuch, parent, env, prefix->xapian_op, prefix->initial,
+           return _sexp_combine_query (notmuch, parent, env, prefix->xapian_op,
+                                       _sexp_initial_query (prefix->initial),
                                        sx->list->next, output);
        }
     }

diff --git a/test/T810-tsan.sh b/test/T810-tsan.sh
index 4071e2968f2ad62eb6642c68b39f2750327682d0..1829a733763eb32d8aa49d1d2a0b0806dc7a22a9 100755 (executable)
--- a/test/T810-tsan.sh
+++ b/test/T810-tsan.sh
@@ -4,7 +4,8 @@ test_directory=$(cd "$(dirname "${BASH_SOURCE[0]}")" > /dev/null && pwd)
 
 test_description='run code with TSan enabled against the library'
 # Note it is hard to ensure race conditions are deterministic so this
-# only provides best effort detection.
+# only provides best effort detection. Compile Notmuch with
+#  make CFLAGS=-fsanitize=thread LDFLAGS=-fsanitize=thread
 
 . "$test_directory"/test-lib.sh || exit 1
 
@@ -89,4 +90,43 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+if [ $NOTMUCH_HAVE_SFSEXP -eq 1 ]; then
+    test_begin_subtest "sexp query"
+    test_C ${MAIL_DIR} ${MAIL_DIR}-2 <<EOF
+#include <notmuch-test.h>
+#include <pthread.h>
+
+void *thread (void *arg) {
+  char *mail_dir = arg;
+  notmuch_database_t *db;
+  /*
+   * Query generation from s-expression used the tread-unsafe
+   * Xapian::Query::MatchAll.
+   */
+  EXPECT0(notmuch_database_open_with_config (mail_dir,
+                                             NOTMUCH_DATABASE_MODE_READ_ONLY,
+                                             NULL, NULL, &db, NULL));
+  notmuch_query_t *query;
+  EXPECT0(notmuch_query_create_with_syntax (db, "(from *)", NOTMUCH_QUERY_SYNTAX_SEXP, &query));
+  notmuch_messages_t *messages;
+  EXPECT0(notmuch_query_search_messages (query, &messages));
+  return NULL;
+}
+
+int main (int argc, char **argv) {
+  pthread_t t1, t2;
+  EXPECT0(pthread_create (&t1, NULL, thread, argv[1]));
+  EXPECT0(pthread_create (&t2, NULL, thread, argv[2]));
+  EXPECT0(pthread_join (t1, NULL));
+  EXPECT0(pthread_join (t2, NULL));
+  return 0;
+}
+EOF
+    cat <<EOF > EXPECTED
+== stdout ==
+== stderr ==
+EOF
+    test_expect_equal_file EXPECTED OUTPUT
+fi
+
 test_done