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