unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* introduce exception handling at top level of libnotmuch
@ 2020-06-30  1:14 David Bremner
  2020-06-30  1:14 ` [PATCH 1/4] test: add known broken test for error handling on closed database David Bremner
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: David Bremner @ 2020-06-30  1:14 UTC (permalink / raw)
  To: notmuch

I know that some of you are not C++ fans, but at the moment this is
the cleanest fix I can think of to uncaught xapian exceptions causing
calls to the library to die. Floris reminded me of this recently with
the discussion about operations on closed databases, but cleaning up
the handling of exceptions in libnotmuch has been on my mind for a
while. It will be bit laborious so I did a few functions for
discussion purposes before getting too carried away.

There is still a certain amount of boilerplate with more or less
identical try/catch blocks (yes, I really miss scheme macros here). I
could mostly eliminate that with C++11 lambdas, but I wasn't sure the
result was more maintainable or nicer.

This is definitely targeted for post 0.30.


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

* [PATCH 1/4] test: add known broken test for error handling on closed database
  2020-06-30  1:14 introduce exception handling at top level of libnotmuch David Bremner
@ 2020-06-30  1:14 ` David Bremner
  2020-06-30  1:14 ` [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id David Bremner
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: David Bremner @ 2020-06-30  1:14 UTC (permalink / raw)
  To: notmuch

Based on id:87d05je1j6.fsf@powell.devork.be
---
 test/T560-lib-error.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 test/notmuch-test.h    |  1 +
 2 files changed, 41 insertions(+)

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index 06a6b860..5a5f66b8 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -318,4 +318,44 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT.clean
 restore_database
 
+cat <<EOF > c_head2
+#include <stdio.h>
+#include <notmuch.h>
+#include <notmuch-test.h>
+#include <assert.h>
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_status_t stat;
+   char *msg = NULL;
+   notmuch_message_t *message = NULL;
+   const char *id = "1258471718-6781-1-git-send-email-dottedmag@dottedmag.net";
+
+   stat = notmuch_database_open_verbose (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db, &msg);
+   if (stat != NOTMUCH_STATUS_SUCCESS) {
+     fprintf (stderr, "error opening database: %d %s\n", stat, msg ? msg : "");
+     exit (1);
+   }
+   EXPECT0(notmuch_database_find_message (db, id, &message));
+   assert(message != NULL);
+   EXPECT0(notmuch_database_close (db));
+EOF
+
+backup_database
+test_begin_subtest "Handle getting message-id from closed database"
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        const char *id2;
+        id2=notmuch_message_get_message_id (message);
+        printf("%s\n%d\n", id, id2==NULL);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1258471718-6781-1-git-send-email-dottedmag@dottedmag.net
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
diff --git a/test/notmuch-test.h b/test/notmuch-test.h
index df852da9..34dbb8e0 100644
--- a/test/notmuch-test.h
+++ b/test/notmuch-test.h
@@ -1,6 +1,7 @@
 #ifndef _NOTMUCH_TEST_H
 #define _NOTMUCH_TEST_H
 #include <stdio.h>
+#include <stdlib.h>
 #include <notmuch.h>
 
 inline static void
-- 
2.27.0

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

* [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id
  2020-06-30  1:14 introduce exception handling at top level of libnotmuch David Bremner
  2020-06-30  1:14 ` [PATCH 1/4] test: add known broken test for error handling on closed database David Bremner
@ 2020-06-30  1:14 ` David Bremner
  2020-07-04 15:44   ` Floris Bruynooghe
  2020-06-30  1:14 ` [PATCH 3/4] test: add known broken test for n_m_get_thread_id on closed db David Bremner
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: David Bremner @ 2020-06-30  1:14 UTC (permalink / raw)
  To: notmuch

By catching it at the library top level, we can return an error value.
---
 lib/message.cc | 23 +++++++++++++++++++----
 lib/notmuch.h  |  5 ++---
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 0fa0eb3a..b7a64b1c 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -90,6 +90,18 @@ _notmuch_message_destructor (notmuch_message_t *message)
     return 0;
 }
 
+#define LOG_XAPIAN_EXCEPTION(message, error) _log_xapian_exception (__location__, message, error)
+
+static void
+_log_xapian_exception (const char *where, notmuch_message_t *message,  const Xapian::Error error) {
+    notmuch_database_t *notmuch = notmuch_message_get_database (message);
+    _notmuch_database_log (notmuch,
+			   "A Xapian exception occurred %s retrieving %s : %s\n",
+			   where,
+			   error.get_msg ().c_str ());
+    notmuch->exception_reported = true;
+}
+
 static notmuch_message_t *
 _notmuch_message_create_for_document (const void *talloc_owner,
 				      notmuch_database_t *notmuch,
@@ -447,9 +459,6 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message, void *field)
 	    if (status != NOTMUCH_STATUS_SUCCESS)
 		INTERNAL_ERROR ("unhandled error from notmuch_database_reopen: %s\n",
 				notmuch_status_to_string (status));
-	} catch (const Xapian::Error &error) {
-	    INTERNAL_ERROR ("A Xapian exception occurred fetching message metadata: %s\n",
-			    error.get_msg ().c_str ());
 	}
     }
     message->last_view = message->notmuch->view;
@@ -507,7 +516,13 @@ _notmuch_message_get_doc_id (notmuch_message_t *message)
 const char *
 notmuch_message_get_message_id (notmuch_message_t *message)
 {
-    _notmuch_message_ensure_metadata (message, message->message_id);
+    try {
+	_notmuch_message_ensure_metadata (message, message->message_id);
+    } catch (const Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	return NULL;
+    }
+
     if (! message->message_id)
 	INTERNAL_ERROR ("Message with document ID of %u has no message ID.\n",
 			message->doc_id);
diff --git a/lib/notmuch.h b/lib/notmuch.h
index ceb5a018..0dc89547 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1363,9 +1363,8 @@ notmuch_message_get_database (const notmuch_message_t *message);
  * message is valid, (which is until the query from which it derived
  * is destroyed).
  *
- * This function will not return NULL since Notmuch ensures that every
- * message has a unique message ID, (Notmuch will generate an ID for a
- * message if the original file does not contain one).
+ * This function will return NULL if triggers an unhandled Xapian
+ * exception.
  */
 const char *
 notmuch_message_get_message_id (notmuch_message_t *message);
-- 
2.27.0

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

* [PATCH 3/4] test: add known broken test for n_m_get_thread_id on closed db
  2020-06-30  1:14 introduce exception handling at top level of libnotmuch David Bremner
  2020-06-30  1:14 ` [PATCH 1/4] test: add known broken test for error handling on closed database David Bremner
  2020-06-30  1:14 ` [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id David Bremner
@ 2020-06-30  1:14 ` David Bremner
  2020-06-30  1:14 ` [PATCH 4/4] lib/message: catch exception in n_m_get_thread_id David Bremner
  2020-07-02 19:16 ` introduce exception handling at top level of libnotmuch Daniel Kahn Gillmor
  4 siblings, 0 replies; 12+ messages in thread
From: David Bremner @ 2020-06-30  1:14 UTC (permalink / raw)
  To: notmuch

This will be fixed in the next commit.
---
 test/T560-lib-error.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index 5a5f66b8..b5600851 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -358,4 +358,22 @@ cat <<EOF > EXPECTED
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+backup_database
+test_begin_subtest "Handle getting thread-id from closed database"
+test_subtest_known_broken
+cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
+    {
+        const char *id2;
+        id2=notmuch_message_get_thread_id (message);
+        printf("%s\n%d\n", id, id2==NULL);
+    }
+EOF
+cat <<EOF > EXPECTED
+== stdout ==
+1258471718-6781-1-git-send-email-dottedmag@dottedmag.net
+1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.27.0

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

* [PATCH 4/4] lib/message: catch exception in n_m_get_thread_id
  2020-06-30  1:14 introduce exception handling at top level of libnotmuch David Bremner
                   ` (2 preceding siblings ...)
  2020-06-30  1:14 ` [PATCH 3/4] test: add known broken test for n_m_get_thread_id on closed db David Bremner
@ 2020-06-30  1:14 ` David Bremner
  2020-07-02 19:16 ` introduce exception handling at top level of libnotmuch Daniel Kahn Gillmor
  4 siblings, 0 replies; 12+ messages in thread
From: David Bremner @ 2020-06-30  1:14 UTC (permalink / raw)
  To: notmuch

This allows us to return an error value from the library.
---
 lib/message.cc         | 7 ++++++-
 test/T560-lib-error.sh | 1 -
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index b7a64b1c..3ca7b902 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -604,7 +604,12 @@ _notmuch_message_get_in_reply_to (notmuch_message_t *message)
 const char *
 notmuch_message_get_thread_id (notmuch_message_t *message)
 {
-    _notmuch_message_ensure_metadata (message, message->thread_id);
+    try {
+	_notmuch_message_ensure_metadata (message, message->thread_id);
+    } catch (Xapian::Error &error) {
+	LOG_XAPIAN_EXCEPTION (message, error);
+	return NULL;
+    }
     if (! message->thread_id)
 	INTERNAL_ERROR ("Message with document ID of %u has no thread ID.\n",
 			message->doc_id);
diff --git a/test/T560-lib-error.sh b/test/T560-lib-error.sh
index b5600851..81500536 100755
--- a/test/T560-lib-error.sh
+++ b/test/T560-lib-error.sh
@@ -360,7 +360,6 @@ test_expect_equal_file EXPECTED OUTPUT
 
 backup_database
 test_begin_subtest "Handle getting thread-id from closed database"
-test_subtest_known_broken
 cat c_head2 - c_tail <<'EOF' | test_C ${MAIL_DIR}
     {
         const char *id2;
-- 
2.27.0

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

* Re: introduce exception handling at top level of libnotmuch
  2020-06-30  1:14 introduce exception handling at top level of libnotmuch David Bremner
                   ` (3 preceding siblings ...)
  2020-06-30  1:14 ` [PATCH 4/4] lib/message: catch exception in n_m_get_thread_id David Bremner
@ 2020-07-02 19:16 ` Daniel Kahn Gillmor
  2020-07-04  0:15   ` David Bremner
  4 siblings, 1 reply; 12+ messages in thread
From: Daniel Kahn Gillmor @ 2020-07-02 19:16 UTC (permalink / raw)
  To: David Bremner, notmuch


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

Hi Bremner, all--

On Mon 2020-06-29 22:14:07 -0300, David Bremner wrote:
> I know that some of you are not C++ fans, but at the moment this is
> the cleanest fix I can think of to uncaught xapian exceptions causing
> calls to the library to die. Floris reminded me of this recently with
> the discussion about operations on closed databases, but cleaning up
> the handling of exceptions in libnotmuch has been on my mind for a
> while. It will be bit laborious so I did a few functions for
> discussion purposes before getting too carried away.

I've read through the series and it looks reasonable to me.

I've also tested them, and they behave as expected.

If someone has a more nuanced approach to dealing with some of the
subtle exceptions that might be raised, i'd be happy to see those
approaches laid on top of this series.

> There is still a certain amount of boilerplate with more or less
> identical try/catch blocks (yes, I really miss scheme macros here). I
> could mostly eliminate that with C++11 lambdas, but I wasn't sure the
> result was more maintainable or nicer.

I think this looks fine, and it isn't a huge amount of boilerplate.

Please merge.

  --dkg

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

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

_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org

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

* Re: introduce exception handling at top level of libnotmuch
  2020-07-02 19:16 ` introduce exception handling at top level of libnotmuch Daniel Kahn Gillmor
@ 2020-07-04  0:15   ` David Bremner
  0 siblings, 0 replies; 12+ messages in thread
From: David Bremner @ 2020-07-04  0:15 UTC (permalink / raw)
  To: Daniel Kahn Gillmor, notmuch

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

>
> I think this looks fine, and it isn't a huge amount of boilerplate.
>
> Please merge.
>
>   --dkg

OK, merged to master, thanks for the review.

d
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org

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

* Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id
  2020-06-30  1:14 ` [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id David Bremner
@ 2020-07-04 15:44   ` Floris Bruynooghe
  2020-07-04 17:17     ` David Bremner
  0 siblings, 1 reply; 12+ messages in thread
From: Floris Bruynooghe @ 2020-07-04 15:44 UTC (permalink / raw)
  To: David Bremner, notmuch

Nice.

On Mon 29 Jun 2020 at 22:14 -0300, David Bremner wrote:
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index ceb5a018..0dc89547 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -1363,9 +1363,8 @@ notmuch_message_get_database (const notmuch_message_t *message);
>   * message is valid, (which is until the query from which it derived
>   * is destroyed).
>   *
> - * This function will not return NULL since Notmuch ensures that every
> - * message has a unique message ID, (Notmuch will generate an ID for a
> - * message if the original file does not contain one).
> + * This function will return NULL if triggers an unhandled Xapian
> + * exception.
>   */
>  const char *
>  notmuch_message_get_message_id (notmuch_message_t *message);

How much of a departure from the existing API is this?  Will this be
possible with all functions?  I had a quick look and tried some other
functions that don't return notmuch_status_t:

notmuch_database_get_version currently returns and unsigned int and
segfaults on use with a closed db.

notmuch_needs_upgrade returns notmuch_bool_t and seems to return a valid
bool with a closed db.  I'm not sure if this is always the right answer
though.

I wonder if a backwards-compatible errno-style API could work,
notmuch_last_status(notmuch_database_t* database) or so.  This kind of
thing is probably easy to adopt in bindings but harder for direct users
of the API.  It's also an extra API call for everything that doesn't
return notmuch_status_t.  But I'll leave the judgement to you, I'm not
as experienced with the API.

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

* Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id
  2020-07-04 15:44   ` Floris Bruynooghe
@ 2020-07-04 17:17     ` David Bremner
  2020-07-05 11:17       ` David Bremner
  2020-07-08 19:52       ` Floris Bruynooghe
  0 siblings, 2 replies; 12+ messages in thread
From: David Bremner @ 2020-07-04 17:17 UTC (permalink / raw)
  To: Floris Bruynooghe, notmuch

Floris Bruynooghe <flub@devork.be> writes:


>> - * This function will not return NULL since Notmuch ensures that every
>> - * message has a unique message ID, (Notmuch will generate an ID for a
>> - * message if the original file does not contain one).
>> + * This function will return NULL if triggers an unhandled Xapian
>> + * exception.

> How much of a departure from the existing API is this?  Will this be
> possible with all functions?  I had a quick look and tried some other
> functions that don't return notmuch_status_t:

It's upward compatible in that any code which crashes because it was not
expecting a NULL pointer, will already be crashing in the same
circumstances because of an uncaught exception / call to abort.

> notmuch_database_get_version currently returns and unsigned int and
> segfaults on use with a closed db.

Yes, the ones without a proper status value are going to be a bit work.

In the next series I just posted [1], I started providing status value
returning version (see notmuch_message_get_flag_st). We've been through
a few of these migrations and it has not been too painful.

> I wonder if a backwards-compatible errno-style API could work,
> notmuch_last_status(notmuch_database_t* database) or so.  This kind of
> thing is probably easy to adopt in bindings but harder for direct users
> of the API.  It's also an extra API call for everything that doesn't
> return notmuch_status_t.  But I'll leave the judgement to you, I'm not
> as experienced with the API.

I think my main objection to this is that there is no out-of-band value
to tell the caller they need to check errno. So basically every call to
to one of the relevant functions would need be followed by a call to
checking the error number. I don't think that's less work than switching
to a new API. Of course it's less work for me, and we already sort of
made that choice with notmuch_database_status_string. In that case it
was a matter of changing the entire API.  Here we're talking about 10
functions, and I'm not sure if they all need to be changed. For example
several of the notmuch_foo_valid functions just check pointers for being
NULL and can't generate I/O or exceptions.

d

[1]: id:20200704151805.3717715-1-david@tethera.net

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

* Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id
  2020-07-04 17:17     ` David Bremner
@ 2020-07-05 11:17       ` David Bremner
  2020-07-08 19:55         ` Floris Bruynooghe
  2020-07-08 19:52       ` Floris Bruynooghe
  1 sibling, 1 reply; 12+ messages in thread
From: David Bremner @ 2020-07-05 11:17 UTC (permalink / raw)
  To: Floris Bruynooghe, notmuch

David Bremner <david@tethera.net> writes:

> Floris Bruynooghe <flub@devork.be> writes:
>
>> notmuch_database_get_version currently returns and unsigned int and
>> segfaults on use with a closed db.
>
> Yes, the ones without a proper status value are going to be a bit work.
>
> In the next series I just posted [1], I started providing status value
> returning version (see notmuch_message_get_flag_st). We've been through
> a few of these migrations and it has not been too painful.
>

I thought of another variation for the boolean valued functions. We
could embed the boolean values in the notmuch_status_t value by adding
one or more new status values corresponding to TRUE and FALSE. I'm not
sure if that would be much simpler, but it would avoid the use of output
parameters.

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

* Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id
  2020-07-04 17:17     ` David Bremner
  2020-07-05 11:17       ` David Bremner
@ 2020-07-08 19:52       ` Floris Bruynooghe
  1 sibling, 0 replies; 12+ messages in thread
From: Floris Bruynooghe @ 2020-07-08 19:52 UTC (permalink / raw)
  To: David Bremner, notmuch

On Sat 04 Jul 2020 at 14:17 -0300, David Bremner wrote:

> Floris Bruynooghe <flub@devork.be> writes:
>
>
>>> - * This function will not return NULL since Notmuch ensures that every
>>> - * message has a unique message ID, (Notmuch will generate an ID for a
>>> - * message if the original file does not contain one).
>>> + * This function will return NULL if triggers an unhandled Xapian
>>> + * exception.
>
>> How much of a departure from the existing API is this?  Will this be
>> possible with all functions?  I had a quick look and tried some other
>> functions that don't return notmuch_status_t:
>
> It's upward compatible in that any code which crashes because it was not
> expecting a NULL pointer, will already be crashing in the same
> circumstances because of an uncaught exception / call to abort.

Oh yes, that is a very good point.  This choice seems very reason then.


Cheers,
Floris

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

* Re: [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id
  2020-07-05 11:17       ` David Bremner
@ 2020-07-08 19:55         ` Floris Bruynooghe
  0 siblings, 0 replies; 12+ messages in thread
From: Floris Bruynooghe @ 2020-07-08 19:55 UTC (permalink / raw)
  To: David Bremner, notmuch

On Sun 05 Jul 2020 at 08:17 -0300, David Bremner wrote:

> David Bremner <david@tethera.net> writes:
>
>> Floris Bruynooghe <flub@devork.be> writes:
>>
>>> notmuch_database_get_version currently returns and unsigned int and
>>> segfaults on use with a closed db.
>>
>> Yes, the ones without a proper status value are going to be a bit work.
>>
>> In the next series I just posted [1], I started providing status value
>> returning version (see notmuch_message_get_flag_st). We've been through
>> a few of these migrations and it has not been too painful.
>>
>
> I thought of another variation for the boolean valued functions. We
> could embed the boolean values in the notmuch_status_t value by adding
> one or more new status values corresponding to TRUE and FALSE. I'm not
> sure if that would be much simpler, but it would avoid the use of output
> parameters.

This also seems very reasonable.

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

end of thread, other threads:[~2020-07-08 19:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-30  1:14 introduce exception handling at top level of libnotmuch David Bremner
2020-06-30  1:14 ` [PATCH 1/4] test: add known broken test for error handling on closed database David Bremner
2020-06-30  1:14 ` [PATCH 2/4] lib: catch error from closed db in n_m_get_message_id David Bremner
2020-07-04 15:44   ` Floris Bruynooghe
2020-07-04 17:17     ` David Bremner
2020-07-05 11:17       ` David Bremner
2020-07-08 19:55         ` Floris Bruynooghe
2020-07-08 19:52       ` Floris Bruynooghe
2020-06-30  1:14 ` [PATCH 3/4] test: add known broken test for n_m_get_thread_id on closed db David Bremner
2020-06-30  1:14 ` [PATCH 4/4] lib/message: catch exception in n_m_get_thread_id David Bremner
2020-07-02 19:16 ` introduce exception handling at top level of libnotmuch Daniel Kahn Gillmor
2020-07-04  0:15   ` 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).