Crash in messages

[rhn]

As it turns out, a Messages must be held until all Message object are dropped. It seems like something the MessageOwner trait is supposed to cover.

I've created a fail case (unfortunately, I didn't manage to test it on the corpus):

https://github.com/rhn/notmuch-rs/tree/messages_break

I'll happily turn this into an actual test case if the bug is fixed.

[vhdirk]

Woops, I've been very busy the past few months. I'll look into it :)

You're correct that this is something the MessageOwner is supposed to prevent. The notmuch docs are not really clear in who owns what. I contacted the devs about that, and they basically stated that only the query should be kept alive. Apparently, that is not the case.

[vhdirk]

@rhn The easiest solution would be to remove the implementation of the Drop trait for Messages. Once Query is dropped, notmuch takes care of the rest anyway. That's less conservative on memory usage than I'd like, but since rust doesn't have streaming iterators (yet), that might be the only reasonable thing to do?

Alternatively, I could wrap the marker for Messages and pass that on to each Message yielded, but I reckon that would use more memory than just keeping the notmuch_messages pointer alive a little longer.

[vhdirk]

@rhn It's been quite a while since I looked at this. It seems it should be possible to implement a conditional drop akin to pre_drop in https://docs.rs/galemu/0.2.3/galemu/struct.Bound.html

[kevinboulain]

In general, should invalid operations be prevented? If notmuch-rs is to be a simple wrapper around the C API, I guess it leaves room for another higher-level and less error-prone API?

For example this will panic:

fn main() -> Result<(), notmuch::Error> {
    let database = notmuch::Database::open_with_config(
        Some("/tmp/notmuch"),
        notmuch::DatabaseMode::ReadWrite,
        Some(""),
        None,
    )?;
    let mut messages = database.create_query("*")?.search_messages()?;
    let message = messages.next().unwrap();
    // Would cache the ID and the next call wouldn't panic.
    // println!("{}", message.id());
    database.close()?;
    // thread 'main' panicked at 'assertion failed: !self.is_null()', .../.cargo/registry/src/github.com-1ecc6299db9ec823/notmuch-0.8.0/src/utils.rs:33:13                                    
    println!("{}", message.id());
    Ok(())
}

This could be made impossible by, for example:

• tying a lifetime for the message to the messages,
• tying a lifetime for the messages to the database,
• making close take &mut selfand not &self.