Quoth Mark Walters on Apr 17 at 9:42 am: > On Thu, 12 Apr 2012, Austin Clements <amdragon@MIT.EDU> wrote: > > Quoth Justus Winter on Apr 12 at 11:05 am: > >> Quoting Austin Clements (2012-04-01 05:23:23) > >> >Quoth Justus Winter on Mar 21 at 1:55 am: > >> >> I propose to split the function notmuch_database_close into > >> >> notmuch_database_close and notmuch_database_destroy so that long > >> >> running processes like alot can close the database while still using > >> >> data obtained from queries to that database. > >> > > >> >Is this actually safe? My understanding of Xapian::Database::close is > >> >that, once you've closed the database, basically anything can throw a > >> >Xapian exception. A lot of data is retrieved lazily, both by notmuch > >> >and by Xapian, so simply having, say, a notmuch_message_t object isn't > >> >enough to guarantee that you'll be able to get data out of it after > >> >closing the database. Hence, I don't see how this interface could be > >> >used correctly. > >> > >> I do not know how, but both alot and afew (and occasionally the > >> notmuch binary) are somehow safely using this interface on my box for > >> the last three weeks. > > > > I see. TL;DR: This isn't safe, but that's okay if we document it. > > > > The bug report [0] you pointed to was quite informative. At its core, > > this is really a memory management issue. To sum up for the record > > (and to check my own thinking): It sounds like alot is careful not to > > use any notmuch objects after closing the database. The problem is > > that, currently, closing the database also talloc_free's it, which > > recursively free's everything derived from it. Python later GCs the > > wrapper objects, which *also* try to free their underlying objects, > > resulting in a double free. > > > > Before the change to expose notmuch_database_close, the Python > > bindings would only talloc_free from destructors. Furthermore, they > > prevented the library from recursively freeing things at other times > > by internally maintaining a reverse reference for every library talloc > > reference (e.g., message is a sub-allocation of query, so the bindings > > keep a reference from each message to its query to ensure the query > > doesn't get freed). The ability to explicitly talloc_free the > > database subverts this mechanism. > > > > > > So, I've come around to thinking that splitting notmuch_database_close > > and _destroy is okay. It certainly parallels the rest of the API > > better. However, notmuch_database_close needs a big warning similar > > to Xapian::Database::close's warning that retrieving information from > > objects derived from this database may not work after calling close. > > notmuch_database_close is really a specialty interface, and about the > > only thing you can guarantee after closing the database is that you > > can destroy other objects. This is also going to require a SONAME > > major version bump, as mentioned by others. Which, to be fair, would > > be a good opportunity to fix some other issues, too, like how > > notmuch_database_open can't return errors and how > > notmuch_database_get_directory is broken on read-only databases. The > > actual bump should be done at release time, but maybe we should drop a > > note somewhere (NEWS?) so we don't forget. > > Can I just check that there is no way to reopen the Xapian database > readonly? (I may be using the wrong term: I mean is there a way of > switching an open read-write database to read-only without losing the > attached structures/messages/threads etc) If I understand it this would > be sufficient as it would free the lock, but could be more generally > useful for long lived notmuch processes. That would be handy and perfect for this situation, but no (I double-checked with Olly on IRC, which you probably saw). We might be able to lobby for this capability if it seems more generally useful. On the other hand, I think it would probably mix poorly with Xapian's optimistic snapshot isolation if we tried to use it for anything non-trivial (combined with real snapshot isolation it would be awesome).