lib: handle DatabaseModifiedError in _n_message_ensure_metadata

The retries are hardcoded to a small number, and error handling aborts
than propagating errors from notmuch_database_reopen. These are both
somewhat justified by the assumption that most things that can go
wrong in Xapian::Database::reopen are rare and fatal. Here's the brief
discussion with Xapian upstream:

   24-02-2017 08:12:57 < bremner> any intuition about how likely
      Xapian::Database::reopen is to fail? I'm catching a
      DatabaseModifiedError somewhere where handling any further errors is
      tricky, and wondering about treating a failed reopen as as "the
      impossible happened, stopping"

   24-02-2017 16:22:34 < olly> bremner: there should not be much scope for
    failure - stuff like out of memory or disk errors, which are probably a
    good enough excuse to stop

diff --git a/lib/message.cc b/lib/message.cc
index 2fb67d85dc283bbca94b30d7831f01a98048e701..9bafff0bb3c83fa67df50176901cfe66070568ca 100644 (file)
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -49,6 +49,9 @@ struct visible _notmuch_message {
     /* Message document modified since last sync */
     notmuch_bool_t modified;
 
+    /* last view of database the struct is synced with */
+    unsigned long last_view;
+
     Xapian::Document doc;
     Xapian::termcount termpos;
 };
@@ -110,6 +113,9 @@ _notmuch_message_create_for_document (const void *talloc_owner,
     message->flags = 0;
     message->lazy_flags = 0;
 
+    /* the message is initially not synchronized with Xapian */
+    message->last_view = 0;
+
     /* Each of these will be lazily created as needed. */
     message->message_id = NULL;
     message->thread_id = NULL;
@@ -314,6 +320,7 @@ static void
 _notmuch_message_ensure_metadata (notmuch_message_t *message)
 {
     Xapian::TermIterator i, end;
+
     const char *thread_prefix = _find_prefix ("thread"),
        *tag_prefix = _find_prefix ("tag"),
        *id_prefix = _find_prefix ("id"),
@@ -327,73 +334,88 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message)
      * slightly more costly than looking up individual fields if only
      * one field of the message object is actually used, it's a huge
      * win as more fields are used. */
+    for (int count=0; count < 3; count++) {
+       try {
+           i = message->doc.termlist_begin ();
+           end = message->doc.termlist_end ();
+
+           /* Get thread */
+           if (!message->thread_id)
+               message->thread_id =
+                   _notmuch_message_get_term (message, i, end, thread_prefix);
+
+           /* Get tags */
+           assert (strcmp (thread_prefix, tag_prefix) < 0);
+           if (!message->tag_list) {
+               message->tag_list =
+                   _notmuch_database_get_terms_with_prefix (message, i, end,
+                                                            tag_prefix);
+               _notmuch_string_list_sort (message->tag_list);
+           }
 
-    i = message->doc.termlist_begin ();
-    end = message->doc.termlist_end ();
+           /* Get id */
+           assert (strcmp (tag_prefix, id_prefix) < 0);
+           if (!message->message_id)
+               message->message_id =
+                   _notmuch_message_get_term (message, i, end, id_prefix);
+
+           /* Get document type */
+           assert (strcmp (id_prefix, type_prefix) < 0);
+           if (! NOTMUCH_TEST_BIT (message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST)) {
+               i.skip_to (type_prefix);
+               /* "T" is the prefix "type" fields.  See
+                * BOOLEAN_PREFIX_INTERNAL. */
+               if (*i == "Tmail")
+                   NOTMUCH_CLEAR_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+               else if (*i == "Tghost")
+                   NOTMUCH_SET_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+               else
+                   INTERNAL_ERROR ("Message without type term");
+               NOTMUCH_SET_BIT (&message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST);
+           }
 
-    /* Get thread */
-    if (!message->thread_id)
-       message->thread_id =
-           _notmuch_message_get_term (message, i, end, thread_prefix);
-
-    /* Get tags */
-    assert (strcmp (thread_prefix, tag_prefix) < 0);
-    if (!message->tag_list) {
-       message->tag_list =
-           _notmuch_database_get_terms_with_prefix (message, i, end,
-                                                    tag_prefix);
-       _notmuch_string_list_sort (message->tag_list);
-    }
+           /* Get filename list.  Here we get only the terms.  We lazily
+            * expand them to full file names when needed in
+            * _notmuch_message_ensure_filename_list. */
+           assert (strcmp (type_prefix, filename_prefix) < 0);
+           if (!message->filename_term_list && !message->filename_list)
+               message->filename_term_list =
+                   _notmuch_database_get_terms_with_prefix (message, i, end,
+                                                            filename_prefix);
 
-    /* Get id */
-    assert (strcmp (tag_prefix, id_prefix) < 0);
-    if (!message->message_id)
-       message->message_id =
-           _notmuch_message_get_term (message, i, end, id_prefix);
-
-    /* Get document type */
-    assert (strcmp (id_prefix, type_prefix) < 0);
-    if (! NOTMUCH_TEST_BIT (message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST)) {
-       i.skip_to (type_prefix);
-       /* "T" is the prefix "type" fields.  See
-        * BOOLEAN_PREFIX_INTERNAL. */
-       if (*i == "Tmail")
-           NOTMUCH_CLEAR_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
-       else if (*i == "Tghost")
-           NOTMUCH_SET_BIT (&message->flags, NOTMUCH_MESSAGE_FLAG_GHOST);
-       else
-           INTERNAL_ERROR ("Message without type term");
-       NOTMUCH_SET_BIT (&message->lazy_flags, NOTMUCH_MESSAGE_FLAG_GHOST);
-    }
 
-    /* Get filename list.  Here we get only the terms.  We lazily
-     * expand them to full file names when needed in
-     * _notmuch_message_ensure_filename_list. */
-    assert (strcmp (type_prefix, filename_prefix) < 0);
-    if (!message->filename_term_list && !message->filename_list)
-       message->filename_term_list =
-           _notmuch_database_get_terms_with_prefix (message, i, end,
-                                                    filename_prefix);
-
-
-    /* Get property terms. Mimic the setup with filenames above */
-    assert (strcmp (filename_prefix, property_prefix) < 0);
-    if (!message->property_map && !message->property_term_list)
-       message->property_term_list =
-           _notmuch_database_get_terms_with_prefix (message, i, end,
-                                                    property_prefix);
-
-    /* Get reply to */
-    assert (strcmp (property_prefix, replyto_prefix) < 0);
-    if (!message->in_reply_to)
-       message->in_reply_to =
-           _notmuch_message_get_term (message, i, end, replyto_prefix);
+           /* Get property terms. Mimic the setup with filenames above */
+           assert (strcmp (filename_prefix, property_prefix) < 0);
+           if (!message->property_map && !message->property_term_list)
+               message->property_term_list =
+                   _notmuch_database_get_terms_with_prefix (message, i, end,
+                                                        property_prefix);
 
+           /* Get reply to */
+           assert (strcmp (property_prefix, replyto_prefix) < 0);
+           if (!message->in_reply_to)
+               message->in_reply_to =
+                   _notmuch_message_get_term (message, i, end, replyto_prefix);
 
-    /* It's perfectly valid for a message to have no In-Reply-To
-     * header. For these cases, we return an empty string. */
-    if (!message->in_reply_to)
-       message->in_reply_to = talloc_strdup (message, "");
+
+           /* It's perfectly valid for a message to have no In-Reply-To
+            * header. For these cases, we return an empty string. */
+           if (!message->in_reply_to)
+               message->in_reply_to = talloc_strdup (message, "");
+
+           /* all the way without an exception */
+           break;
+       } catch (const Xapian::DatabaseModifiedError &error) {
+           notmuch_status_t status = _notmuch_database_reopen (message->notmuch);
+           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;
 }
 
 void

diff --git a/test/T640-database-modified.sh b/test/T640-database-modified.sh
index b5b3bc2bd440b13c54267970aac40a691a0bf6b4..516836b0c91e192c2b6c8cb1c9f30b7a6bb95697 100755 (executable)
--- a/test/T640-database-modified.sh
+++ b/test/T640-database-modified.sh
@@ -6,7 +6,6 @@ test_description="DatabaseModifiedError handling"
 add_email_corpus
 
 test_begin_subtest "catching DatabaseModifiedError in _notmuch_message_ensure_metadata"
-test_subtest_known_broken
 # it seems to need to be an early document to trigger the exception
 first_id=$(notmuch search --output=messages '*'| head -1 | sed s/^id://)