Re: Usage after database close
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
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
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
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
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
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