Python bindings and Xapian exceptions

Python bindings and Xapian exceptions

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

Is anything happening about:

  A Xapian exception occurred finding message: The revision being read
  has been discarded - you should call Xapian::Database::reopen() and
  retry the operation.

?

It makes the Python bindings almost useless to me.

dme.
-- 
David Edmondson, http://dme.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Python bindings and Xapian exceptions

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

>   A Xapian exception occurred finding message: The revision being read
>   has been discarded - you should call Xapian::Database::reopen() and
>   retry the operation.
> It makes the Python bindings almost useless to me.

Not sure, if the python bindings should simply drop and reopen a
database connection in that case? But I am not sure if libnotmuch.so
still simply exits on such an error case.

The proper fix, and a reason why I am not immediately hacking around in
the python bindings is that notmuch the library would actually return a
proper error value rather than print to stderr and quit.

Happy to receive suggestions on how to handle this best from the python
side of things.

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Python bindings and Xapian exceptions

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

On Tue, 15 Jun 2010 11:03:55 +0200, "Sebastian Spaeth" <Sebastian@SSpaeth.de> wrote:
> >   A Xapian exception occurred finding message: The revision being read
> >   has been discarded - you should call Xapian::Database::reopen() and
> >   retry the operation.
> > It makes the Python bindings almost useless to me.
> 
> Not sure, if the python bindings should simply drop and reopen a
> database connection in that case? But I am not sure if libnotmuch.so
> still simply exits on such an error case.
> 
> The proper fix, and a reason why I am not immediately hacking around in
> the python bindings is that notmuch the library would actually return a
> proper error value rather than print to stderr and quit.
> 
> Happy to receive suggestions on how to handle this best from the python
> side of things.

Unfortunately I'm an ignorant about the best solution.

Having the Python layer hide the fact that the database was re-opened
seems as though it might be dangerous (in case I mix state from before
and after the re-opening), but I can't give a concrete example of
something that would fail.

Given that exception based programming is common in Python, simple
raising the exception and allowing the application to handle it appears
to make sense.

dme.
-- 
David Edmondson, http://dme.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Python bindings and Xapian exceptions

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On Tue, 15 Jun 2010 11:03:55 +0200, "Sebastian Spaeth" <Sebastian@SSpaeth.de> wrote:
> >   A Xapian exception occurred finding message: The revision being read
> >   has been discarded - you should call Xapian::Database::reopen() and
> >   retry the operation.
> > It makes the Python bindings almost useless to me.
> 
> Not sure, if the python bindings should simply drop and reopen a
> database connection in that case? But I am not sure if libnotmuch.so
> still simply exits on such an error case.
> 
> The proper fix, and a reason why I am not immediately hacking around in
> the python bindings is that notmuch the library would actually return a
> proper error value rather than print to stderr and quit.

For this particular case, I think the correct thing is for the library
to simply do the reopen() itself.

That way we can pretend that Xapian doesn't make readers block on
writers.

But for exceptions in general, yes the notmuch library does need to be
fixed to allow the caller of functions to distinguish between things
like "no matches found" and "an exception occurred, so it's unknown if
any messages match the search". That's a general class of library
interface bugs that all need to be fixed.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Python bindings and Xapian exceptions

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

On Thu, 04 Nov 2010 12:31:48 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Tue, 15 Jun 2010 11:03:55 +0200, "Sebastian Spaeth" wrote:
> > >   A Xapian exception occurred finding message: The revision being read
> > >   has been discarded - you should call Xapian::Database::reopen() and
> > >   retry the operation.
> > > It makes the Python bindings almost useless to me.

> > The proper fix, and a reason why I am not immediately hacking around in
> > the python bindings is that notmuch the library would actually return a
> > proper error value rather than print to stderr and quit.
> 
> For this particular case, I think the correct thing is for the library
> to simply do the reopen() itself.

That would already help a lot. But if there is for example a background
task running that modifies the database a lot, reopening once might
still not suffice. So we should try to reopen, yes. But if it still does
not work after a reopen, we still need the possibility to fail
gracefully while notifying the user about the failure.

> But for exceptions in general, yes the notmuch library does need to be
> fixed to allow the caller of functions to distinguish between things
> like "no matches found" and "an exception occurred, so it's unknown if
> any messages match the search". That's a general class of library
> interface bugs that all need to be fixed.

It does return a status for many operations already which is good. THere
are only a few cases where it does not and the only one I know off-hand
is find_message() which is useless at the moment as its result cannot be
trusted. 

What would probably also be good if notmuchlib could export a
database.reopen() command so bindings can reopen the database in case
they need to, without having to have xapian development headers
installed and linking to it directly. Of course if libnotmuch reopens
itself, that would not be required.

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Python bindings and Xapian exceptions

[Matt]

> > But for exceptions in general, yes the notmuch library does need to be
> > fixed to allow the caller of functions to distinguish between things
> > like "no matches found" and "an exception occurred, so it's unknown if
> > any messages match the search". That's a general class of library
> > interface bugs that all need to be fixed.

I 've also hit this *API bug* and was wondering if a fix had been done since
then (I use notmuch 0.17) ? I found nothing on http://notmuchmail.org/news/ 

Re: Python bindings and Xapian exceptions

[David Bremner]

Matt <mattator@gmail.com> writes:

>> > But for exceptions in general, yes the notmuch library does need to be
>> > fixed to allow the caller of functions to distinguish between things
>> > like "no matches found" and "an exception occurred, so it's unknown if
>> > any messages match the search". That's a general class of library
>> > interface bugs that all need to be fixed.
>
> I 've also hit this *API bug* and was wondering if a fix had been done since
> then (I use notmuch 0.17) ? I found nothing on http://notmuchmail.org/news/ 

Can you be more specific? I'd say in general no thorough overhaul of
error handling has happened, but if you can tell us what particular
libnotmuch function (or the equivalient python binding) you are having
trouble with, we may be able to give a more informative answer.

d

Re: Python bindings and Xapian exceptions

[Matt]

2014-12-15 21:41 GMT+01:00 David Bremner <david@tethera.net>:
> Matt <mattator@gmail.com> writes:
>
>>> > But for exceptions in general, yes the notmuch library does need to be
>>> > fixed to allow the caller of functions to distinguish between things
>>> > like "no matches found" and "an exception occurred, so it's unknown if
>>> > any messages match the search". That's a general class of library
>>> > interface bugs that all need to be fixed.
>>
>> I 've also hit this *API bug* and was wondering if a fix had been done since
>> then (I use notmuch 0.17) ? I found nothing on http://notmuchmail.org/news/
>
> Can you be more specific? I'd say in general no thorough overhaul of
> error handling has happened, but if you can tell us what particular
> libnotmuch function (or the equivalient python binding) you are having
> trouble with, we may be able to give a more informative answer.
>

For instance when using the python bindings:
In constructor I do
self.db = notmuch.Database(self.db_path)
and there I have a method called periodically that returns:
returns notmuch.Query(self.db, "tag:unread and tag:inbox").count_messages()

When it fails the previous method returns 0 and displays on stdout/stderr;
"A Xapian exception occurred: The revision being read has been
discarded - you should call Xapian::Database::reopen() and retry the
operation
Query string was: tag:unread and tag:inbox"

The way for me to detect an error is to redirect stdout and check if
the method outputted to stdout, which is not practical. I wish it
would either return "-1", throw an exception or return 2 numbers like
"errorcode, returned value". I personnally don't care if it's done at
the library level or in the python bindings.

Regards

Re: Python bindings and Xapian exceptions

[David Bremner]

Matt <mattator@gmail.com> writes:

> 2014-12-15 21:41 GMT+01:00 David Bremner <david@tethera.net>:
>> Matt <mattator@gmail.com> writes:
>>
>>>> > But for exceptions in general, yes the notmuch library does need to be
>>>> > fixed to allow the caller of functions to distinguish between things
>>>> > like "no matches found" and "an exception occurred, so it's unknown if
>>>> > any messages match the search". That's a general class of library
>>>> > interface bugs that all need to be fixed.
>>>
>>> I 've also hit this *API bug* and was wondering if a fix had been done since
>>> then (I use notmuch 0.17) ? I found nothing on http://notmuchmail.org/news/
>>
>> Can you be more specific? I'd say in general no thorough overhaul of
>> error handling has happened, but if you can tell us what particular
>> libnotmuch function (or the equivalient python binding) you are having
>> trouble with, we may be able to give a more informative answer.
>>
>
> For instance when using the python bindings:
> In constructor I do
> self.db = notmuch.Database(self.db_path)
> and there I have a method called periodically that returns:
> returns notmuch.Query(self.db, "tag:unread and tag:inbox").count_messages()
>
> When it fails the previous method returns 0 and displays on stdout/stderr;
> "A Xapian exception occurred: The revision being read has been
> discarded - you should call Xapian::Database::reopen() and retry the
> operation
> Query string was: tag:unread and tag:inbox"

Right, this seems to be a particularly heinous example.

Any objections (or better ideas) from fellow developers to something
along the lines of the following? It isn't a huge improvement, and I
didn't update the other 3 places it's called (or the bindings), but it
seems like a step forward.  I guess something similar should be done for
notmuch_query_count_threads.

Alternatively, we could follow unix tradition and return -1 on error.
The only argument I can see either way at the moment is that fewer error
return styles is better than more.

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 220839b..06228bc 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -901,8 +901,8 @@ notmuch_threads_destroy (notmuch_threads_t *threads);
  * If a Xapian exception occurs, this function may return 0 (after
  * printing a message).
  */
-unsigned
-notmuch_query_count_messages (notmuch_query_t *query);
+notmuch_status_t
+notmuch_query_count_messages (notmuch_query_t *query, unsigned *count);
 
 /**
  * Return the number of threads matching a search.
diff --git a/lib/query.cc b/lib/query.cc
index 60ff8bd..a623ea8 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -508,8 +508,8 @@ notmuch_threads_destroy (notmuch_threads_t *threads)
     talloc_free (threads);
 }
 
-unsigned
-notmuch_query_count_messages (notmuch_query_t *query)
+notmuch_status_t
+notmuch_query_count_messages (notmuch_query_t *query, unsigned *count_out)
 {
     notmuch_database_t *notmuch = query->notmuch;
     const char *query_string = query->query_string;
@@ -562,12 +562,11 @@ notmuch_query_count_messages (notmuch_query_t *query)
 	count = mset.get_matches_estimated();
 
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred: %s\n",
-		 error.get_msg().c_str());
-	fprintf (stderr, "Query string was: %s\n", query->query_string);
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
-
-    return count;
+    
+    *count_out=count;
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 unsigned
diff --git a/notmuch-count.c b/notmuch-count.c
index 6058f7c..db43959 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -71,6 +71,7 @@ print_count (notmuch_database_t *notmuch, const char *query_str,
 {
     notmuch_query_t *query;
     size_t i;
+    unsigned count;
 
     query = notmuch_query_create (notmuch, query_str);
     if (query == NULL) {
@@ -83,7 +84,9 @@ print_count (notmuch_database_t *notmuch, const char *query_str,
 
     switch (output) {
     case OUTPUT_MESSAGES:
-	printf ("%u\n", notmuch_query_count_messages (query));
+	if (notmuch_query_count_messages (query, &count))
+	    return 1;
+	printf ("%u\n", count);
 	break;
     case OUTPUT_THREADS:
 	printf ("%u\n", notmuch_query_count_threads (query));
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 7c1c809..5b7c0e1 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -650,8 +650,13 @@ notmuch_reply_format_sprinter(void *ctx,
     notmuch_messages_t *messages;
     notmuch_message_t *message;
     mime_node_t *node;
+    unsigned count;
 
-    if (notmuch_query_count_messages (query) != 1) {
+    if (notmuch_query_count_messages (query, &count)) {
+	fprintf (stderr, "Error: Xapian exception counting messages.\n");
+	return 1;
+    }
+    if (count != 1) {
 	fprintf (stderr, "Error: search term did not match precisely one message.\n");
 	return 1;
     }

Re: Python bindings and Xapian exceptions

[Matt]

For now, as a workaround, I open the db, do my request, close it and
repeat the process 5/10 sec later. I hope it does not add to much
overhead ?! Anyway, it owuld be nice to have this fixed in one way or
another.

Thanks for your answer David.
Best regards

2014-12-16 9:02 GMT+01:00 David Bremner <david@tethera.net>:
> Matt <mattator@gmail.com> writes:
>
>> 2014-12-15 21:41 GMT+01:00 David Bremner <david@tethera.net>:
>>> Matt <mattator@gmail.com> writes:
>>>
>>>>> > But for exceptions in general, yes the notmuch library does need to be
>>>>> > fixed to allow the caller of functions to distinguish between things
>>>>> > like "no matches found" and "an exception occurred, so it's unknown if
>>>>> > any messages match the search". That's a general class of library
>>>>> > interface bugs that all need to be fixed.
>>>>
>>>> I 've also hit this *API bug* and was wondering if a fix had been done since
>>>> then (I use notmuch 0.17) ? I found nothing on http://notmuchmail.org/news/
>>>
>>> Can you be more specific? I'd say in general no thorough overhaul of
>>> error handling has happened, but if you can tell us what particular
>>> libnotmuch function (or the equivalient python binding) you are having
>>> trouble with, we may be able to give a more informative answer.
>>>
>>
>> For instance when using the python bindings:
>> In constructor I do
>> self.db = notmuch.Database(self.db_path)
>> and there I have a method called periodically that returns:
>> returns notmuch.Query(self.db, "tag:unread and tag:inbox").count_messages()
>>
>> When it fails the previous method returns 0 and displays on stdout/stderr;
>> "A Xapian exception occurred: The revision being read has been
>> discarded - you should call Xapian::Database::reopen() and retry the
>> operation
>> Query string was: tag:unread and tag:inbox"
>
> Right, this seems to be a particularly heinous example.
>
> Any objections (or better ideas) from fellow developers to something
> along the lines of the following? It isn't a huge improvement, and I
> didn't update the other 3 places it's called (or the bindings), but it
> seems like a step forward.  I guess something similar should be done for
> notmuch_query_count_threads.
>
> Alternatively, we could follow unix tradition and return -1 on error.
> The only argument I can see either way at the moment is that fewer error
> return styles is better than more.
>
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 220839b..06228bc 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -901,8 +901,8 @@ notmuch_threads_destroy (notmuch_threads_t *threads);
>   * If a Xapian exception occurs, this function may return 0 (after
>   * printing a message).
>   */
> -unsigned
> -notmuch_query_count_messages (notmuch_query_t *query);
> +notmuch_status_t
> +notmuch_query_count_messages (notmuch_query_t *query, unsigned *count);
>
>  /**
>   * Return the number of threads matching a search.
> diff --git a/lib/query.cc b/lib/query.cc
> index 60ff8bd..a623ea8 100644
> --- a/lib/query.cc
> +++ b/lib/query.cc
> @@ -508,8 +508,8 @@ notmuch_threads_destroy (notmuch_threads_t *threads)
>      talloc_free (threads);
>  }
>
> -unsigned
> -notmuch_query_count_messages (notmuch_query_t *query)
> +notmuch_status_t
> +notmuch_query_count_messages (notmuch_query_t *query, unsigned *count_out)
>  {
>      notmuch_database_t *notmuch = query->notmuch;
>      const char *query_string = query->query_string;
> @@ -562,12 +562,11 @@ notmuch_query_count_messages (notmuch_query_t *query)
>         count = mset.get_matches_estimated();
>
>      } catch (const Xapian::Error &error) {
> -       fprintf (stderr, "A Xapian exception occurred: %s\n",
> -                error.get_msg().c_str());
> -       fprintf (stderr, "Query string was: %s\n", query->query_string);
> +       return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>      }
> -
> -    return count;
> +
> +    *count_out=count;
> +    return NOTMUCH_STATUS_SUCCESS;
>  }
>
>  unsigned
> diff --git a/notmuch-count.c b/notmuch-count.c
> index 6058f7c..db43959 100644
> --- a/notmuch-count.c
> +++ b/notmuch-count.c
> @@ -71,6 +71,7 @@ print_count (notmuch_database_t *notmuch, const char *query_str,
>  {
>      notmuch_query_t *query;
>      size_t i;
> +    unsigned count;
>
>      query = notmuch_query_create (notmuch, query_str);
>      if (query == NULL) {
> @@ -83,7 +84,9 @@ print_count (notmuch_database_t *notmuch, const char *query_str,
>
>      switch (output) {
>      case OUTPUT_MESSAGES:
> -       printf ("%u\n", notmuch_query_count_messages (query));
> +       if (notmuch_query_count_messages (query, &count))
> +           return 1;
> +       printf ("%u\n", count);
>         break;
>      case OUTPUT_THREADS:
>         printf ("%u\n", notmuch_query_count_threads (query));
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 7c1c809..5b7c0e1 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -650,8 +650,13 @@ notmuch_reply_format_sprinter(void *ctx,
>      notmuch_messages_t *messages;
>      notmuch_message_t *message;
>      mime_node_t *node;
> +    unsigned count;
>
> -    if (notmuch_query_count_messages (query) != 1) {
> +    if (notmuch_query_count_messages (query, &count)) {
> +       fprintf (stderr, "Error: Xapian exception counting messages.\n");
> +       return 1;
> +    }
> +    if (count != 1) {
>         fprintf (stderr, "Error: search term did not match precisely one message.\n");
>         return 1;
>      }