Thread safety?

Thread safety?

[Kevin Boulain]

Hey,

I quickly searched the mailing list archives but couldn't find a related
topic, apologies if this has already been discussed.
Should Notmuch be thread-safe?

Here's a simple repro (notmuch 09f2ad8e, xapian-core 1.4.22):
// cc -Wall -g -lpthread -lnotmuch
#include <stdlib.h>
#include <pthread.h>
#include <notmuch.h>
void *thread(void *_) {
  char path[] = "/tmp/notmuch.XXXXXX";
  mkdtemp(path);
  notmuch_database_create(path, NULL);
  return NULL;
}
int main() {
  pthread_t t1, t2;
  pthread_create(&t1, NULL, thread, NULL);
  pthread_create(&t2, NULL, thread, NULL);
  pthread_join(t1, NULL);
  pthread_join(t2, NULL);
  return 0;
}

(gdb) b _exit
(gdb) commands
> run
> end
(gdb) run
... let it run until the crash happens.
(gdb) thread apply all bt
Thread 3 (Thread 0x7ffff666e640 (LWP 154253) "a.out"):
#0  0x00007ffff76822af in Xapian::Query::get_terms_begin (this=0x7fffe80137f0) at api/query.cc:141
#1  0x00007ffff7f933f5 in _notmuch_query_cache_terms (query=0x7fffe80137c0) at lib/query.cc:176
#2  0x00007ffff7f93784 in _notmuch_query_ensure_parsed_xapian (query=0x7fffe80137c0) at lib/query.cc:225
#3  0x00007ffff7f9381a in _notmuch_query_ensure_parsed (query=0x7fffe80137c0) at lib/query.cc:260
#4  0x00007ffff7f93bfe in _notmuch_query_search_documents (query=0x7fffe80137c0, type=0x7ffff7fa9b1e "mail", out=0x7ffff666da18) at lib/query.cc:361
#5  0x00007ffff7f93ba4 in notmuch_query_search_messages (query=0x7fffe80137c0, out=0x7ffff666da18) at lib/query.cc:349
#6  0x00007ffff7f83d98 in notmuch_database_upgrade (notmuch=0x7fffe8000bd0, progress_notify=0x0, closure=0x0) at lib/database.cc:934
#7  0x00007ffff7fa110f in notmuch_database_create_with_config (database_path=0x7ffff666dcb0 "/tmp/notmuch.MZ2AGr", config_path=0x7ffff7faab3c "", profile=0x0, database=0x0, status_string=0x7ffff666dc90) at lib/open.cc:754
#8  0x00007ffff7fa0d6f in notmuch_database_create_verbose (path=0x7ffff666dcb0 "/tmp/notmuch.MZ2AGr", database=0x0, status_string=0x7ffff666dc90) at lib/open.cc:653
#9  0x00007ffff7fa0ceb in notmuch_database_create (path=0x7ffff666dcb0 "/tmp/notmuch.MZ2AGr", database=0x0) at lib/open.cc:637
...

Thread 2 (Thread 0x7ffff6e6f640 (LWP 154252) "a.out"):
#0  0x00007ffff76822af in Xapian::Query::get_terms_begin (this=0x7ffff001e8a0) at api/query.cc:141
#1  0x00007ffff7f933f5 in _notmuch_query_cache_terms (query=0x7ffff001e870) at lib/query.cc:176
#2  0x00007ffff7f93784 in _notmuch_query_ensure_parsed_xapian (query=0x7ffff001e870) at lib/query.cc:225
#3  0x00007ffff7f9381a in _notmuch_query_ensure_parsed (query=0x7ffff001e870) at lib/query.cc:260
#4  0x00007ffff7f93bfe in _notmuch_query_search_documents (query=0x7ffff001e870, type=0x7ffff7fa9b1e "mail", out=0x7ffff6e6ea18) at lib/query.cc:361
#5  0x00007ffff7f93ba4 in notmuch_query_search_messages (query=0x7ffff001e870, out=0x7ffff6e6ea18) at lib/query.cc:349
#6  0x00007ffff7f83d98 in notmuch_database_upgrade (notmuch=0x7ffff0003120, progress_notify=0x0, closure=0x0) at lib/database.cc:934
#7  0x00007ffff7fa110f in notmuch_database_create_with_config (database_path=0x7ffff6e6ecb0 "/tmp/notmuch.eKNT5x", config_path=0x7ffff7faab3c "", profile=0x0, database=0x0, status_string=0x7ffff6e6ec90) at lib/open.cc:754
#8  0x00007ffff7fa0d6f in notmuch_database_create_verbose (path=0x7ffff6e6ecb0 "/tmp/notmuch.eKNT5x", database=0x0, status_string=0x7ffff6e6ec90) at lib/open.cc:653
#9  0x00007ffff7fa0ceb in notmuch_database_create (path=0x7ffff6e6ecb0 "/tmp/notmuch.eKNT5x", database=0x0) at lib/open.cc:637
...

Thread 1 (Thread 0x7ffff74468c0 (LWP 154248) "a.out"):
...

The weird number of references in this(Xapian::Query)->internal gave me
a clue and the documentation acknowledges the issue:
https://github.com/xapian/xapian/blob/4715de3a9fcee741587439dc3cc1d2ff01ffeaf2/xapian-core/include/xapian/query.h#L65

With the following patch I can't get it to crash anymore:

diff --git i/lib/query.cc w/lib/query.cc
index 707f6222..348e1ec2 100644
--- i/lib/query.cc
+++ w/lib/query.cc
@@ -188,3 +188,3 @@ _notmuch_query_string_to_xapian_query (notmuch_database_t *notmuch,
        if (query_string == "" || query_string == "*") {
-           output = Xapian::Query::MatchAll;
+           output = Xapian::Query(std::string());
        } else {

So, is Notmuch expected to be thread safe? There are some indications it
should be (e.g.: lib/init.cc has locking) but all instances of
Xapian::Query::MatchAll would have to be replaced.
Xapian states it's thread safe:
https://github.com/xapian/xapian-docsprint/blob/master/concepts/concurrency.rst

Thanks!

Re: Thread safety?

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:


> So, is Notmuch expected to be thread safe? There are some indications it
> should be (e.g.: lib/init.cc has locking) but all instances of
> Xapian::Query::MatchAll would have to be replaced.
> Xapian states it's thread safe:
> https://github.com/xapian/xapian-docsprint/blob/master/concepts/concurrency.rst
>

Hi Kevin;

Thanks for tracking this down. I did try some experiments a while ago
with multi-threaded indexing (which is where the locking you mentioned
arose), but overall it isn't well tested (by me).  It seems reasonable
to try to provide the same level of thread safety as Xapian does. So I
guess we should go ahead and replace all of the MatchAll objects. I'm
guessing this might apply to Xapian::Query::MatchNothing as well.
Probably preprocessor macros wrapping Xapian::Query(std::string()) and
whatever the equivalent for MatchNothing is would make this easier to
read.

d

Re: Thread safety?

[Kevin Boulain]

On 2023-02-21 at 07:59 -04, David Bremner <david@tethera.net> wrote:
> Thanks for tracking this down. I did try some experiments a while ago
> with multi-threaded indexing (which is where the locking you mentioned
> arose), but overall it isn't well tested (by me).

To be honest I struggled a bit, notably on GLib which emitted false
positives under -fsanitize so I resorted to debugging that one manually
(but maybe I missed something obvious).
I only discovered the issue because the test framework I'm using runs
the tests in different threads.

> It seems reasonable to try to provide the same level of thread safety
> as Xapian does.

That's good to hear.

> So I guess we should go ahead and replace all of the MatchAll objects.
> I'm guessing this might apply to Xapian::Query::MatchNothing as well.
> Probably preprocessor macros wrapping Xapian::Query(std::string()) and
> whatever the equivalent for MatchNothing is would make this easier to
> read.

The documentation of MatchNothing says it's thread-safe.

If you'd like, I'm happy to submit a patch (likely very small, given the
limited number of occurrences) after I review a bit more the
documentation and the code (I'm just starting with Notmuch and Xapian so
that's probably the extent of what I can do).

Re: Thread safety?

[David Bremner]

Kevin Boulain <kevin@boula.in> writes:

> The documentation of MatchNothing says it's thread-safe.
>
> If you'd like, I'm happy to submit a patch (likely very small, given the
> limited number of occurrences) after I review a bit more the
> documentation and the code (I'm just starting with Notmuch and Xapian so
> that's probably the extent of what I can do).

Sounds good. With apologies for the imposing table of contents, please
also have a quick look at

     https://notmuchmail.org/contributing/

Probably the relevant bit is

         https://notmuchmail.org/contributing/#index5h2

I suspect a (deterministic) regression test is not really feasible.