Rethinking *_destroy()

Rethinking *_destroy()

[Ben Gamari]

As many might have noticed, there was recently a bit of a discussion on
this list concerning the state of memory management in libnotmuch,
especially regarding some classes of garbage collectors.

To summarize (someone correct me if I get something wrong),

 * Garbage collectors don't always dispose of objects in the order that
   they become unreachable (due to the fact that in the existence of
   cycles, this order is not well-defined).

 * Notmuch emulates a C-style free() function (which we call
   *_destroy()) on top of talloc

 * Calling *_destroy() on an object (e.g. Query) will also cause its
   children (e.g. Messages) to be freed

 * Calling *_destroy() on an object which has already been freed
   (not surprisingly) causes talloc to abort

Overall, this means that languages with cyclical garbage collectors
(Python, Haskell, and I'm sure others) can not bind libnotmuch
correctly. In the case of my work on the Haskell binding, I've found
that very often Query objects are released before Messages, quickly
causing talloc to abort. 

The solution as suggested my several people is for notmuch to
expose its reference counting mechanism (which even the problems with
*_destroy() notwithstanding, seems like a more natural means of memory
management, IMHO).

I can see at least two ways of doing this,

  1) Acknowledging that we use talloc and allowing users to use
     talloc_ref and talloc_unlink directly

  2) Wrapping talloc by adding a *_ref() and *_unref() to each object

Additionally, we need to decide to what extent we want to break the
libnotmuch API. While strictly speaking we could keep *_destroy() around
without breaking existing code, this will mean we will have two ways of
freeing an object. Perhaps a scheduled deprecation in a release or two
is in order here?

Anyways, I strongly believe that one of the above actions should be
taken as the current state of affairs is unacceptable for binding
writers.

Any and all feedback is desired.

Cheers,

- Ben

Re: Rethinking *_destroy()

[Ben Gamari]

On Mon, 19 Sep 2011 16:22:39 -0400, Ben Gamari <bgamari.foss@gmail.com> wrote:
> I can see at least two ways of doing this,
> 
>   1) Acknowledging that we use talloc and allowing users to use
>      talloc_ref and talloc_unlink directly
> 
>   2) Wrapping talloc by adding a *_ref() and *_unref() to each object
> 
I should not that these aren't quite as trivial as they sound. As I
neglect to mention in this message, we currently use talloc_free in
*_destroy(). As of talloc-2.0, talloc_free() fails on objects with more
than one parent. When we allow library users to add their own references
to notmuch objects, this assumption will break. Sorry for the confusion,

Cheers,

- Ben

Re: Rethinking *_destroy()

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2969 bytes --]

On Mon, 19 Sep 2011 16:22:39 -0400, Ben Gamari <bgamari.foss@gmail.com> wrote:
> As many might have noticed, there was recently a bit of a discussion on
> this list concerning the state of memory management in libnotmuch,
> especially regarding some classes of garbage collectors.
> 
> To summarize (someone correct me if I get something wrong),

Thanks for the summary, Ben. As many might have noticed, I'm somewhat
behind on my reading of the notmuch mailing list right now. So I
appreciate you bringing this issue to my attention.

> Overall, this means that languages with cyclical garbage collectors
> (Python, Haskell, and I'm sure others) can not bind libnotmuch
> correctly.

That certainly sound unappealing.

> I can see at least two ways of doing this,
> 
>   1) Acknowledging that we use talloc and allowing users to use
>      talloc_ref and talloc_unlink directly

I like this option, myself. I think talloc has been a wonderful boon to
my programming. So I don't have a problem with the notmuch API
documentation committing the implementation to using talloc. I'm also
quite glad to let the notmuch documentation advertise talloc to any
readers.

>   2) Wrapping talloc by adding a *_ref() and *_unref() to each object

That looks like a lot of extra API, but with no substantial benefit. (We
would get the freedom to switch to some other implementation of talloc,
but, I don't think we need that.)

> Additionally, we need to decide to what extent we want to break the
> libnotmuch API. While strictly speaking we could keep *_destroy() around
> without breaking existing code, this will mean we will have two ways of
> freeing an object. Perhaps a scheduled deprecation in a release or two
> is in order here?

Actually, I would prefer to leave *_destroy around, (as long as it's
doing nothing other than tall_free (or talloc_unlink? [*]) which does
appear to be the case for all existing functions).

To me, it doesn't really look like two ways of freeing an object.
Anyone who calls notmuch_query_create, (for example), should call
notmuch_query_destroy and those two calls pair nicely.

Then, *some* callers will want to take advantage of talloc. Those users
can add calls to talloc_reference and talloc_unlink (which pair nicely
themselves).

What I don't want is the unnatural pairing of something like
"notmuch_query_create" with "talloc_unlink". That just seems uselessly
harder to learn and remember.

> Any and all feedback is desired.

Thanks for pointing out the issue. Let me know if I've missed
anything. And, happy binding!

-Carl

[*] These destroy() functions are all currently calling talloc_free()
which won't work if the user has any remaining parents still present due
to calls to talloc_reference(). Perhaps we can just document that all
talloc_reference()/talloc_unlink() pairs must be complete before the
caller calls _destroy()?

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Rethinking *_destroy()

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Mon, 19 Sep 2011 18:08:22 -0400, Ben Gamari <bgamari.foss@gmail.com> wrote:
[...]

Just sayin' that from a python perspective, this happen to seem to work
fine with the current cpython implementation, but it might well break
when switching to pypy or what not.

So, given some spare time, I would be happy to move towards using a
talloc_refunref scheme which would work just fine as well. Even better,
I would take patches doing that for me :-)

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]