* v1 _notmuch_message_ensure_metadata exception handling
@ 2017-02-25 3:45 David Bremner
2017-02-25 3:45 ` [PATCH 1/4] test: add known broken test for uncaught DatabaseModifiedError David Bremner
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: David Bremner @ 2017-02-25 3:45 UTC (permalink / raw)
To: notmuch, eg
In id:1487583702.5ghl7kdkaw.astroid@strange.none Gaute provided a test
case that demonstrated an uncaught Xapian::DatabaseModifiedError in
_notmuch_message_ensure_metadata.
Starting with id:20170218144551.22925-2-david@tethera.net, I provided
one approach to dealing with this via error propagation. I believe the
current series is a better approach because it actually fixes the
problem (i.e. re-opens the database), and because it takes less code.
There are probably plenty of other places where the new
notmuch_database_open needs to be called.
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/4] test: add known broken test for uncaught DatabaseModifiedError 2017-02-25 3:45 v1 _notmuch_message_ensure_metadata exception handling David Bremner @ 2017-02-25 3:45 ` David Bremner 2017-02-25 18:49 ` Jani Nikula 2017-02-25 3:45 ` [PATCH 2/4] lib: add notmuch_database_reopen David Bremner ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: David Bremner @ 2017-02-25 3:45 UTC (permalink / raw) To: notmuch, eg There are several of these to track down, but one that is in quite a few code paths is _notmuch_message_ensure_metadata. --- test/T640-database-modified.sh | 60 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100755 test/T640-database-modified.sh diff --git a/test/T640-database-modified.sh b/test/T640-database-modified.sh new file mode 100755 index 00000000..41869eaa --- /dev/null +++ b/test/T640-database-modified.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +test_description="DatabaseModifiedError handling" +. ./test-lib.sh || exit 1 + +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> +#include <notmuch-test.h> +#include <talloc.h> +int +main (int argc, char **argv) +{ + pid_t child; + const char *path = argv[1]; + + notmuch_database_t *rw_db, *ro_db; + notmuch_messages_t *messages; + notmuch_message_t *message, *ro_message; + notmuch_query_t *query; + notmuch_tags_t *tags; + + EXPECT0 (notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_ONLY, &ro_db)); + EXPECT0 (notmuch_database_find_message (ro_db, "4EFC743A.3060609@april.org", &ro_message)); + + EXPECT0 (notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &rw_db)); + query = notmuch_query_create(rw_db, ""); + EXPECT0 (notmuch_query_search_messages_st (query, &messages)); + + for (int count=0; + notmuch_messages_valid (messages); + notmuch_messages_move_to_next (messages), count++) { + message = notmuch_messages_get (messages); + for (int i=0; i<200; i++) { + char *tag_str = talloc_asprintf(rw_db, "%d", i); + EXPECT0 (notmuch_message_add_tag (message, tag_str)); + talloc_free (tag_str); + } + } + + notmuch_database_close (rw_db); + + tags = notmuch_message_get_tags (ro_message); + if (tags) + printf("SUCCESS\n"); + return 0; +} +EOF + +cat <<'EOF' >EXPECTED +== stdout == +SUCCESS +== stderr == +EOF +test_expect_equal_file EXPECTED OUTPUT + +test_done -- 2.11.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] test: add known broken test for uncaught DatabaseModifiedError 2017-02-25 3:45 ` [PATCH 1/4] test: add known broken test for uncaught DatabaseModifiedError David Bremner @ 2017-02-25 18:49 ` Jani Nikula 2017-02-25 20:08 ` David Bremner 0 siblings, 1 reply; 13+ messages in thread From: Jani Nikula @ 2017-02-25 18:49 UTC (permalink / raw) To: David Bremner, notmuch, eg On Fri, 24 Feb 2017, David Bremner <david@tethera.net> wrote: > There are several of these to track down, but one that is in quite a > few code paths is _notmuch_message_ensure_metadata. > --- > test/T640-database-modified.sh | 60 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > create mode 100755 test/T640-database-modified.sh > > diff --git a/test/T640-database-modified.sh b/test/T640-database-modified.sh > new file mode 100755 > index 00000000..41869eaa > --- /dev/null > +++ b/test/T640-database-modified.sh > @@ -0,0 +1,60 @@ > +#!/usr/bin/env bash > +test_description="DatabaseModifiedError handling" > +. ./test-lib.sh || exit 1 > + > +add_email_corpus Is the whole corpus necessary? Just add a single message? > + > +test_begin_subtest "catching DatabaseModifiedError in _notmuch_message_ensure_metadata" > +test_subtest_known_broken > +test_C ${MAIL_DIR} <<'EOF' I'm not entirely thrilled by the whole test_C thing, but I guess I've lost that battle... :/ > +#include <unistd.h> > +#include <stdlib.h> > +#include <notmuch-test.h> > +#include <talloc.h> > +int > +main (int argc, char **argv) > +{ > + pid_t child; Leftover variable? > + const char *path = argv[1]; > + > + notmuch_database_t *rw_db, *ro_db; > + notmuch_messages_t *messages; > + notmuch_message_t *message, *ro_message; > + notmuch_query_t *query; > + notmuch_tags_t *tags; > + > + EXPECT0 (notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_ONLY, &ro_db)); > + EXPECT0 (notmuch_database_find_message (ro_db, "4EFC743A.3060609@april.org", &ro_message)); How about add_message with a specific message-id, and using that here? I think we rely too much on the test corpus and hard-code stuff from there too much. Otherwise, LGTM. > + > + EXPECT0 (notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &rw_db)); > + query = notmuch_query_create(rw_db, ""); > + EXPECT0 (notmuch_query_search_messages_st (query, &messages)); > + > + for (int count=0; > + notmuch_messages_valid (messages); > + notmuch_messages_move_to_next (messages), count++) { > + message = notmuch_messages_get (messages); > + for (int i=0; i<200; i++) { > + char *tag_str = talloc_asprintf(rw_db, "%d", i); > + EXPECT0 (notmuch_message_add_tag (message, tag_str)); > + talloc_free (tag_str); > + } > + } > + > + notmuch_database_close (rw_db); > + > + tags = notmuch_message_get_tags (ro_message); > + if (tags) > + printf("SUCCESS\n"); > + return 0; > +} > +EOF > + > +cat <<'EOF' >EXPECTED > +== stdout == > +SUCCESS > +== stderr == > +EOF > +test_expect_equal_file EXPECTED OUTPUT > + > +test_done > -- > 2.11.0 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] test: add known broken test for uncaught DatabaseModifiedError 2017-02-25 18:49 ` Jani Nikula @ 2017-02-25 20:08 ` David Bremner 0 siblings, 0 replies; 13+ messages in thread From: David Bremner @ 2017-02-25 20:08 UTC (permalink / raw) To: Jani Nikula, notmuch, eg Jani Nikula <jani@nikula.org> writes: >> + >> +add_email_corpus > > Is the whole corpus necessary? Just add a single message? Good point, that's actually leftover from a previous failed attempt. >> +test_begin_subtest "catching DatabaseModifiedError in _notmuch_message_ensure_metadata" >> +test_subtest_known_broken >> +test_C ${MAIL_DIR} <<'EOF' > > I'm not entirely thrilled by the whole test_C thing, but I guess I've > lost that battle... :/ > >> +#include <unistd.h> >> +#include <stdlib.h> >> +#include <notmuch-test.h> >> +#include <talloc.h> >> +int >> +main (int argc, char **argv) >> +{ >> + pid_t child; > > Leftover variable? correct. > How about add_message with a specific message-id, and using that here? I > think we rely too much on the test corpus and hard-code stuff from there > too much. Good idea. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] lib: add notmuch_database_reopen 2017-02-25 3:45 v1 _notmuch_message_ensure_metadata exception handling David Bremner 2017-02-25 3:45 ` [PATCH 1/4] test: add known broken test for uncaught DatabaseModifiedError David Bremner @ 2017-02-25 3:45 ` David Bremner 2017-02-25 18:33 ` Jani Nikula 2017-02-25 3:45 ` [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata David Bremner 2017-02-25 3:45 ` [PATCH 4/4] lib/message.cc: use view number to invalidate cached metadata David Bremner 3 siblings, 1 reply; 13+ messages in thread From: David Bremner @ 2017-02-25 3:45 UTC (permalink / raw) To: notmuch, eg The main expected use is to recover from a Xapian::DatabaseChanged exception. --- lib/database-private.h | 4 ++++ lib/database.cc | 23 +++++++++++++++++++++++ lib/notmuch.h | 12 ++++++++++++ 3 files changed, 39 insertions(+) diff --git a/lib/database-private.h b/lib/database-private.h index 2fb60f5e..06882439 100644 --- a/lib/database-private.h +++ b/lib/database-private.h @@ -207,6 +207,10 @@ struct _notmuch_database { unsigned long revision; const char *uuid; + /* Keep track of the number of times the database has been re-opened + * (or other global invalidations of notmuch's caching) + */ + unsigned long view; Xapian::QueryParser *query_parser; Xapian::TermGenerator *term_gen; Xapian::ValueRangeProcessor *value_range_processor; diff --git a/lib/database.cc b/lib/database.cc index 386dcd17..1e958b65 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -951,6 +951,7 @@ notmuch_database_open_verbose (const char *path, notmuch->mode = mode; notmuch->atomic_nesting = 0; + notmuch->view = 1; try { string last_thread_id; string last_mod; @@ -1133,6 +1134,28 @@ notmuch_database_close (notmuch_database_t *notmuch) return status; } +notmuch_status_t +notmuch_database_reopen (notmuch_database_t *notmuch) +{ + if (notmuch->mode != NOTMUCH_DATABASE_MODE_READ_ONLY) + return NOTMUCH_STATUS_UNSUPPORTED_OPERATION; + + try { + notmuch->xapian_db->reopen (); + } catch (const Xapian::Error &error) { + if (! notmuch->exception_reported) { + _notmuch_database_log (notmuch, "Error: A Xapian exception reopening database: %s\n", + error.get_msg ().c_str ()); + notmuch->exception_reported = TRUE; + return NOTMUCH_STATUS_XAPIAN_EXCEPTION; + } + } + + notmuch->view++; + + return NOTMUCH_STATUS_SUCCESS; +} + static int unlink_cb (const char *path, unused (const struct stat *sb), diff --git a/lib/notmuch.h b/lib/notmuch.h index 16da8be9..e9ed01dd 100644 --- a/lib/notmuch.h +++ b/lib/notmuch.h @@ -363,6 +363,18 @@ notmuch_status_t notmuch_database_close (notmuch_database_t *database); /** + * Reopen a (read-only) database, synching the database view with that + * on disk. + * + * @returns + * - NOTMUCH_STATUS_UNSUPPORTED_OPERATION: database is not read only + * - NOTMUCH_STATUS_XAPIAN_EXCEPTION: a Xapian exception occured trying to + * re-open the database. + */ +notmuch_status_t +notmuch_database_reopen (notmuch_database_t *database); + +/** * A callback invoked by notmuch_database_compact to notify the user * of the progress of the compaction process. */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] lib: add notmuch_database_reopen 2017-02-25 3:45 ` [PATCH 2/4] lib: add notmuch_database_reopen David Bremner @ 2017-02-25 18:33 ` Jani Nikula 2017-02-25 20:11 ` David Bremner 0 siblings, 1 reply; 13+ messages in thread From: Jani Nikula @ 2017-02-25 18:33 UTC (permalink / raw) To: David Bremner, notmuch, eg On Fri, 24 Feb 2017, David Bremner <david@tethera.net> wrote: > The main expected use is to recover from a Xapian::DatabaseChanged > exception. I guess the main question here is if this should be exposed from the library or not. If the intention is to recover *within* the library, why add new API? > --- > lib/database-private.h | 4 ++++ > lib/database.cc | 23 +++++++++++++++++++++++ > lib/notmuch.h | 12 ++++++++++++ > 3 files changed, 39 insertions(+) > > diff --git a/lib/database-private.h b/lib/database-private.h > index 2fb60f5e..06882439 100644 > --- a/lib/database-private.h > +++ b/lib/database-private.h > @@ -207,6 +207,10 @@ struct _notmuch_database { > unsigned long revision; > const char *uuid; > > + /* Keep track of the number of times the database has been re-opened > + * (or other global invalidations of notmuch's caching) > + */ > + unsigned long view; > Xapian::QueryParser *query_parser; > Xapian::TermGenerator *term_gen; > Xapian::ValueRangeProcessor *value_range_processor; > diff --git a/lib/database.cc b/lib/database.cc > index 386dcd17..1e958b65 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -951,6 +951,7 @@ notmuch_database_open_verbose (const char *path, > > notmuch->mode = mode; > notmuch->atomic_nesting = 0; > + notmuch->view = 1; > try { > string last_thread_id; > string last_mod; > @@ -1133,6 +1134,28 @@ notmuch_database_close (notmuch_database_t *notmuch) > return status; > } > > +notmuch_status_t > +notmuch_database_reopen (notmuch_database_t *notmuch) > +{ > + if (notmuch->mode != NOTMUCH_DATABASE_MODE_READ_ONLY) > + return NOTMUCH_STATUS_UNSUPPORTED_OPERATION; > + > + try { > + notmuch->xapian_db->reopen (); > + } catch (const Xapian::Error &error) { > + if (! notmuch->exception_reported) { > + _notmuch_database_log (notmuch, "Error: A Xapian exception reopening database: %s\n", > + error.get_msg ().c_str ()); > + notmuch->exception_reported = TRUE; > + return NOTMUCH_STATUS_XAPIAN_EXCEPTION; > + } Move the above return out of the if block? > + } > + > + notmuch->view++; > + > + return NOTMUCH_STATUS_SUCCESS; > +} > + > static int > unlink_cb (const char *path, > unused (const struct stat *sb), > diff --git a/lib/notmuch.h b/lib/notmuch.h > index 16da8be9..e9ed01dd 100644 > --- a/lib/notmuch.h > +++ b/lib/notmuch.h > @@ -363,6 +363,18 @@ notmuch_status_t > notmuch_database_close (notmuch_database_t *database); > > /** > + * Reopen a (read-only) database, synching the database view with that > + * on disk. > + * > + * @returns > + * - NOTMUCH_STATUS_UNSUPPORTED_OPERATION: database is not read only > + * - NOTMUCH_STATUS_XAPIAN_EXCEPTION: a Xapian exception occured trying to > + * re-open the database. > + */ > +notmuch_status_t > +notmuch_database_reopen (notmuch_database_t *database); > + > +/** > * A callback invoked by notmuch_database_compact to notify the user > * of the progress of the compaction process. > */ > -- > 2.11.0 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] lib: add notmuch_database_reopen 2017-02-25 18:33 ` Jani Nikula @ 2017-02-25 20:11 ` David Bremner 0 siblings, 0 replies; 13+ messages in thread From: David Bremner @ 2017-02-25 20:11 UTC (permalink / raw) To: Jani Nikula, notmuch, eg Jani Nikula <jani@nikula.org> writes: > On Fri, 24 Feb 2017, David Bremner <david@tethera.net> wrote: >> The main expected use is to recover from a Xapian::DatabaseChanged >> exception. > > I guess the main question here is if this should be exposed from the > library or not. If the intention is to recover *within* the library, why > add new API? Yes that would work. Although as I mentioned on IRC, the API has been proposed a few times before. I don't mind either way, it could always be exposed later. In particular then I could feel less guilty about not yet providing any bindings. >> + notmuch->xapian_db->reopen (); >> + } catch (const Xapian::Error &error) { >> + if (! notmuch->exception_reported) { >> + _notmuch_database_log (notmuch, "Error: A Xapian exception reopening database: %s\n", >> + error.get_msg ().c_str ()); >> + notmuch->exception_reported = TRUE; >> + return NOTMUCH_STATUS_XAPIAN_EXCEPTION; >> + } > > Move the above return out of the if block? oh duh. good catch. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata 2017-02-25 3:45 v1 _notmuch_message_ensure_metadata exception handling David Bremner 2017-02-25 3:45 ` [PATCH 1/4] test: add known broken test for uncaught DatabaseModifiedError David Bremner 2017-02-25 3:45 ` [PATCH 2/4] lib: add notmuch_database_reopen David Bremner @ 2017-02-25 3:45 ` David Bremner 2017-02-25 18:42 ` Jani Nikula 2017-02-25 3:45 ` [PATCH 4/4] lib/message.cc: use view number to invalidate cached metadata David Bremner 3 siblings, 1 reply; 13+ messages in thread From: David Bremner @ 2017-02-25 3:45 UTC (permalink / raw) To: notmuch, eg 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). --- 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; + } 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()); + } } - - /* 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata 2017-02-25 3:45 ` [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata David Bremner @ 2017-02-25 18:42 ` Jani Nikula 2017-02-25 20:21 ` David Bremner 0 siblings, 1 reply; 13+ messages in thread From: Jani Nikula @ 2017-02-25 18:42 UTC (permalink / raw) To: David Bremner, notmuch, eg 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata 2017-02-25 18:42 ` Jani Nikula @ 2017-02-25 20:21 ` David Bremner 2017-02-25 20:39 ` Jani Nikula 0 siblings, 1 reply; 13+ messages in thread From: David Bremner @ 2017-02-25 20:21 UTC (permalink / raw) To: Jani Nikula, notmuch, eg Jani Nikula <jani@nikula.org> writes: > 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... That was my conclusion from talking to Olly (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 I could add that to the commit 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. > I think I have some kind of mental block about break and continue. But it could even be a goto, those I understand ;). > >> + } 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. Yeah, I had a go at that in the previous longer series, but I was not very happy with the (incomplete) results ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata 2017-02-25 20:21 ` David Bremner @ 2017-02-25 20:39 ` Jani Nikula 0 siblings, 0 replies; 13+ messages in thread From: Jani Nikula @ 2017-02-25 20:39 UTC (permalink / raw) To: David Bremner, notmuch, eg On Sat, 25 Feb 2017, David Bremner <david@tethera.net> wrote: > Jani Nikula <jani@nikula.org> writes: > >> 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... > > That was my conclusion from talking to Olly (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 > > I could add that to the commit message? Yes, please, it'll come in handy when the memory of the discussion has faded! (Like two weeks from now... ;) > >>> + >>> + /* 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. >> > > I think I have some kind of mental block about break and continue. But > it could even be a goto, those I understand ;). Heh, as I said, 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. > > Yeah, I had a go at that in the previous longer series, but I was not > very happy with the (incomplete) results Err, I'm sorry for not being clear, I meant that this patch *series* is a worthwhile improvement as-is, even if reopen did have common and unrecoverable failure modes (which it shouldn't). This series catches a previously uncaught exception, tries to do something sensible about it, and failing at that, causes an INTERNAL_ERROR. That's a huge improvement. If we end up seeing those errors later, we can reconsider the error propagation. BR, Jani. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] lib/message.cc: use view number to invalidate cached metadata 2017-02-25 3:45 v1 _notmuch_message_ensure_metadata exception handling David Bremner ` (2 preceding siblings ...) 2017-02-25 3:45 ` [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata David Bremner @ 2017-02-25 3:45 ` David Bremner 2017-02-25 18:44 ` Jani Nikula 3 siblings, 1 reply; 13+ messages in thread From: David Bremner @ 2017-02-25 3:45 UTC (permalink / raw) To: notmuch, eg Currently the view number is incremented by notmuch_database_reopen --- lib/message.cc | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/message.cc b/lib/message.cc index 15e2f528..bfb95917 100644 --- a/lib/message.cc +++ b/lib/message.cc @@ -317,11 +317,14 @@ _notmuch_message_get_term (notmuch_message_t *message, } static void -_notmuch_message_ensure_metadata (notmuch_message_t *message) +_notmuch_message_ensure_metadata (notmuch_message_t *message, void *field) { Xapian::TermIterator i, end; notmuch_bool_t success = FALSE; + if ((field != NULL) && (message->last_view >= message->notmuch->view)) + return; + const char *thread_prefix = _find_prefix ("thread"), *tag_prefix = _find_prefix ("tag"), *id_prefix = _find_prefix ("id"), @@ -472,8 +475,7 @@ _notmuch_message_get_doc_id (notmuch_message_t *message) const char * notmuch_message_get_message_id (notmuch_message_t *message) { - if (!message->message_id) - _notmuch_message_ensure_metadata (message); + _notmuch_message_ensure_metadata (message, message->message_id); if (!message->message_id) INTERNAL_ERROR ("Message with document ID of %u has no message ID.\n", message->doc_id); @@ -548,16 +550,14 @@ notmuch_message_get_header (notmuch_message_t *message, const char *header) const char * _notmuch_message_get_in_reply_to (notmuch_message_t *message) { - if (!message->in_reply_to) - _notmuch_message_ensure_metadata (message); + _notmuch_message_ensure_metadata (message, message->in_reply_to); return message->in_reply_to; } const char * notmuch_message_get_thread_id (notmuch_message_t *message) { - if (!message->thread_id) - _notmuch_message_ensure_metadata (message); + _notmuch_message_ensure_metadata (message, message->thread_id); if (!message->thread_id) INTERNAL_ERROR ("Message with document ID of %u has no thread ID.\n", message->doc_id); @@ -860,8 +860,7 @@ _notmuch_message_ensure_filename_list (notmuch_message_t *message) if (message->filename_list) return; - if (!message->filename_term_list) - _notmuch_message_ensure_metadata (message); + _notmuch_message_ensure_metadata (message, message->filename_term_list); message->filename_list = _notmuch_string_list_create (message); node = message->filename_term_list->head; @@ -955,7 +954,7 @@ notmuch_message_get_flag (notmuch_message_t *message, { if (flag == NOTMUCH_MESSAGE_FLAG_GHOST && ! NOTMUCH_TEST_BIT (message->lazy_flags, flag)) - _notmuch_message_ensure_metadata (message); + _notmuch_message_ensure_metadata (message, NULL); return NOTMUCH_TEST_BIT (message->flags, flag); } @@ -996,8 +995,7 @@ notmuch_message_get_tags (notmuch_message_t *message) { notmuch_tags_t *tags; - if (!message->tag_list) - _notmuch_message_ensure_metadata (message); + _notmuch_message_ensure_metadata (message, message->tag_list); tags = _notmuch_tags_create (message, message->tag_list); /* _notmuch_tags_create steals the reference to the tag_list, but @@ -1833,8 +1831,7 @@ _notmuch_message_ensure_property_map (notmuch_message_t *message) if (message->property_map) return; - if (!message->property_term_list) - _notmuch_message_ensure_metadata (message); + _notmuch_message_ensure_metadata (message, message->property_term_list); message->property_map = _notmuch_string_map_create (message); -- 2.11.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] lib/message.cc: use view number to invalidate cached metadata 2017-02-25 3:45 ` [PATCH 4/4] lib/message.cc: use view number to invalidate cached metadata David Bremner @ 2017-02-25 18:44 ` Jani Nikula 0 siblings, 0 replies; 13+ messages in thread From: Jani Nikula @ 2017-02-25 18:44 UTC (permalink / raw) To: David Bremner, notmuch, eg On Fri, 24 Feb 2017, David Bremner <david@tethera.net> wrote: > Currently the view number is incremented by notmuch_database_reopen > --- > lib/message.cc | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/lib/message.cc b/lib/message.cc > index 15e2f528..bfb95917 100644 > --- a/lib/message.cc > +++ b/lib/message.cc > @@ -317,11 +317,14 @@ _notmuch_message_get_term (notmuch_message_t *message, > } > > static void > -_notmuch_message_ensure_metadata (notmuch_message_t *message) > +_notmuch_message_ensure_metadata (notmuch_message_t *message, void *field) > { > Xapian::TermIterator i, end; > notmuch_bool_t success = FALSE; > > + if ((field != NULL) && (message->last_view >= message->notmuch->view)) Nitpick, excessive braces for my taste! > + return; > + > const char *thread_prefix = _find_prefix ("thread"), > *tag_prefix = _find_prefix ("tag"), > *id_prefix = _find_prefix ("id"), > @@ -472,8 +475,7 @@ _notmuch_message_get_doc_id (notmuch_message_t *message) > const char * > notmuch_message_get_message_id (notmuch_message_t *message) > { > - if (!message->message_id) > - _notmuch_message_ensure_metadata (message); > + _notmuch_message_ensure_metadata (message, message->message_id); > if (!message->message_id) Side note, looks like we don't consistently check the metadata we want is really available. All in all, LGTM. > INTERNAL_ERROR ("Message with document ID of %u has no message ID.\n", > message->doc_id); > @@ -548,16 +550,14 @@ notmuch_message_get_header (notmuch_message_t *message, const char *header) > const char * > _notmuch_message_get_in_reply_to (notmuch_message_t *message) > { > - if (!message->in_reply_to) > - _notmuch_message_ensure_metadata (message); > + _notmuch_message_ensure_metadata (message, message->in_reply_to); > return message->in_reply_to; > } > > const char * > notmuch_message_get_thread_id (notmuch_message_t *message) > { > - if (!message->thread_id) > - _notmuch_message_ensure_metadata (message); > + _notmuch_message_ensure_metadata (message, message->thread_id); > if (!message->thread_id) > INTERNAL_ERROR ("Message with document ID of %u has no thread ID.\n", > message->doc_id); > @@ -860,8 +860,7 @@ _notmuch_message_ensure_filename_list (notmuch_message_t *message) > if (message->filename_list) > return; > > - if (!message->filename_term_list) > - _notmuch_message_ensure_metadata (message); > + _notmuch_message_ensure_metadata (message, message->filename_term_list); > > message->filename_list = _notmuch_string_list_create (message); > node = message->filename_term_list->head; > @@ -955,7 +954,7 @@ notmuch_message_get_flag (notmuch_message_t *message, > { > if (flag == NOTMUCH_MESSAGE_FLAG_GHOST && > ! NOTMUCH_TEST_BIT (message->lazy_flags, flag)) > - _notmuch_message_ensure_metadata (message); > + _notmuch_message_ensure_metadata (message, NULL); > > return NOTMUCH_TEST_BIT (message->flags, flag); > } > @@ -996,8 +995,7 @@ notmuch_message_get_tags (notmuch_message_t *message) > { > notmuch_tags_t *tags; > > - if (!message->tag_list) > - _notmuch_message_ensure_metadata (message); > + _notmuch_message_ensure_metadata (message, message->tag_list); > > tags = _notmuch_tags_create (message, message->tag_list); > /* _notmuch_tags_create steals the reference to the tag_list, but > @@ -1833,8 +1831,7 @@ _notmuch_message_ensure_property_map (notmuch_message_t *message) > if (message->property_map) > return; > > - if (!message->property_term_list) > - _notmuch_message_ensure_metadata (message); > + _notmuch_message_ensure_metadata (message, message->property_term_list); > > message->property_map = _notmuch_string_map_create (message); > > -- > 2.11.0 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-02-25 20:39 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-25 3:45 v1 _notmuch_message_ensure_metadata exception handling David Bremner 2017-02-25 3:45 ` [PATCH 1/4] test: add known broken test for uncaught DatabaseModifiedError David Bremner 2017-02-25 18:49 ` Jani Nikula 2017-02-25 20:08 ` David Bremner 2017-02-25 3:45 ` [PATCH 2/4] lib: add notmuch_database_reopen David Bremner 2017-02-25 18:33 ` Jani Nikula 2017-02-25 20:11 ` David Bremner 2017-02-25 3:45 ` [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata David Bremner 2017-02-25 18:42 ` Jani Nikula 2017-02-25 20:21 ` David Bremner 2017-02-25 20:39 ` Jani Nikula 2017-02-25 3:45 ` [PATCH 4/4] lib/message.cc: use view number to invalidate cached metadata David Bremner 2017-02-25 18:44 ` Jani Nikula
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).