unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* Database locked when running hooks
@ 2021-03-18  8:32 Matthew Lear
  2021-03-18 10:55 ` David Bremner
  2021-03-19  2:07 ` [PATCH 1/2] test: Add tests for write access to database from hooks David Bremner
  0 siblings, 2 replies; 5+ messages in thread
From: Matthew Lear @ 2021-03-18  8:32 UTC (permalink / raw)
  To: notmuch


[-- Attachment #1.1: Type: text/plain, Size: 3495 bytes --]

Hello all,
I recently updated to notmuch 0.31.4+137~g6967dcb and made some changes to the way I sync my mail using lieer.
I wanted to utilise the pre-new and post-new notmuch hooks.
However, when running an operation to import mail in the context of the notmuch pre-new hook (by running gmi pull), I can see that the database is locked, which means that running notmuch new hangs.
From man (5) notmuch-hooks the wording is such that it implies that the database is not locked when the pre-new and post-new hooks are run.

If I strace notmuch new, I can see that the database is opened for writing before the pre-new hook is run, and it is not closed before the hook is run.
This means that any operation which needs write access to database (e.g. lieer’s gmi pull) cannot obtain the database lock, so it hangs.

The problem can be reproduced by creating $DATABASEDIR/.notmuch/hooks/pre-new and having it perform a notmuch CLI command to write to the DB, such as apply a tag.

I described the problem in https://github.com/gauteh/lieer/issues/195 <https://github.com/gauteh/lieer/issues/195>

Assuming the database should be able to be opened for writing in the context of the pre-new (and post-new) hooks, this seems like a bug.
Here is some output...

Running notmuch new before the pre-new hook is executed:

..
openat(AT_FDCWD, "/home/myhome/path/to/maildir//.notmuch/xapian/flintlock", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 3
fcntl(3, F_OFD_SETLKW, {l_type=F_WRLCK, l_whence=SEEK_SET, l_start=0, l_len=1}) = 0
openat(AT_FDCWD, "/home/ml943601/b/mail/brcm//.notmuch/xapian/iamglass", O_RDONLY) = 4
..

It’s never closed before the hook is executed.
What the hang looks like:

 [~]
$ notmuch new
path: /home/myhome/path/to/maildir/gmaildir/
pull: partial synchronization.. (hid: 66446860)
fetching changes ....fetching changes ....fetching changes ....fetching changes ...resolving changes (376) ........................................receiving content (156) ...^C
Traceback (most recent call last):
  File "/home/myhome/git/lieer/gmi", line 24, in <module>
    g.main ()
  File "/home/myhome/git/lieer/lieer/gmailieer.py", line 215, in main
    args.func (args)
  File "/home/myhome/git/lieer/lieer/gmailieer.py", line 418, in pull
    self.partial_pull ()
  File "/home/myhome/git/lieer/lieer/gmailieer.py", line 548, in partial_pull
    updated     = self.get_content (message_gids)
  File "/home/myhome/git/lieer/lieer/gmailieer.py", line 754, in get_content
    self.remote.get_messages (need_content, _got_msgs, 'raw')
  File "/home/myhome/git/lieer/lieer/remote.py", line 133, in func_wrap
    return func (self, *args, **kwargs)
  File "/home/myhome/git/lieer/lieer/remote.py", line 384, in get_messages
    cb (msg_batch)
  File "/home/myhome/git/lieer/lieer/gmailieer.py", line 749, in _got_msgs
    with notmuch.Database (mode = notmuch.Database.MODE.READ_WRITE) as db:
  File "/usr/local/lib/python3.8/dist-packages/notmuch/database.py", line 164, in __init__
    self.open(path, mode)
  File "/usr/local/lib/python3.8/dist-packages/notmuch/database.py", line 223, in open
    status = Database._open(_str(path), mode, byref(db))

There is no other program running at the time of the hang which accesses the database.
The problem seems to be caused by the database being opened for writing before the hook is executed.

Does notmuch require the database to be open when calling the hook?

Cheers,
—  Matt


[-- Attachment #1.2: Type: text/html, Size: 5177 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Database locked when running hooks
  2021-03-18  8:32 Database locked when running hooks Matthew Lear
@ 2021-03-18 10:55 ` David Bremner
  2021-03-19  2:07 ` [PATCH 1/2] test: Add tests for write access to database from hooks David Bremner
  1 sibling, 0 replies; 5+ messages in thread
From: David Bremner @ 2021-03-18 10:55 UTC (permalink / raw)
  To: Matthew Lear, notmuch

Matthew Lear <matt@bubblegen.co.uk> writes:
>
> The problem can be reproduced by creating $DATABASEDIR/.notmuch/hooks/pre-new and having it perform a notmuch CLI command to write to the DB, such as apply a tag.
>
> I described the problem in https://github.com/gauteh/lieer/issues/195 <https://github.com/gauteh/lieer/issues/195>
>
> Assuming the database should be able to be opened for writing in the context of the pre-new (and post-new) hooks, this seems like a bug.

Yes, this is a bug I introduced recently, thanks for bringing that to my
attention. It should be just the pre-new hook which is (incorrectly) run
while the database is locked, the post-new should continue to work as
before.  It should be fairly easy to fix, I just need to initially open
the database read only, then re-open it for read/write after running the
hook.

d

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] test: Add tests for write access to database from hooks.
  2021-03-18  8:32 Database locked when running hooks Matthew Lear
  2021-03-18 10:55 ` David Bremner
@ 2021-03-19  2:07 ` David Bremner
  2021-03-19  2:07   ` [PATCH 2/2] CLI/new: drop the write lock to run the pre-new hook David Bremner
  1 sibling, 1 reply; 5+ messages in thread
From: David Bremner @ 2021-03-19  2:07 UTC (permalink / raw)
  To: Matthew Lear, notmuch; +Cc: David Bremner

Recent changes to configuration handling meant the pre-new hook was
run while the database was open read only, limiting what could be done
in the hook. Add some known broken tests for this problem, as well as
a regression test for write access from the post-new hook.
---
 test/T400-hooks.sh | 28 ++++++++++++++++++++++++++++
 test/test-lib.sh   |  1 +
 2 files changed, 29 insertions(+)

diff --git a/test/T400-hooks.sh b/test/T400-hooks.sh
index a3dd4c63..a2b67d59 100755
--- a/test/T400-hooks.sh
+++ b/test/T400-hooks.sh
@@ -2,6 +2,8 @@
 test_description='hooks'
 . $(dirname "$0")/test-lib.sh || exit 1
 
+test_require_external_prereq xapian-delve
+
 create_echo_hook () {
     local TOKEN="${RANDOM}"
     mkdir -p ${HOOK_DIR}
@@ -13,6 +15,19 @@ EOF
     echo "${TOKEN}" > ${2}
 }
 
+create_write_hook () {
+    local TOKEN="${RANDOM}"
+    mkdir -p ${HOOK_DIR}
+    cat <<EOF >"${HOOK_DIR}/${1}"
+#!/bin/sh
+if xapian-delve ${MAIL_DIR}/.notmuch/xapian | grep -q "writing = false"; then
+   echo "${TOKEN}" > ${3}
+fi
+EOF
+    chmod +x "${HOOK_DIR}/${1}"
+    echo "${TOKEN}" > ${2}
+}
+
 create_failing_hook () {
     local HOOK_DIR=${2}
     mkdir -p ${HOOK_DIR}
@@ -137,6 +152,19 @@ EOF
     chmod +x "${HOOK_DIR}/pre-new"
     test_expect_code 1 "notmuch new"
 
+    test_begin_subtest "post-new with write access [${config}]"
+    rm -rf ${HOOK_DIR}
+    create_write_hook "post-new" write.expected write.output $HOOK_DIR
+    NOTMUCH_NEW
+    test_expect_equal_file write.expected write.output
+
+    test_begin_subtest "pre-new with write access [${config}]"
+    test_subtest_known_broken
+    rm -rf ${HOOK_DIR}
+    create_write_hook "pre-new" write.expected write.output $HOOK_DIR
+    NOTMUCH_NEW
+    test_expect_equal_file write.expected write.output
+
     rm -rf ${HOOK_DIR}
 done
 test_done
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 29baa0c1..fc176af8 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1271,3 +1271,4 @@ test_declare_external_prereq openssl
 test_declare_external_prereq gpgsm
 test_declare_external_prereq ${NOTMUCH_PYTHON}
 test_declare_external_prereq xapian-metadata
+test_declare_external_prereq xapian-delve
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] CLI/new: drop the write lock to run the pre-new hook.
  2021-03-19  2:07 ` [PATCH 1/2] test: Add tests for write access to database from hooks David Bremner
@ 2021-03-19  2:07   ` David Bremner
  2021-03-24 11:03     ` David Bremner
  0 siblings, 1 reply; 5+ messages in thread
From: David Bremner @ 2021-03-19  2:07 UTC (permalink / raw)
  To: Matthew Lear, notmuch; +Cc: David Bremner

This fixes a bug reported in [1]. In principle it could be possible
avoid one of these reopens, but it complicates the logic in main with
respect to creating new databases.

[1]: id:9C1993DF-84BD-4199-A9C8-BADA98498812@bubblegen.co.uk
---
 notmuch-new.c      | 10 ++++++++++
 test/T400-hooks.sh |  1 -
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 223d68bb..8214fb23 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -1170,9 +1170,19 @@ notmuch_new_command (unused(notmuch_config_t *config), notmuch_database_t *notmu
     }
 
     if (hooks) {
+	/* Drop write lock to run hook */
+	status = notmuch_database_reopen (notmuch, NOTMUCH_DATABASE_MODE_READ_ONLY);
+	if (print_status_database ("notmuch new", notmuch, status))
+	    return EXIT_FAILURE;
+
 	ret = notmuch_run_hook (notmuch, "pre-new");
 	if (ret)
 	    return EXIT_FAILURE;
+
+	/* acquire write lock again */
+	status = notmuch_database_reopen (notmuch, NOTMUCH_DATABASE_MODE_READ_WRITE);
+	if (print_status_database ("notmuch new", notmuch, status))
+	    return EXIT_FAILURE;
     }
 
     notmuch_exit_if_unmatched_db_uuid (notmuch);
diff --git a/test/T400-hooks.sh b/test/T400-hooks.sh
index a2b67d59..de8e4ba4 100755
--- a/test/T400-hooks.sh
+++ b/test/T400-hooks.sh
@@ -159,7 +159,6 @@ EOF
     test_expect_equal_file write.expected write.output
 
     test_begin_subtest "pre-new with write access [${config}]"
-    test_subtest_known_broken
     rm -rf ${HOOK_DIR}
     create_write_hook "pre-new" write.expected write.output $HOOK_DIR
     NOTMUCH_NEW
-- 
2.30.2

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] CLI/new: drop the write lock to run the pre-new hook.
  2021-03-19  2:07   ` [PATCH 2/2] CLI/new: drop the write lock to run the pre-new hook David Bremner
@ 2021-03-24 11:03     ` David Bremner
  0 siblings, 0 replies; 5+ messages in thread
From: David Bremner @ 2021-03-24 11:03 UTC (permalink / raw)
  To: Matthew Lear, notmuch

David Bremner <david@tethera.net> writes:

> This fixes a bug reported in [1]. In principle it could be possible
> avoid one of these reopens, but it complicates the logic in main with
> respect to creating new databases.

I have applied these two patches to master

d

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-03-24 11:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18  8:32 Database locked when running hooks Matthew Lear
2021-03-18 10:55 ` David Bremner
2021-03-19  2:07 ` [PATCH 1/2] test: Add tests for write access to database from hooks David Bremner
2021-03-19  2:07   ` [PATCH 2/2] CLI/new: drop the write lock to run the pre-new hook David Bremner
2021-03-24 11:03     ` David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).