Re: Usage after database close

2020-06-29 Thread Floris Bruynooghe
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!
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Usage after database close

2020-06-29 Thread David Bremner
David Bremner  writes:

> David Bremner  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Usage after database close

2020-06-28 Thread David Bremner
David Bremner  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






___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Usage after database close

2020-06-28 Thread David Bremner
Floris Bruynooghe  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


___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Usage after database close

2020-06-28 Thread Floris Bruynooghe
On Sun 28 Jun 2020 at 13:19 -0300, David Bremner wrote:

> Floris Bruynooghe  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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: Usage after database close

2020-06-28 Thread David Bremner
Floris Bruynooghe  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

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Usage after database close

2020-06-28 Thread Floris Bruynooghe
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 < $MAILDIR/cur/1593342032.M818430P304029Q3.powell <
Date: Sun, 28 Jun 2020 13:00:32 -
From: s...@example.com
To: d...@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 <
#include 

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, 
);
if (status != NOTMUCH_STATUS_SUCCESS) {
return status;
}
printf("Finding msg\n");
status = notmuch_database_find_message(db, "0...@powell.devork.be", );
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
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch