From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id 247186DE1F48 for ; Sat, 25 Feb 2017 10:42:29 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: -0.185 X-Spam-Level: X-Spam-Status: No, score=-0.185 tagged_above=-999 required=5 tests=[AWL=-0.165, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01] autolearn=disabled Received: from arlo.cworth.org ([127.0.0.1]) by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id D4acCvmGsUsU for ; Sat, 25 Feb 2017 10:42:28 -0800 (PST) Received: from mail-lf0-f67.google.com (mail-lf0-f67.google.com [209.85.215.67]) by arlo.cworth.org (Postfix) with ESMTPS id E0E356DE1F47 for ; Sat, 25 Feb 2017 10:42:27 -0800 (PST) Received: by mail-lf0-f67.google.com with SMTP id p197so3513616lfp.3 for ; Sat, 25 Feb 2017 10:42:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nikula-org.20150623.gappssmtp.com; s=20150623; h=from:to:subject:in-reply-to:references:date:message-id:mime-version; bh=LdXhYJ+FpqprutfqNB4/OtPmlHRrlwaFzWg+f99gGos=; b=pTWsogmUn1j3ph2zDvzyFukhrrHKlvCoU0VT0goG1AqW3Vp8k/ppkf4yj53v9ER7vJ dnfCyUsdBxcO9QvtOvzUo3125fU0wUt1q95KFmWyruRQ6V3BnywT+9fCj0oFsS2gtYMJ MwWZVhatfKUJmr4WlUVxWu8lzi6MuOeenFV2ugYf0sCy2Q0j6lRk+IGUuGlD2Yd+ivv2 SlcBh0SwoDHxLxqh16eyYKJ4koYc3rh5MyEk08zkySOCinFtjWBURrKLQG9JSTWnoiiB hWCu11/BsHmpxFHiKW86IbGgl74qXSJBf94xa26zDuluOJUxQ5Pt1I5FTDrJaIHZbkET Mjrg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=LdXhYJ+FpqprutfqNB4/OtPmlHRrlwaFzWg+f99gGos=; b=FyJKITm8qkczKPpevSgCqTqKbQCHqOe8D+ETCZRZCI8Emk/RGlsJqr/Q7flmQs8eVr 0NlzhAgzVg82mZBWDElFuIhcTXHdhD9+zueEOOgRCm3H5rUmVTlWXmdYhLYc8AUgLr09 Bb0wCYyAEN9ByG/8igWcJitHVkJ1uafKSkmGdLUbcICXkbVicu195URD6eUOBgc/MLYI viC5ei0R6tqepXF+J/5kUqjG11wpmREtpwCK0wpv8PmXr3H32dcmt2zuTi+F+5tzglfF h+DKH7dtdXxY1TGXO4QTjmjzSP3hKHNA0AI0jMjJVoez6gM8KEVo4zSgeIIwX2XJiv6C /1Ow== X-Gm-Message-State: AMke39n3p+oZUTxvokLCJ3+Qd3bAQ3t/s+1QWWLb5vaITP/EaIqt689/nHnwPB8z3O3xfA== X-Received: by 10.46.6.9 with SMTP id 9mr2222403ljg.3.1488048146188; Sat, 25 Feb 2017 10:42:26 -0800 (PST) Received: from localhost (mobile-access-bcee80-14.dhcp.inet.fi. [188.238.128.14]) by smtp.gmail.com with ESMTPSA id f9sm9422798ljb.20.2017.02.25.10.42.25 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 25 Feb 2017 10:42:25 -0800 (PST) From: Jani Nikula To: David Bremner , notmuch@notmuchmail.org, eg@gaute.vetsj.com Subject: Re: [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata In-Reply-To: <20170225034513.19427-4-david@tethera.net> References: <20170225034513.19427-1-david@tethera.net> <20170225034513.19427-4-david@tethera.net> Date: Sat, 25 Feb 2017 20:42:24 +0200 Message-ID: <87poi66rjj.fsf@nikula.org> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 25 Feb 2017 18:42:29 -0000 On Fri, 24 Feb 2017, David Bremner 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 > #include > -- > 2.11.0 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch