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 AA97D6DE1F26 for ; Sat, 25 Feb 2017 12:21:12 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: -0.005 X-Spam-Level: X-Spam-Status: No, score=-0.005 tagged_above=-999 required=5 tests=[AWL=0.006, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-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 yCeMpdmJeKr9 for ; Sat, 25 Feb 2017 12:21:12 -0800 (PST) Received: from fethera.tethera.net (fethera.tethera.net [198.245.60.197]) by arlo.cworth.org (Postfix) with ESMTPS id 07C346DE1F19 for ; Sat, 25 Feb 2017 12:21:11 -0800 (PST) Received: from remotemail by fethera.tethera.net with local (Exim 4.84_2) (envelope-from ) id 1chipU-0006bd-JI; Sat, 25 Feb 2017 15:20:32 -0500 Received: (nullmailer pid 23549 invoked by uid 1000); Sat, 25 Feb 2017 20:21:09 -0000 From: David Bremner To: Jani Nikula , notmuch@notmuchmail.org, eg@gaute.vetsj.com Subject: Re: [PATCH 3/4] lib: handle DatabaseModifiedError in _n_message_ensure_metadata In-Reply-To: <87poi66rjj.fsf@nikula.org> References: <20170225034513.19427-1-david@tethera.net> <20170225034513.19427-4-david@tethera.net> <87poi66rjj.fsf@nikula.org> Date: Sat, 25 Feb 2017 16:21:09 -0400 Message-ID: <8737f210p6.fsf@tethera.net> 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 20:21:12 -0000 Jani Nikula writes: > 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... 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