Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash in messages #24

Open
rhn opened this issue Mar 30, 2019 · 4 comments
Open

Crash in messages #24

rhn opened this issue Mar 30, 2019 · 4 comments

Comments

@rhn
Copy link
Contributor

rhn commented Mar 30, 2019

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.

@rhn rhn mentioned this issue Mar 30, 2019
@vhdirk
Copy link
Owner

vhdirk commented Aug 28, 2019

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
Copy link
Owner

vhdirk commented Aug 29, 2019

@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
Copy link
Owner

vhdirk commented Oct 15, 2021

@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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants