unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* More usable error messages when exceptions are not caught
@ 2022-12-02 19:05 Thomas Schneider
  2022-12-04  2:28 ` handle exceptions in _notmuch_message_delete David Bremner
  0 siblings, 1 reply; 7+ messages in thread
From: Thomas Schneider @ 2022-12-02 19:05 UTC (permalink / raw)
  To: notmuch

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

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

* handle exceptions in _notmuch_message_delete
  2022-12-02 19:05 More usable error messages when exceptions are not caught Thomas Schneider
@ 2022-12-04  2:28 ` David Bremner
  2022-12-04  2:28   ` [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete David Bremner
                     ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: David Bremner @ 2022-12-04  2:28 UTC (permalink / raw)
  To: Thomas Schneider, notmuch

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


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

* [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete
  2022-12-04  2:28 ` handle exceptions in _notmuch_message_delete David Bremner
@ 2022-12-04  2:28   ` David Bremner
  2022-12-26  0:04     ` Tomi Ollila
  2022-12-04  2:28   ` [PATCH 2/3] test: add known broken test for exception handling in _n_m_delete David Bremner
  2022-12-04  2:28   ` [PATCH 3/3] lib/message: move xapian call inside try/catch block " David Bremner
  2 siblings, 1 reply; 7+ messages in thread
From: David Bremner @ 2022-12-04  2:28 UTC (permalink / raw)
  To: Thomas Schneider, notmuch

_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

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

* [PATCH 2/3] test: add known broken test for exception handling in _n_m_delete
  2022-12-04  2:28 ` handle exceptions in _notmuch_message_delete David Bremner
  2022-12-04  2:28   ` [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete David Bremner
@ 2022-12-04  2:28   ` David Bremner
  2022-12-04  2:28   ` [PATCH 3/3] lib/message: move xapian call inside try/catch block " David Bremner
  2 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2022-12-04  2:28 UTC (permalink / raw)
  To: Thomas Schneider, notmuch

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

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

* [PATCH 3/3] lib/message: move xapian call inside try/catch block in _n_m_delete
  2022-12-04  2:28 ` handle exceptions in _notmuch_message_delete David Bremner
  2022-12-04  2:28   ` [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete David Bremner
  2022-12-04  2:28   ` [PATCH 2/3] test: add known broken test for exception handling in _n_m_delete David Bremner
@ 2022-12-04  2:28   ` David Bremner
  2 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2022-12-04  2:28 UTC (permalink / raw)
  To: Thomas Schneider, notmuch

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

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

* Re: [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete
  2022-12-04  2:28   ` [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete David Bremner
@ 2022-12-26  0:04     ` Tomi Ollila
  2022-12-27 16:05       ` David Bremner
  0 siblings, 1 reply; 7+ messages in thread
From: Tomi Ollila @ 2022-12-26  0:04 UTC (permalink / raw)
  To: David Bremner, Thomas Schneider, notmuch

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

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

* Re: [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete
  2022-12-26  0:04     ` Tomi Ollila
@ 2022-12-27 16:05       ` David Bremner
  0 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2022-12-27 16:05 UTC (permalink / raw)
  To: Tomi Ollila, Thomas Schneider, notmuch

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

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

end of thread, other threads:[~2022-12-27 16:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-02 19:05 More usable error messages when exceptions are not caught Thomas Schneider
2022-12-04  2:28 ` handle exceptions in _notmuch_message_delete David Bremner
2022-12-04  2:28   ` [PATCH 1/3] lib/database: propagate status code from _notmuch_message_delete David Bremner
2022-12-26  0:04     ` Tomi Ollila
2022-12-27 16:05       ` David Bremner
2022-12-04  2:28   ` [PATCH 2/3] test: add known broken test for exception handling in _n_m_delete David Bremner
2022-12-04  2:28   ` [PATCH 3/3] lib/message: move xapian call inside try/catch block " 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).