On Fri, 24 Feb 2017, David Bremner <david@tethera.net> wrote: > 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 (like running out > of memory or disk corruption). I think the sanity of the implementation hinges on that assumption. It makes sense if you're right, but I really have no idea either way... > --- > lib/message.cc | 152 ++++++++++++++++++++++++----------------- > test/T640-database-modified.sh | 1 - > 2 files changed, 88 insertions(+), 65 deletions(-) > > diff --git a/lib/message.cc b/lib/message.cc > index 2fb67d85..15e2f528 100644 > --- 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,8 @@ static void > _notmuch_message_ensure_metadata (notmuch_message_t *message) > { > Xapian::TermIterator i, end; > + notmuch_bool_t success = FALSE; > + > const char *thread_prefix = _find_prefix ("thread"), > *tag_prefix = _find_prefix ("tag"), > *id_prefix = _find_prefix ("id"), > @@ -327,73 +335,89 @@ _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 && !success; 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 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 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 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); > + > + > + /* 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 */ > + success = TRUE; Nitpick, if you don't intend to use that variable to return status from the function, you can just break here, and get rid of the variable. But no big deal. > + } 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)); > + success = FALSE; > + } catch (const Xapian::Error &error) { > + INTERNAL_ERROR ("A Xapian exception occurred fetching message metadata: %s\n", > + error.get_msg().c_str()); > + } If the assumption is that these really are rare cases (read: shouldn't happen), INTERNAL_ERROR is an improvement over leaking the exception. Otherwise, I think we'd need to propagate the status all the way to the API, which would really be annoying. I guess I think this is a worthwhile improvement no matter what. > } > - > - /* 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); > - > - > - /* 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, ""); > + message->last_view = message->notmuch->view; > } > > void > diff --git a/test/T640-database-modified.sh b/test/T640-database-modified.sh > index 41869eaa..9599417c 100755 > --- a/test/T640-database-modified.sh > +++ b/test/T640-database-modified.sh > @@ -5,7 +5,6 @@ test_description="DatabaseModifiedError handling" > add_email_corpus > > test_begin_subtest "catching DatabaseModifiedError in _notmuch_message_ensure_metadata" > -test_subtest_known_broken > test_C ${MAIL_DIR} <<'EOF' > #include <unistd.h> > #include <stdlib.h> > -- > 2.11.0 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch