On Fri, 24 Feb 2017, David Bremner <[email protected]> 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
> [email protected]
> https://notmuchmail.org/mailman/listinfo/notmuch
_______________________________________________
notmuch mailing list
[email protected]
https://notmuchmail.org/mailman/listinfo/notmuch
