Re: Thread safety?

2023-02-22 Thread David Bremner 
Kevin Boulain  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.

___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: Thread safety?

2023-02-21 Thread Kevin Boulain 
On 2023-02-21 at 07:59 -04, David Bremner  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).
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Re: Thread safety?

2023-02-21 Thread David Bremner 
Kevin Boulain  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
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org

Thread safety?

2023-02-21 Thread 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 
#include 
#include 
void *thread(void *_) {
  char path[] = "/tmp/notmuch.XX";
  mkdtemp(path);
  notmuch_database_create(path, NULL);
  return NULL;
}
int main() {
  pthread_t t1, t2;
  pthread_create(, NULL, thread, NULL);
  pthread_create(, 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 0x7666e640 (LWP 154253) "a.out"):
#0  0x776822af in Xapian::Query::get_terms_begin (this=0x7fffe80137f0) 
at api/query.cc:141
#1  0x77f933f5 in _notmuch_query_cache_terms (query=0x7fffe80137c0) at 
lib/query.cc:176
#2  0x77f93784 in _notmuch_query_ensure_parsed_xapian 
(query=0x7fffe80137c0) at lib/query.cc:225
#3  0x77f9381a in _notmuch_query_ensure_parsed (query=0x7fffe80137c0) 
at lib/query.cc:260
#4  0x77f93bfe in _notmuch_query_search_documents 
(query=0x7fffe80137c0, type=0x77fa9b1e "mail", out=0x7666da18) at 
lib/query.cc:361
#5  0x77f93ba4 in notmuch_query_search_messages (query=0x7fffe80137c0, 
out=0x7666da18) at lib/query.cc:349
#6  0x77f83d98 in notmuch_database_upgrade (notmuch=0x7fffe8000bd0, 
progress_notify=0x0, closure=0x0) at lib/database.cc:934
#7  0x77fa110f in notmuch_database_create_with_config 
(database_path=0x7666dcb0 "/tmp/notmuch.MZ2AGr", config_path=0x77faab3c 
"", profile=0x0, database=0x0, status_string=0x7666dc90) at lib/open.cc:754
#8  0x77fa0d6f in notmuch_database_create_verbose (path=0x7666dcb0 
"/tmp/notmuch.MZ2AGr", database=0x0, status_string=0x7666dc90) at 
lib/open.cc:653
#9  0x77fa0ceb in notmuch_database_create (path=0x7666dcb0 
"/tmp/notmuch.MZ2AGr", database=0x0) at lib/open.cc:637
...

Thread 2 (Thread 0x76e6f640 (LWP 154252) "a.out"):
#0  0x776822af in Xapian::Query::get_terms_begin (this=0x7001e8a0) 
at api/query.cc:141
#1  0x77f933f5 in _notmuch_query_cache_terms (query=0x7001e870) at 
lib/query.cc:176
#2  0x77f93784 in _notmuch_query_ensure_parsed_xapian 
(query=0x7001e870) at lib/query.cc:225
#3  0x77f9381a in _notmuch_query_ensure_parsed (query=0x7001e870) 
at lib/query.cc:260
#4  0x77f93bfe in _notmuch_query_search_documents 
(query=0x7001e870, type=0x77fa9b1e "mail", out=0x76e6ea18) at 
lib/query.cc:361
#5  0x77f93ba4 in notmuch_query_search_messages (query=0x7001e870, 
out=0x76e6ea18) at lib/query.cc:349
#6  0x77f83d98 in notmuch_database_upgrade (notmuch=0x70003120, 
progress_notify=0x0, closure=0x0) at lib/database.cc:934
#7  0x77fa110f in notmuch_database_create_with_config 
(database_path=0x76e6ecb0 "/tmp/notmuch.eKNT5x", config_path=0x77faab3c 
"", profile=0x0, database=0x0, status_string=0x76e6ec90) at lib/open.cc:754
#8  0x77fa0d6f in notmuch_database_create_verbose (path=0x76e6ecb0 
"/tmp/notmuch.eKNT5x", database=0x0, status_string=0x76e6ec90) at 
lib/open.cc:653
#9  0x77fa0ceb in notmuch_database_create (path=0x76e6ecb0 
"/tmp/notmuch.eKNT5x", database=0x0) at lib/open.cc:637
...

Thread 1 (Thread 0x774468c0 (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!
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org