unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* Usage after database close
@ 2020-06-28 13:57 Floris Bruynooghe
  2020-06-28 16:19 ` David Bremner
  0 siblings, 1 reply; 7+ messages in thread
From: Floris Bruynooghe @ 2020-06-28 13:57 UTC (permalink / raw)
  To: notmuch

Hi,

I started writing some test cases to define better what you can do with
a closed database and make sure that the python bindings do not behave
unexpectedly here too.

One of the first things I tried ends up with xapian calling
exit_group(2) directly, terminating the process.  So I'm wondering if
I'm approaching this entirely the wrong way.  My understanding is that
we should generally be allowed to use anything after the database has
been closed, as long as nothing has been destroyed.

Below is a minimal reproducible example of what I'm testing so far.  I
must admit I'm generally lazy here and usually just test with notmuch
that is currently in Debian testing.

Cheers,
Floris

Here the script:

#!/bin/sh

MAILDIR=$(mktemp --directory)
export MAILDIR
echo $MAILDIR

mkdir $MAILDIR/tmp
mkdir $MAILDIR/new
mkdir $MAILDIR/cur

cat > $MAILDIR/notmuch-config <<EOF
[database]
path=$MAILDIR
[user]
name=Some Hacker
primary_email=dst@example.com
[new]
tags=unread;inbox;
ignore=
[search]
exclude_tags=deleted;spam;
[maildir]
synchronize_flags=true
EOF

NOTMUCH_CONFIG=$MAILDIR/notmuch-config
export NOTMUCH_CONFIG

cat > $MAILDIR/cur/1593342032.M818430P304029Q3.powell <<EOF
Received: by MailDir; Sun Jun 28 13:00:32 2020
Message-ID: <0@powell.devork.be>
Date: Sun, 28 Jun 2020 13:00:32 -0000
From: src@example.com
To: dst@example.com
Subject: Test mail
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

This is a test mail
EOF

notmuch new

cat >$MAILDIR/test.c <<EOF
#include <stdio.h>
#include <notmuch.h>

int main(int argc, char** argv) {
    notmuch_status_t status;
    notmuch_database_t* db;
    notmuch_message_t* msg;
    const char* messageid;
    printf("Opening db\n");
    status = notmuch_database_open(argv[1], NOTMUCH_DATABASE_MODE_READ_ONLY, &db);
    if (status != NOTMUCH_STATUS_SUCCESS) {
        return status;
    }
    printf("Finding msg\n");
    status = notmuch_database_find_message(db, "0@powell.devork.be", &msg);
    if (status != NOTMUCH_STATUS_SUCCESS) {
        return status;
    }
    printf("Closing db\n");
    status = notmuch_database_close(db);
    if (status != NOTMUCH_STATUS_SUCCESS) {
        return status;
    }
    printf("Get messageid\n");
    messageid = notmuch_message_get_message_id(msg);
    if (messageid == NULL) {
        return 1;
    }
    printf("Messageid: %s\n", messageid);
    printf("The end.\n");
}
EOF

gcc $MAILDIR/test.c -lnotmuch -o $MAILDIR/test

$MAILDIR/test $MAILDIR

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Usage after database close
  2020-06-28 13:57 Usage after database close Floris Bruynooghe
@ 2020-06-28 16:19 ` David Bremner
  2020-06-28 20:47   ` Floris Bruynooghe
  0 siblings, 1 reply; 7+ messages in thread
From: David Bremner @ 2020-06-28 16:19 UTC (permalink / raw)
  To: Floris Bruynooghe, notmuch

Floris Bruynooghe <flub@devork.be> writes:

> Hi,
>
> I started writing some test cases to define better what you can do with
> a closed database and make sure that the python bindings do not behave
> unexpectedly here too.
>
> One of the first things I tried ends up with xapian calling
> exit_group(2) directly, terminating the process.  So I'm wondering if
> I'm approaching this entirely the wrong way.  My understanding is that
> we should generally be allowed to use anything after the database has
> been closed, as long as nothing has been destroyed.
>
> Below is a minimal reproducible example of what I'm testing so far.  I
> must admit I'm generally lazy here and usually just test with notmuch
> that is currently in Debian testing.

Funny that you should mention lazy, that's basically what the problem is
here ;). notmuch_message_get_message_id is lazily trying to read the
information from the database. This is a bit surprising here because of
the query, but that's not really visible once the message object is
created.

In principle it could be documented what parts of the API can trigger
access to the database, but I'm not sure about the benefit of the extra
complexity. It might be safer to assume that only access to already
fetched information is safe. In particular if you move

    messageid = notmuch_message_get_message_id(msg);

before you close the database, then printing it out afterwards works. I
didn't run it valgrind to make sure, but afaik, that should be perfectly
legal.

The original motivation (see 7864350c938944276c1a378539da7670c211b9b5)
to allow long running processes to release the lock on the
database. This is not a pattern we use in the CLI, so it's not as well
tested as it could be. In particular the work to export
notmuch_database_reopen (tests, documentation) has not happened yet.

d

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Usage after database close
  2020-06-28 16:19 ` David Bremner
@ 2020-06-28 20:47   ` Floris Bruynooghe
  2020-06-28 22:11     ` David Bremner
  0 siblings, 1 reply; 7+ messages in thread
From: Floris Bruynooghe @ 2020-06-28 20:47 UTC (permalink / raw)
  To: David Bremner, notmuch

On Sun 28 Jun 2020 at 13:19 -0300, David Bremner wrote:

> Floris Bruynooghe <flub@devork.be> writes:
>
>> Hi,
>>
>> I started writing some test cases to define better what you can do with
>> a closed database and make sure that the python bindings do not behave
>> unexpectedly here too.
>>
>> One of the first things I tried ends up with xapian calling
>> exit_group(2) directly, terminating the process.  So I'm wondering if
>> I'm approaching this entirely the wrong way.  My understanding is that
>> we should generally be allowed to use anything after the database has
>> been closed, as long as nothing has been destroyed.
>>
>> Below is a minimal reproducible example of what I'm testing so far.  I
>> must admit I'm generally lazy here and usually just test with notmuch
>> that is currently in Debian testing.
>
> Funny that you should mention lazy, that's basically what the problem is
> here ;). notmuch_message_get_message_id is lazily trying to read the
> information from the database. This is a bit surprising here because of
> the query, but that's not really visible once the message object is
> created.
>
> In principle it could be documented what parts of the API can trigger
> access to the database, but I'm not sure about the benefit of the extra
> complexity. It might be safer to assume that only access to already
> fetched information is safe. In particular if you move
>
>     messageid = notmuch_message_get_message_id(msg);
>
> before you close the database, then printing it out afterwards works. I
> didn't run it valgrind to make sure, but afaik, that should be perfectly
> legal.

Ok, I forgot the "expected behaviour" part of the bug report ;) I think
that this doesn't work is fine and I'm not surprised by and your
description of fetching it first is very reasonable.  However I was
expecting NOTMUCH_STATUS_XAPIAN_EXEPTION instead of bluntly getting
terminated.  This is what the notmuch_database_close() docs say after
all.

I had a little look and this seems to be caused by the
message->doc.termlist_begin() call in
_notmuch_message_ensure_metadata(), I didn't have xapian debug symbols
and am not familiar with xapian to quickly have an idea of whether this
case can be improved or not.  (-dbg debian packages for notmuch and
xapian would be very handy ;))

But part of my question is, *should* this be improved?  Am I
interpreting notmuch's intended API correctly?

> The original motivation (see 7864350c938944276c1a378539da7670c211b9b5)
> to allow long running processes to release the lock on the
> database. This is not a pattern we use in the CLI, so it's not as well
> tested as it could be. In particular the work to export
> notmuch_database_reopen (tests, documentation) has not happened yet.
>
> d

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Usage after database close
  2020-06-28 20:47   ` Floris Bruynooghe
@ 2020-06-28 22:11     ` David Bremner
  2020-06-28 22:24       ` David Bremner
  2020-06-29 21:18       ` Floris Bruynooghe
  0 siblings, 2 replies; 7+ messages in thread
From: David Bremner @ 2020-06-28 22:11 UTC (permalink / raw)
  To: Floris Bruynooghe, notmuch

Floris Bruynooghe <flub@devork.be> writes:

>
> Ok, I forgot the "expected behaviour" part of the bug report ;) I think
> that this doesn't work is fine and I'm not surprised by and your
> description of fetching it first is very reasonable.  However I was
> expecting NOTMUCH_STATUS_XAPIAN_EXEPTION instead of bluntly getting
> terminated.  This is what the notmuch_database_close() docs say after
> all.

Sure, uncaught exceptions are never nice. 

>
> I had a little look and this seems to be caused by the
> message->doc.termlist_begin() call in
> _notmuch_message_ensure_metadata(),

I guess almost every Xapian API call will fail with the database closed.

> I didn't have xapian debug symbols and am not familiar with xapian to
> quickly have an idea of whether this case can be improved or not.
> (-dbg debian packages for notmuch and xapian would be very handy ;))
>

You need to add a seperate repo for the new style debug symbols in
Debian:
$ (git)-[master]-% apt policy libxapian30-dbgsym
libxapian30-dbgsym:
  Installed: 1.4.15-1
  Candidate: 1.4.15-1
  Version table:
 *** 1.4.15-1 500
        500 http://debug.mirrors.debian.org/debian-debug testing-debug/main amd64 Packages
        500 http://debug.mirrors.debian.org/debian-debug unstable-debug/main amd64 Packages
        100 /var/lib/dpkg/status

> But part of my question is, *should* this be improved?  Am I
> interpreting notmuch's intended API correctly?

Well, I agree you should get NOTMUCH_STATUS_XAPIAN_EXCEPTION back, or we
should change the docs to say "just don't do that".

d


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Usage after database close
  2020-06-28 22:11     ` David Bremner
@ 2020-06-28 22:24       ` David Bremner
  2020-06-29 14:39         ` David Bremner
  2020-06-29 21:18       ` Floris Bruynooghe
  1 sibling, 1 reply; 7+ messages in thread
From: David Bremner @ 2020-06-28 22:24 UTC (permalink / raw)
  To: Floris Bruynooghe, notmuch

David Bremner <david@tethera.net> writes:


>> But part of my question is, *should* this be improved?  Am I
>> interpreting notmuch's intended API correctly?
>
> Well, I agree you should get NOTMUCH_STATUS_XAPIAN_EXCEPTION back, or we
> should change the docs to say "just don't do that".

Arguments in favour of the latter:

1) several API calls don't return notmuch_status_t, so can't literally
   return NOTMUCH_STATUS_XAPIAN_EXCEPTION

2) notmuch_message_get_{message,thread}_id promise never to return NULL,
   has no way to report errors.

I think it would probably make sense to say (if notmuch_database_reopen)
existed, that if you call notmuch_database_close, don't call anything
else except notmuch_database_reopen or notmuch_database_destroy.

d






^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Usage after database close
  2020-06-28 22:24       ` David Bremner
@ 2020-06-29 14:39         ` David Bremner
  0 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2020-06-29 14:39 UTC (permalink / raw)
  To: Floris Bruynooghe, notmuch

David Bremner <david@tethera.net> writes:

> David Bremner <david@tethera.net> writes:
>
>
>>> But part of my question is, *should* this be improved?  Am I
>>> interpreting notmuch's intended API correctly?
>>
>> Well, I agree you should get NOTMUCH_STATUS_XAPIAN_EXCEPTION back, or we
>> should change the docs to say "just don't do that".
>
> Arguments in favour of the latter:
>
> 1) several API calls don't return notmuch_status_t, so can't literally
>    return NOTMUCH_STATUS_XAPIAN_EXCEPTION
>
> 2) notmuch_message_get_{message,thread}_id promise never to return NULL,
>    has no way to report errors.
>
> I think it would probably make sense to say (if notmuch_database_reopen)
> existed, that if you call notmuch_database_close, don't call anything
> else except notmuch_database_reopen or notmuch_database_destroy.

I belatedly realized the exception is being caught, but then because of
a lack of an error path (and presumably thinking this error was unlikely
/ impossible), INTERNAL_ERROR is called. This is not great for bindings
either.

Regardless of how the API docs are updated, the current calling of
INTERNAL_ERROR should be avoided. I think I know what to do, it's just a
matter doing so with a sensible amount of boilerplate and changes.

d

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Usage after database close
  2020-06-28 22:11     ` David Bremner
  2020-06-28 22:24       ` David Bremner
@ 2020-06-29 21:18       ` Floris Bruynooghe
  1 sibling, 0 replies; 7+ messages in thread
From: Floris Bruynooghe @ 2020-06-29 21:18 UTC (permalink / raw)
  To: David Bremner, notmuch

On Sun 28 Jun 2020 at 19:11 -0300, David Bremner wrote:
> You need to add a seperate repo for the new style debug symbols in
> Debian:
> $ (git)-[master]-% apt policy libxapian30-dbgsym
> libxapian30-dbgsym:
>   Installed: 1.4.15-1
>   Candidate: 1.4.15-1
>   Version table:
>  *** 1.4.15-1 500
>         500 http://debug.mirrors.debian.org/debian-debug testing-debug/main amd64 Packages
>         500 http://debug.mirrors.debian.org/debian-debug unstable-debug/main amd64 Packages
>         100 /var/lib/dpkg/status

My goodness, I completely missed the dbgsym memo.  Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-06-29 21:18 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-28 13:57 Usage after database close Floris Bruynooghe
2020-06-28 16:19 ` David Bremner
2020-06-28 20:47   ` Floris Bruynooghe
2020-06-28 22:11     ` David Bremner
2020-06-28 22:24       ` David Bremner
2020-06-29 14:39         ` David Bremner
2020-06-29 21:18       ` Floris Bruynooghe

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).