Re: [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata

[Jani Nikula <jani@nikula.org>]

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