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

[David Bremner]

Jani Nikula <jani at nikula.org> writes:

> On Fri, 24 Feb 2017, David Bremner <david at 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