More usable error messages when exceptions are not caught

More usable error messages when exceptions are not caught

[Thomas Schneider]

Hi,

it would be nice if the error message notmuch shows when an uncaught
exception occurs was more helpful to the user.  It could at least show
the exception message (`e->what()` iirc), which would have helped in
this case.

From IRC:

<qsx> % notmuch new
<qsx> libc++abi: terminating due to uncaught exception of type Xapian::DatabaseError
<qsx> fun
<qsx> does this sound interesting enough to build notmuch and xapian with debug symbols?
[…]
<qsx> but i think the issue is that there’s no space left on the filesystem where the xapian db lives
[…]
<qsx> sounds reasonable
<qsx> but i think it would be nicer for the user if notmuch at least showed the message of the exception, because that was some work to get to this point

Backtrace:

(gdb) p *(Xapian::DatabaseError *)thrown_object
$13 = {<Xapian::RuntimeError> = {<Xapian::Error> = {msg = {static __endian_factor = 2, 
        __r_ = {<std::__1::__compressed_pair_elem<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__rep, 0, false>> = {__value_ = {{__l = {{__is_long_ = 1, __cap_ = 16}, 
                  __size_ = 25, __data_ = 0x555582cd4fc0 "Error writing block 83232"}, __s = {{__is_long_ = 1 '\001', __size_ = 16 '\020'}, __padding_ = 0x555555c15e61 "", 
                  __data_ = "\000\000\000\000\000\000\000\031\000\000\000\000\000\000\000\300O͂UU\000"}, __r = {__words = {33, 25, 
                    93825755074496}}}}}, <std::__1::__compressed_pair_elem<std::__1::allocator<char>, 1, true>> = {<std::__1::allocator<char>> = {<std::__1::__non_trivial_if<true, std::__1::allocator<char> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, static npos = 18446744073709551615}, context = {static __endian_factor = 2, 
        __r_ = {<std::__1::__compressed_pair_elem<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__rep, 0, false>> = {__value_ = {{__l = {{__is_long_ = 0, __cap_ = 0}, 
                  __size_ = 0, __data_ = 0x0}, __s = {{__is_long_ = 0 '\000', __size_ = 0 '\000'}, __padding_ = 0x555555c15e79 "", __data_ = '\000' <repeats 22 times>}, __r = {__words = {0, 0, 
                    0}}}}}, <std::__1::__compressed_pair_elem<std::__1::allocator<char>, 1, true>> = {<std::__1::allocator<char>> = {<std::__1::__non_trivial_if<true, std::__1::allocator<char> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, static npos = 18446744073709551615}, error_string = {static __endian_factor = 2, 
        __r_ = {<std::__1::__compressed_pair_elem<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__rep, 0, false>> = {__value_ = {{__l = {{__is_long_ = 0, __cap_ = 0}, 
                  __size_ = 0, __data_ = 0x0}, __s = {{__is_long_ = 0 '\000', __size_ = 0 '\000'}, __padding_ = 0x555555c15e91 "", __data_ = '\000' <repeats 22 times>}, __r = {__words = {0, 0, 
                    0}}}}}, <std::__1::__compressed_pair_elem<std::__1::allocator<char>, 1, true>> = {<std::__1::allocator<char>> = {<std::__1::__non_trivial_if<true, std::__1::allocator<char> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, static npos = 18446744073709551615}, type = 0x7ffff797410c <str.17.llvm> "\004DatabaseError", my_errno = 28, 
      already_handled = false}, <No data fields>}, <No data fields>}

	--qsx\r

handle exceptions in _notmuch_message_delete

[David Bremner]

It's a bit tricky to test running out of disk space in the test suite but I think I caught the exception shown
in your first backtrace (which was not attached to your mail, only pasted to IRC).

#10 GlassWritableDatabase::delete_document(unsigned int) (this=0x55da391cc550, did=413358) at /tmp/portage/dev-libs/xapian-1.4.19/work/xapian-core-1.4.19/backends/glass/glass_database.cc:1223
#11 0x00007fb7f3fba3a0 in _notmuch_message_delete(notmuch_message_t*) (message=0x55da5f74f070) at lib/message.cc:1374
#12 0x00007fb7f3fb376a in notmuch_database_remove_message(notmuch_database_t*, char const*)
    (notmuch=<optimized out>, filename=0x55da41a8c440 "/var/dedup/qsx/maildir/AStA.INBOX.Admin/cur/1636633029.12621_18.neptun,U=9117:2,S") at lib/database.cc:1438
#13 0x000055da374eea01 in remove_filename (notmuch=0x55da39183530, path=0x55da41a8c440 "/var/dedup/qsx/maildir/AStA.INBOX.Admin/cur/1636633029.12621_18.neptun,U=9117:2,S", add_files_state=0x7ffe493a1440)
    at notmuch-new.c:950
#14 0x000055da374eed09 in _remove_directory (notmuch=0x55da39183530, path=0x55da3faad350 "/var/dedup/qsx/maildir/AStA.INBOX.Admin/cur", add_files_state=0x7ffe493a1440) at notmuch-new.c:987
#15 0x000055da374eed79 in _remove_directory (notmuch=0x55da39183530, path=0x55da3a7349d0 "/var/dedup/qsx/maildir/AStA.INBOX.Admin", add_files_state=0x7ffe493a1440) at notmuch-new.c:998
#16 0x000055da374ed6e5 in notmuch_new_command (notmuch=0x55da39183530, argc=<optimized out>, argv=<optimized out>) at notmuch-new.c:1267
#17 0x000055da374e910b in main (argc=<optimized out>, argv=0x7ffe493a1bc8) at notmuch.c:588

[PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete

[David Bremner]

_notmuch_message_delete can return (at least)
NOTMUCH_STATUS_XAPIAN_EXCEPTION, which we should not ignore.
---
 lib/database.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/database.cc b/lib/database.cc
index c05d70d3..d1e5f1af 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1456,7 +1456,7 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
     if (status == NOTMUCH_STATUS_SUCCESS && message) {
 	status = _notmuch_message_remove_filename (message, filename);
 	if (status == NOTMUCH_STATUS_SUCCESS)
-	    _notmuch_message_delete (message);
+	    status = _notmuch_message_delete (message);
 	else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
 	    _notmuch_message_sync (message);
 
-- 
2.35.2

Re: [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete

[Tomi Ollila]

On Sat, Dec 03 2022, David Bremner wrote:

> _notmuch_message_delete can return (at least)
> NOTMUCH_STATUS_XAPIAN_EXCEPTION, which we should not ignore.

Series LGTM
Tomi


> ---
>  lib/database.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index c05d70d3..d1e5f1af 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1456,7 +1456,7 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
>      if (status == NOTMUCH_STATUS_SUCCESS && message) {
>  	status = _notmuch_message_remove_filename (message, filename);
>  	if (status == NOTMUCH_STATUS_SUCCESS)
> -	    _notmuch_message_delete (message);
> +	    status = _notmuch_message_delete (message);
>  	else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
>  	    _notmuch_message_sync (message);
>  
> -- 
> 2.35.2
>
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org

Re: [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Sat, Dec 03 2022, David Bremner wrote:
>
>> _notmuch_message_delete can return (at least)
>> NOTMUCH_STATUS_XAPIAN_EXCEPTION, which we should not ignore.
>
> Series LGTM
> Tomi

Series applied to master.

d

[PATCH 2/3] test: add known broken test for exception handling in _n_m_delete

[David Bremner]

In [1], Thomas Schneider reported an uncaught Xapian exception when
running out of disk space. We generate the same exception via database
corruption.

[1]: id:wwuk039sk2p.fsf@chaotikum.eu
---
 test/T566-lib-message.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/test/T566-lib-message.sh b/test/T566-lib-message.sh
index 511d56ca..562ab05a 100755
--- a/test/T566-lib-message.sh
+++ b/test/T566-lib-message.sh
@@ -516,4 +516,32 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+TERMLIST_PATH=(${MAIL_DIR}/.notmuch/xapian/termlist.*)
+test_begin_subtest "remove message with corrupted db"
+test_subtest_known_broken
+backup_database
+cat c_head0 - c_tail <<'EOF' | test_private_C ${MAIL_DIR} ${TERMLIST_PATH}
+    {
+        notmuch_status_t status;
+
+        int fd = open(argv[2],O_WRONLY|O_TRUNC);
+        if (fd < 0) {
+            fprintf (stderr, "error opening %s\n", argv[1]);
+            exit (1);
+        }
+
+        stat = _notmuch_message_delete (message);
+        printf ("%d\n", stat == NOTMUCH_STATUS_XAPIAN_EXCEPTION);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1
+== stderr ==
+A Xapian exception occurred at message.cc:XXX: EOF reading block YYY
+EOF
+sed 's/EOF reading block [0-9]*/EOF reading block YYY/' < OUTPUT > OUTPUT.clean
+test_expect_equal_file EXPECTED OUTPUT.clean
+restore_database
+
 test_done
-- 
2.35.2

[PATCH 3/3] lib/message: move xapian call inside try/catch block in _n_m_delete

[David Bremner]

The call to delete_document can throw exceptions (and can happen in
practice [1]), so catch the exception and extract the error
message. As a side effect, also move the call to _n_m_has_term inside
the try/catch. This should not change anything as that function
already traps any Xapian exceptions.

[1]: id:wwuk039sk2p.fsf@chaotikum.eu
---
 lib/message.cc           | 22 +++++++++++-----------
 test/T566-lib-message.sh |  1 -
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 1c87f8c0..5ccca95a 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1383,21 +1383,21 @@ _notmuch_message_delete (notmuch_message_t *message)
     if (status)
 	return status;
 
-    message->notmuch->writable_xapian_db->delete_document (message->doc_id);
-
-    /* if this was a ghost to begin with, we are done */
-    private_status = _notmuch_message_has_term (message, "type", "ghost", &is_ghost);
-    if (private_status)
-	return COERCE_STATUS (private_status,
-			      "Error trying to determine whether message was a ghost");
-    if (is_ghost)
-	return NOTMUCH_STATUS_SUCCESS;
-
-    /* look for a non-ghost message in the same thread */
     try {
 	Xapian::PostingIterator thread_doc, thread_doc_end;
 	Xapian::PostingIterator mail_doc, mail_doc_end;
 
+	message->notmuch->writable_xapian_db->delete_document (message->doc_id);
+
+	/* look for a non-ghost message in the same thread */
+	/* if this was a ghost to begin with, we are done */
+	private_status = _notmuch_message_has_term (message, "type", "ghost", &is_ghost);
+	if (private_status)
+	    return COERCE_STATUS (private_status,
+				  "Error trying to determine whether message was a ghost");
+	if (is_ghost)
+	    return NOTMUCH_STATUS_SUCCESS;
+
 	_notmuch_database_find_doc_ids (message->notmuch, "thread", tid, &thread_doc,
 					&thread_doc_end);
 	_notmuch_database_find_doc_ids (message->notmuch, "type", "mail", &mail_doc, &mail_doc_end);
diff --git a/test/T566-lib-message.sh b/test/T566-lib-message.sh
index 562ab05a..7f0e8eb0 100755
--- a/test/T566-lib-message.sh
+++ b/test/T566-lib-message.sh
@@ -518,7 +518,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 TERMLIST_PATH=(${MAIL_DIR}/.notmuch/xapian/termlist.*)
 test_begin_subtest "remove message with corrupted db"
-test_subtest_known_broken
 backup_database
 cat c_head0 - c_tail <<'EOF' | test_private_C ${MAIL_DIR} ${TERMLIST_PATH}
     {
-- 
2.35.2