notmuch as a shared object aka library knigge

notmuch as a shared object aka library knigge

[Justus Winter]

Hi fellow notmuchrs,

while going through the python bindings I recently came across the
following note in the documentation for the Database.get_directory
function [0]:

~~~ snip ~~~
Warning

This call needs a writable database in Database.MODE.READ_WRITE
mode. The underlying library will exit the program if this method is
used on a read-only database!
~~~ snap ~~~

and indeed, the following program exits with an error:

~~~ snip ~~~
import os
import notmuch

db_path = os.path.expanduser('~/Maildir')

with notmuch.Database(db_path, mode=notmuch.Database.MODE.READ_ONLY) as db:
    db.get_directory('')
~~~ snap ~~~

% python temp/get_directory.py
Internal error: Failure to ensure database is writable (lib/directory.cc:100).

The line mentioned in the error message reads:

    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
        INTERNAL_ERROR ("Failure to ensure database is writable");

with

/* There's no point in continuing when we've detected that we've done
 * something wrong internally (as opposed to the user passing in a
 * bogus value).
 *
 * Note that __location__ comes from talloc.h.
 */
#define INTERNAL_ERROR(format, ...)                     \
    _internal_error (format " (%s).\n",                 \
                         ##__VA_ARGS__, __location__)

and _internal_error calling exit(3).

If creating a shared library is the preferred way to increase the
startup time of the notmuch binary that's totally cool, but if
libnotmuch is to be used as a library for arbitrary programs it is not
acceptable to call exit(3).

(And as mentioned before, neither is writing to any file descriptor
unless it has been specifically requested by the caller.)

Cheers,
Justus

0: http://notmuch.readthedocs.org/en/latest/index.html#notmuch.Database.get_directory

Re: notmuch as a shared object aka library knigge

[Patrick Totzke]

Hi all,

Those of you with long enough backlog on the list to remember my rant (id:20110626202733.GA26837@brick)
can guess my opion on this matter but just to be sure..

I am not much of an expert on libnotmuch internals but am using the python bindings extensively.
It feels super-strange using a python module that possibly writes to stderr or any other descriptor
without me explicitly telling it to.
Also, if the library segfauls or calls exit, it essentially rips out the python interpreter underneath my code
without me being able to do any proper error handling.

I know that error handling on a library level is hard, juggling around with bare C, talloc and Xapian.
But i can only strongly encourage any rewrite that ends in the python bindings behaving more pythonic!

Cheers,
/p

Re: notmuch as a shared object aka library knigge

[Austin Clements]

Quoth Justus Winter on Feb 21 at  1:29 am:
> Hi fellow notmuchrs,
> 
> while going through the python bindings I recently came across the
> following note in the documentation for the Database.get_directory
> function [0]:
> 
> ~~~ snip ~~~
> Warning
> 
> This call needs a writable database in Database.MODE.READ_WRITE
> mode. The underlying library will exit the program if this method is
> used on a read-only database!
> ~~~ snap ~~~

This is a bug and should be thought of as such.  INTERNAL_ERROR should
only be used for internal library inconsistencies (e.g., things that
should never ever happen) and the fact that it's leaking out here (and
easy to trigger) is simply a mistake.

This hasn't been fixed because it derives from an interface flaw.
What should notmuch_database_get_directory do on a read-only database?
It's specified to *create* the directory document if it doesn't exist,
which is the problem.  We could of course bandage this up and make it
return an error if you request a non-existent directory on a read-only
database, but that's an inconsistent interface.  I think we were
hoping to tweak the interface so you can tell it whether or not you
want to create the directory, independent of the database being
read/write or read-only, but no one has gotten around to that.  As
always, patches welcome!

Re: notmuch as a shared object aka library knigge

[Justus Winter]

Quoting Austin Clements (2012-02-21 16:53:12)
>Quoth Justus Winter on Feb 21 at  1:29 am:
>> Hi fellow notmuchrs,
>> 
>> while going through the python bindings I recently came across the
>> following note in the documentation for the Database.get_directory
>> function [0]:
>> 
>> ~~~ snip ~~~
>> Warning
>> 
>> This call needs a writable database in Database.MODE.READ_WRITE
>> mode. The underlying library will exit the program if this method is
>> used on a read-only database!
>> ~~~ snap ~~~
>
>This is a bug and should be thought of as such.

Agreed.

>INTERNAL_ERROR should
>only be used for internal library inconsistencies (e.g., things that
>should never ever happen) [...]

Well, I do not agree. If there is a inconsitency within the library
that library should report this to the caller and it is totally okay
to say that if the callee ignores this error, bad things will happen
(i.e. we're in unspecified behavior territory here).

It is still not okay to kill the whole process. Imagine you're using
the alot mail client that uses libnotmuch through the python bindings
and you've just finished writing a letter when libnotmuch decides to
commit suicide and prevent the python code from saving the draft.

For the record, there is libabcs README [0] that clearly states:

~~~ snip ~~~
Never call exit(), abort(), be very careful with assert()
  - Always return error codes.
  - Libraries need to be safe for usage in critical processes that
    need to recover from errors instead of getting killed (think PID 1!).
[...]
Always provide logging/debugging, but do not clutter stderr
  - Allow the app to hook the libs logging into its logging facility.
  - Use conditional logging, do not filter too late.
  - Do not burn cycles with printf() to /dev/null.
  - By default: do not generate any output on stdout/stderr.
~~~ snap ~~~

>This hasn't been fixed because it derives from an interface flaw.

Yes. And the interface flaw is the way error reporting is done within
libnotmuch. I've mentioned this once on the list and received little
feedback wrt how we can fix this kind problems if we need to change
the api to do so.

>As always, patches welcome!

Well, hacking on c code in my free time is not my idea of fun and I'm
not familiar with the code base, so I'd appreciate it if someone who
is in a better position to whip up a patch would step up and do so.

Cheers,
Justus

0: https://git.kernel.org/?p=linux/kernel/git/kay/libabc.git;a=blob_plain;f=README

Re: notmuch as a shared object aka library knigge

[Justus Winter]

Quoting Justus Winter (2012-02-22 16:17:45)
>Quoting Austin Clements (2012-02-21 16:53:12)
>>As always, patches welcome!
>
>Well, hacking on c code in my free time is not my idea of fun and I'm
>not familiar with the code base, so I'd appreciate it if someone who
>is in a better position to whip up a patch would step up and do so.

That wasn't meant to sound as harsh as it probably did. I seriously
hope that someone is around who enjoys to hack on the c/c++ part of
the library and is willing fix problems in it.

I've got a lot of ideas how to improve the python bindings and have
been refactoring it in the past few days. And while doing so I came
across a few problems in the library, one of which was so easy to fix
that I did just that.

And I worked around the two functions (that I know of) that call
exit(3) by conditionally raising exceptions in the python bindings,
but this is only meant as a intermediate fix, a hack that should be
removed as soon as the library is fixed.

But most of those problems require api changes and some kind of idea
on how to do this in a consistent and extensible way while hopefully
providing a smooth transition to the new api. And I don't feel that
I'm in a good position to do this (I know next to nothing about symbol
versions and linker magic) so I mentioned the problems and asked for
help on this issue.

Btw, I think that we can keep the python api stable even if we change
the underlying library. So if there isn't actually anyone who directly
links against libnotmuch (maybe the mutt fork does?) we may not even
worry so much about api stability of libnotmuch.

Happy hacking,
Justus

Re: notmuch as a shared object aka library knigge

[David Bremner]

On Thu, 23 Feb 2012 23:22:00 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:

> That wasn't meant to sound as harsh as it probably did. I seriously
> hope that someone is around who enjoys to hack on the c/c++ part of
> the library and is willing fix problems in it.

Luckily I deleted my snarky reply ;). 

> And I worked around the two functions (that I know of) that call
> exit(3) by conditionally raising exceptions in the python bindings,
> but this is only meant as a intermediate fix, a hack that should be
> removed as soon as the library is fixed.

Can you make test cases to document exactly when internal errors are
occuring in the library? Somehow it seems like the CLI is not triggering
them. It might help clarify the discussion and/or motivate people to fix
them.

d

Re: notmuch as a shared object aka library knigge

[Justus Winter]

Quoting David Bremner (2012-02-24 01:29:36)
>On Thu, 23 Feb 2012 23:22:00 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
>
>> That wasn't meant to sound as harsh as it probably did. I seriously
>> hope that someone is around who enjoys to hack on the c/c++ part of
>> the library and is willing fix problems in it.
>
>Luckily I deleted my snarky reply ;).

*phew* ;)

>> And I worked around the two functions (that I know of) that call
>> exit(3) by conditionally raising exceptions in the python bindings,
>> but this is only meant as a intermediate fix, a hack that should be
>> removed as soon as the library is fixed.
>
>Can you make test cases to document exactly when internal errors are
>occuring in the library? Somehow it seems like the CLI is not triggering
>them. It might help clarify the discussion and/or motivate people to fix
>them.

I did ;) I provided a python program in my original mail for
Database.get_directory aka notmuch_database_get_directory and the one
for Database.find_message_by_filename aka
notmuch_database_find_message_by_filename can be trivially derived
from it (though you need a path to a mail within your maildir). It
should be straight forward to port those to c if you want to.

Justus

Re: notmuch as a shared object aka library knigge

[David Bremner]

On Thu, 23 Feb 2012 23:22:00 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:

> I've got a lot of ideas how to improve the python bindings and have
> been refactoring it in the past few days. And while doing so I came
> across a few problems in the library, one of which was so easy to fix
> that I did just that.

BTW, it would be nice if some of the non-trivial patches were sent to
the list. I'm not suggesting any particular workflow, but there are
certainly others interested in the Python bindings, and who might have
useful comments.

Cheers, 

David

Re: notmuch as a shared object aka library knigge

[Justus Winter]

Quoting David Bremner (2012-02-24 01:33:20)
>On Thu, 23 Feb 2012 23:22:00 +0100, Justus Winter <4winter@informatik.uni-hamburg.de> wrote:
>
>> I've got a lot of ideas how to improve the python bindings and have
>> been refactoring it in the past few days. And while doing so I came
>> across a few problems in the library, one of which was so easy to fix
>> that I did just that.
>
>BTW, it would be nice if some of the non-trivial patches were sent to
>the list. I'm not suggesting any particular workflow, but there are
>certainly others interested in the Python bindings, and who might have
>useful comments.

Sure, I'll do that before doing any invasive changes. So far I've been
breaking the code apart and did mostly doc fixes, wrapped one more
function, fixing bugs here and there.

Justus