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

destroy the msg when dropping it, closes file descriptor #30

Closed
wants to merge 1 commit into from

Conversation

foosinn
Copy link

@foosinn foosinn commented Jul 27, 2020

Calls notmuch_message_destroy upon dropping the message. This frees the memory and the filedescriptor.

From the documentation:

It can be useful to call this function in the case of a single query object with many messages in the result, (such as iterating over the entire database). Otherwise, it's fine to never call this function and there will still be no memory leaks. (The memory from the messages get reclaimed when the containing query is destroyed.)

This fixes file descriptor issues when querying a lot of messages and e.g. call header() on them.

fixes #27

Please note: I am not an experienced rust programmer, if there are any issues let me know.

@andir
Copy link

andir commented Jul 27, 2020

While this works I was reluctant to do the same since I never fully understood the role of the "MessageOwner" in this code. I guessed the lifetime of said object defines the lifetime of (all) the message(s) associated with e.g. the related query?

@foosinn
Copy link
Author

foosinn commented Jul 27, 2020

As far as i understood a query can only once be fetched, otherwise one would have to make a new query. Maybe someone here has more knowlege about notmuch's C side?

@foosinn
Copy link
Author

foosinn commented Jul 27, 2020

Sidenote: I ran into the issue using notcoal to tag my mails. I haven't noticed any issues there with the patch applied

@foosinn
Copy link
Author

foosinn commented Jul 27, 2020

Note: If you are finished with a message before its containing query, you can call notmuch_message_destroy to clean up some memory sooner (as in the above example). Otherwise, if your message objects are long-lived, then you don't need to call notmuch_message_destroy and all the memory will still be reclaimed when the query is destroyed.

Note that there's no explicit destructor needed for the notmuch_messages_t object. (For consistency, we do provide a notmuch_messages_destroy function, but there's no good reason to call it if the query is about to be destroyed).

from https://git.notmuchmail.org/git?p=notmuch;a=blob;f=lib/notmuch.h;hb=HEAD#l942

@eaon
Copy link
Contributor

eaon commented Jul 29, 2020

I think I may have stumbled over this once but I wasn't able to reproduce it. Makes sense because I don't think I index enough messages to run out of file descriptors particularly often. Apparently the lack of drop was explicitly mentioned here #3 but I'm not sure if it's still relevant.

Either way, thanks for this, I'll push a bugfix release of notcoal when/if this gets resolved.

@vhdirk
Copy link
Owner

vhdirk commented Jul 29, 2020

Unfortunately, this has unintended side effects. Dropping a Message essentially invalidates it throughout the Query it is bound to. Meaning that, if you call Query.search_messages() after that again, on the same query, it will panic because of a null pointer.
I was trying to implement a mechanism where the Drop trait would only call the c impl when the handle to the Query the Message was bound to was no longer accessible (only remaining through a PhantomCow), but so far I've been unsuccessful.

@foosinn
Copy link
Author

foosinn commented Jul 29, 2020

I always expected search_messages to re-fetch all the messages. Thanks for noticing @vhdirk.

@vhdirk
Copy link
Owner

vhdirk commented Jul 29, 2020

Still, If you figure out a way to make this work, I'd be happy to integrate it. Lifetime management of notmuch is pretty hard :(

@foosinn
Copy link
Author

foosinn commented Aug 4, 2020

What do you think about an into_search_messages that consumes the query?

The big downside is that this requires new types.

@vhdirk
Copy link
Owner

vhdirk commented Aug 15, 2020

I'm certainly not against it. I just don't have the time these days :)

@OJFord
Copy link

OJFord commented Jul 25, 2021

Did you get any further with this @foosinn? I like the into_search_messages (or just into_iter?) approach personally.

@vhdirk
Copy link
Owner

vhdirk commented Oct 20, 2021

@OJFord @foosinn: I was unable to fix this with supercow so I'm now trying something radically different: See #37. It completely breaks the API, so please let me know what you think.

@vhdirk
Copy link
Owner

vhdirk commented May 2, 2022

Closing as supercow has been replaced with Rc's

@vhdirk vhdirk closed this May 2, 2022
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

Successfully merging this pull request may close these issues.

File descriptors lifetime bound to query not to message
5 participants