Re: add status value to _notmuch_message_ensure_metadata
Gaute Hope writes: > Ideally if the error could be caught in `notmuch_threads_valid` or > `notmuch_threads_get_thread` I think that would be the clearest, _st > versions would be nice. We can't really control when the exceptions happen, due to lazily reading data from disk. Looking more carefully at the backtrace, the problem is actually inside the library, in particular some descendent of notmuch_threads_get is not properly handling the error from notmuch_message_get_tags. > As I mentioned in > id:1487582192.57s86yczcg.astr...@strange.none it seems that at later > arbitrary iterations (without re-loading the threads object) the > functions return valid data (even `notmuch_thread_get_tags` does). Can > this data be trusted? I feel like this should all be invalid at this > point. > I guess that's up to Xapian to decide, but I imagine anything after the first exception is "undefined behaviour". Data is cached in memory per message in the notmuch layer, so in principle later calls that don't actually reach xapian could succeed. ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: add status value to _notmuch_message_ensure_metadata
David Bremner writes on februar 23, 2017 1:58: Gaute Hope writes: Gaute Hope writes on februar 20, 2017 10:27: David Bremner writes on februar 18, 2017 15:45: In id:1487339566.mz8acpov1j.astr...@strange.none , Gaute provided a traceback of an uncaught Xapian::DatabaseModifiedError. The fix for this is simple, but somewhat intrusive. [...] I haven't tested against Gaute's test case (needs more boost than I have handy). Alright then, attached is a non-boost version that takes a notmuch db path (absolute) as the first argument (no warranty). With the patches above this crashes in a predictable / preventable way, because notmuch_message_get_tags returns NULL. It isn't clear to me yet what the best API choice is here: minimize difference with the old API by returning NULL to indicate errors, or switch completely to the pattern of e.g. notmuch_query_search_messages_st. I suppose we could do something along the same lines and add new _st versions of the problematic functions Hi, Ideally if the error could be caught in `notmuch_threads_valid` or `notmuch_threads_get_thread` I think that would be the clearest, _st versions would be nice. As I mentioned in id:1487582192.57s86yczcg.astr...@strange.none it seems that at later arbitrary iterations (without re-loading the threads object) the functions return valid data (even `notmuch_thread_get_tags` does). Can this data be trusted? I feel like this should all be invalid at this point. Regards, Gaute ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: add status value to _notmuch_message_ensure_metadata
Gaute Hope writes: > Gaute Hope writes on februar 20, 2017 10:27: >> David Bremner writes on februar 18, 2017 15:45: >>> In id:1487339566.mz8acpov1j.astr...@strange.none , Gaute provided a >>> traceback of an uncaught Xapian::DatabaseModifiedError. The fix for >>> this is simple, but somewhat intrusive. >>> >>> [...] >>> >>> I haven't tested against Gaute's test case (needs more boost than I >>> have handy). > > Alright then, attached is a non-boost version that takes a notmuch db > path (absolute) as the first argument (no warranty). > With the patches above this crashes in a predictable / preventable way, because notmuch_message_get_tags returns NULL. It isn't clear to me yet what the best API choice is here: minimize difference with the old API by returning NULL to indicate errors, or switch completely to the pattern of e.g. notmuch_query_search_messages_st. I suppose we could do something along the same lines and add new _st versions of the problematic functions ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: add status value to _notmuch_message_ensure_metadata
Gaute Hope writes on februar 20, 2017 10:27:
David Bremner writes on februar 18, 2017 15:45:
In id:1487339566.mz8acpov1j.astr...@strange.none , Gaute provided a
traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
this is simple, but somewhat intrusive.
[...]
I haven't tested against Gaute's test case (needs more boost than I
have handy).
Alright then, attached is a non-boost version that takes a notmuch db
path (absolute) as the first argument (no warranty).
- gaute
# include
# include
# include
using std::cout;
using std::endl;
int main (int argc, char **argv) {
if (argc < 2) {
std::cerr << "error: specify the path to the test notmuch database" << endl;
return 1;
}
std::string path_db;
path_db = argv[1];
cout << "using db: " << path_db << endl;
notmuch_database_t * nm_db;
notmuch_status_t s =
notmuch_database_open (
path_db.c_str(),
notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_ONLY,
&nm_db);
cout << "db: running test query.." << endl;
notmuch_query_t * q = notmuch_query_create (nm_db, "*");
unsigned int c;
notmuch_status_t st = notmuch_query_count_threads_st (q, &c); // destructive
notmuch_query_destroy (q);
q = notmuch_query_create (nm_db, "*");
cout << "query: " << notmuch_query_get_query_string (q) << ", approx: "
<< c << " threads." << endl;
notmuch_threads_t * threads;
notmuch_thread_t * thread;
st = notmuch_query_search_threads_st (q, &threads);
std::string thread_id;
int i = 0;
for (; notmuch_threads_valid (threads);
notmuch_threads_move_to_next (threads)) {
thread = notmuch_threads_get (threads);
i++;
if (i == 3)
thread_id = notmuch_thread_get_thread_id (thread);
notmuch_thread_destroy (thread);
if (i == 3) break;
}
cout << "thread id to change: " << thread_id << ", thread no: " << i << endl;
notmuch_query_destroy (q);
/* restart query */
cout << "restarting query.." << endl;
q = notmuch_query_create (nm_db, "*");
st = notmuch_query_search_threads_st (q, &threads);
i = 0;
int stop = 2;
cout << "moving to thread: " << stop << endl;
for ( ; notmuch_threads_valid (threads);
notmuch_threads_move_to_next (threads))
{
thread = notmuch_threads_get (threads);
notmuch_thread_get_thread_id (thread);
i++;
cout << "tags: ";
/* get tags */
notmuch_tags_t *tags;
const char *tag;
for (tags = notmuch_thread_get_tags (thread);
notmuch_tags_valid (tags);
notmuch_tags_move_to_next (tags))
{
tag = notmuch_tags_get (tags);
cout << tag << " ";
}
cout << endl;
notmuch_thread_destroy (thread);
if (i == stop) break;
}
/* now open a new db instance, modify the already loaded thread and
* continue loading the original query */
notmuch_database_t * nm_db2;
s = notmuch_database_open (
path_db.c_str(),
notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_WRITE,
&nm_db2);
char qry_s[256];
sprintf (qry_s, "thread:%s", thread_id.c_str ());
notmuch_query_t * q2 = notmuch_query_create (nm_db2, qry_s);
notmuch_threads_t * ts2;
notmuch_thread_t * t2;
st = notmuch_query_search_threads_st (q2, &ts2);
for ( ; notmuch_threads_valid (ts2);
notmuch_threads_move_to_next (ts2))
{
t2 = notmuch_threads_get (ts2);
std::string thread_id = notmuch_thread_get_thread_id (t2);
/* remove unread tag */
notmuch_messages_t * ms = notmuch_thread_get_messages (t2);
notmuch_message_t * m;
for (; notmuch_messages_valid (ms); notmuch_messages_move_to_next (ms)) {
m = notmuch_messages_get (ms);
st = notmuch_message_remove_tag (m, "unread");
notmuch_message_destroy (m);
}
notmuch_messages_destroy (ms);
notmuch_thread_destroy (t2);
break;
}
notmuch_query_destroy (q2);
notmuch_database_close (nm_db2);
/* re-add unread tag */
s = notmuch_database_open (
path_db.c_str(),
notmuch_database_mode_t::NOTMUCH_DATABASE_MODE_READ_WRITE,
&nm_db2);
q2 = notmuch_query_create (nm_db2, qry_s);
st = notmuch_query_search_threads_st (q2, &ts2);
for ( ; notmuch_threads_valid (ts2);
notmuch_threads_move_to_next (ts2))
{
t2 = notmuch_threads_get (ts2);
std::string thread_id = notmuch_thread_get_thread_id (t2);
/* remove unread tag */
notmuch_messages_t * ms = notmuch_thread_get_messages (t2);
notmuch_message_t * m;
for (; notmuch_messages_valid (ms); notmuch_messages_move_to_next (ms)) {
m = notmuch_messages_get (ms);
st = notmuch_message_add_tag (m, "unread");
notmuch_message_destroy (m);
}
notmuch_messages_destroy (ms);
notmuch_thread_destroy (t2);
break;
}
notmuch_query_destroy (q2);
notmuch_database_close (nm_db2);
/* continue loading */
cout << "continue loading.." << endl;
for ( ; notmuch_threads_valid (threads);
notmuch_
Re: add status value to _notmuch_message_ensure_metadata
David Bremner writes on februar 18, 2017 15:45:
In id:1487339566.mz8acpov1j.astr...@strange.none , Gaute provided a
traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
this is simple, but somewhat intrusive.
[...]
I haven't tested against Gaute's test case (needs more boost than I
have handy).
Hi, thanks this seems to get rid of the hard crash, the output from my test
case is attached below.
I have not yet tried to use any new status returns, it seems that the
immediate result is that `notmuch_threads_get ()` returns a valid (but
empty?) notmuch_thread_t * which results in
`notmuch_thread_get_thread_id ()` returning NULL. Strangely, at some
seemingly arbitrary intervals the methods seem to work correctly and
return a valid `notmuch_thread_t` struct.
`threads` and `thread` remain not NULL.
```
$ test/test_notmuch_standalone
db: running test query..
query: *, approx: 10 threads.
thread id to change: 0002, thread no: 3
restarting query..
moving to thread: 2
tags: inbox unread
tags: inbox unread
continue loading..
threads != NULL
thread != NULL
loading: 2:
tags:
threads != NULL
thread != NULL
loading: 3: 0009
tags: inbox unread
threads != NULL
thread != NULL
loading: 4:
tags:
threads != NULL
thread != NULL
loading: 5:
tags:
threads != NULL
thread != NULL
loading: 6: 0004
tags: inbox unread
threads != NULL
thread != NULL
loading: 7: 0006
tags: inbox unread
threads != NULL
thread != NULL
loading: 8: 000a
tags: inbox signed unread
threads != NULL
thread != NULL
loading: 9: 0005
tags: inbox unread
```
with the following changes made to the test program:
```
diff --git a/test/test_notmuch_standalone.cc b/test/test_notmuch_standalone.cc
index 5ef0a00..4634986 100644
--- a/test/test_notmuch_standalone.cc
+++ b/test/test_notmuch_standalone.cc
@@ -189,8 +189,16 @@ int main () {
cout << "threads != NULL" << endl;
}
thread = notmuch_threads_get (threads);
+if (thread == NULL) {
+ cout << "thread == NULL" << endl;
+} else {
+ cout << "thread != NULL" << endl;
+}
+
cout << "loading: " << i;
-std::string tid = notmuch_thread_get_thread_id (thread);
+const char * cid = notmuch_thread_get_thread_id (thread);
+std::string tid = "";
+if (cid != NULL) tid = cid;
cout << ": " << tid << endl;
/* get tags */
```
- gaute
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
add status value to _notmuch_message_ensure_metadata
In id:1487339566.mz8acpov1j.astr...@strange.none , Gaute provided a
traceback of an uncaught Xapian::DatabaseModifiedError. The fix for
this is simple, but somewhat intrusive.
This patch series catches any Xapian exceptions in
_notmuch_message_ensure_metadata and converts them into status returns
(this could be refined in the future to different codes if someone(TM)
finds the time). The rest of the series is either trivial cleanup, or
propagating that status through the API. In particular the following
changes to the API docs are included.
Some of the NULL returns were there already, just not
documented. However message_get_message_id is a genuine change.
The change to message_get_flag is a bit annoying, the new API is
notably less friendly, and the proposed solution for the CLI is the
wrap_message_get_flag function. Neither being inline nor the name is
crucial. Perhaps we should start a file of convenenience functions
that succeed or exit in the CLI.
I haven't tested against Gaute's test case (needs more boost than I
have handy).
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 16da8be9..ac588922 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1286,9 +1286,7 @@ notmuch_messages_collect_tags (notmuch_messages_t
*messages);
* message is valid, (which is until the query from which it derived
* is destroyed).
*
- * This function will not return NULL since Notmuch ensures that every
- * message has a unique message ID, (Notmuch will generate an ID for a
- * message if the original file does not contain one).
+ * The function returns NULL on error.
*/
const char *
notmuch_message_get_message_id (notmuch_message_t *message);
@@ -1302,8 +1300,7 @@ notmuch_message_get_message_id (notmuch_message_t
*message);
* notmuch_message_destroy on 'message' or until a query from which it
* derived is destroyed).
*
- * This function will not return NULL since Notmuch ensures that every
- * message belongs to a single thread.
+ * The function returns NULL on error.
*/
const char *
notmuch_message_get_thread_id (notmuch_message_t *message);
@@ -1344,6 +1341,8 @@ notmuch_message_get_replies (notmuch_message_t *message);
* this function will arbitrarily return a single one of those
* filenames. See notmuch_message_get_filenames for returning the
* complete list of filenames.
+ *
+ * The function returns NULL on error.
*/
const char *
notmuch_message_get_filename (notmuch_message_t *message);
@@ -1357,6 +1356,8 @@ notmuch_message_get_filename (notmuch_message_t *message);
*
* Each filename in the iterator is an absolute filename, (the initial
* component will match notmuch_database_get_path() ).
+ *
+ * The function returns NULL on error.
*/
notmuch_filenames_t *
notmuch_message_get_filenames (notmuch_message_t *message);
@@ -1378,10 +1379,16 @@ typedef enum _notmuch_message_flag {
/**
* Get a value of a flag for the email corresponding to 'message'.
+ * @returns
+ * - NOTMUCH_STATUS_XAPIAN_EXCEPTION: an error occured reading message
metadata from disk
+ * - NOTMUCH_STATUS_NULL_POINTER: Neither *key* nor *value* may be NULL.
+ * - NOTMUCH_STATUS_SUCCESS: No error occured.
+ * @since libnotmuch 5 (notmuch 0.24)
*/
-notmuch_bool_t
+notmuch_status_t
notmuch_message_get_flag (notmuch_message_t *message,
- notmuch_message_flag_t flag);
+ notmuch_message_flag_t flag,
+ notmuch_bool_t *value);
/**
* Set a value of a flag for the email corresponding to 'message'.
@@ -1449,6 +1456,8 @@ notmuch_message_get_header (notmuch_message_t *message,
const char *header);
* notmuch_tags_t object. (For consistency, we do provide a
* notmuch_tags_destroy function, but there's no good reason to call
* it if the message is about to be destroyed).
+ *
+ * The function returns NULL on error.
*/
notmuch_tags_t *
notmuch_message_get_tags (notmuch_message_t *message);
@@ -1659,6 +1668,12 @@ void
notmuch_message_destroy (notmuch_message_t *message);
/**
+ * Return the notmuch database of this message.
+ */
+notmuch_database_t *
+notmuch_message_get_database (notmuch_message_t *message);
+
+/**
* @name Message Properties
*
* This interface provides the ability to attach arbitrary (key,value)
@@ -1750,7 +1765,7 @@ typedef struct _notmuch_string_map_iterator
notmuch_message_properties_t;
* notmuch_message_properties_t *list;
*
* for (list = notmuch_message_get_properties (message, "testkey1", TRUE);
- * notmuch_message_properties_valid (list);
notmuch_message_properties_move_to_next (list)) {
+ * list && notmuch_message_properties_valid (list);
notmuch_message_properties_move_to_next (list)) {
*printf("%s\n", notmuch_message_properties_value(list));
* }
*
@@ -1758,9 +1773,11 @@ typedef struct _notmuch_string_map_iterator
notmuch_message_properties_t;
*
* Note that there's no explicit destructor needed for the
* notmuch_message_properties_t object.
