Date: Wed, 18 Jul 2012 16:40:01 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Adrien Bustany <adrien@bustany.org>
Subject: Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected
Message-ID: <20120718204001.GT31670@mit.edu>
References: <1342636475-16057-1-git-send-email-adrien@bustany.org>
	<1342636475-16057-4-git-send-email-adrien@bustany.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1342636475-16057-4-git-send-email-adrien@bustany.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

This is subtle enough that I think it deserves a comment in the source
code explaining that tracking the talloc owner reference, combined
with the fact that Go finalizers are run in dependency order, ensures
that the C objects will always be destroyed from the talloc leaves up.

Just one inline comment below.  Otherwise, I think this is all
correct.

Is reproducing the talloc hierarchy in all of the bindings really the
right approach?  I feel like there has to be a better way (or that the
way we use talloc in the library is slightly broken).  What if the
bindings created an additional talloc reference to each managed
object, just to keep the object alive, and used talloc_unlink instead
of the destroy functions?

Quoth Adrien Bustany on Jul 18 at  9:34 pm:
> This makes notmuch appropriately free the underlying notmuch C objects
> when garbage collecting their Go wrappers. To make sure we don't break
> the underlying links between objects (for example, a notmuch_messages_t
> being GC'ed before a notmuch_message_t belonging to it), we add for each
> wraper struct a pointer to the owner object (Go objects with a reference
> pointing to them don't get garbage collected).
> ---
>  bindings/go/src/notmuch/notmuch.go |  153 +++++++++++++++++++++++++++++++-----
>  1 files changed, 134 insertions(+), 19 deletions(-)
> 
> diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
> index 1d77fd2..3f436a0 100644
> --- a/bindings/go/src/notmuch/notmuch.go
> +++ b/bindings/go/src/notmuch/notmuch.go
> @@ -11,6 +11,7 @@ package notmuch
>  #include "notmuch.h"
>  */
>  import "C"
> +import "runtime"
>  import "unsafe"
>  
>  // Status codes used for the return values of most functions
> @@ -47,40 +48,152 @@ func (self Status) String() string {
>  /* Various opaque data types. For each notmuch_<foo>_t see the various
>   * notmuch_<foo> functions below. */
>  
> +type Object interface {}
> +
>  type Database struct {
>  	db *C.notmuch_database_t
>  }
>  
> +func createDatabase(db *C.notmuch_database_t) *Database {
> +	self := &Database{db: db}
> +
> +	runtime.SetFinalizer(self, func(x *Database) {
> +		if (x.db != nil) {
> +			C.notmuch_database_destroy(x.db)
> +		}
> +	})
> +
> +	return self
> +}
> +
>  type Query struct {
>  	query *C.notmuch_query_t
> +	owner Object
> +}
> +
> +func createQuery(query *C.notmuch_query_t, owner Object) *Query {
> +	self := &Query{query: query, owner: owner}
> +
> +	runtime.SetFinalizer(self, func(x *Query) {
> +		if (x.query != nil) {
> +			C.notmuch_query_destroy(x.query)
> +		}
> +	})
> +
> +	return self
>  }
>  
>  type Threads struct {
>  	threads *C.notmuch_threads_t
> +	owner Object
> +}
> +
> +func createThreads(threads *C.notmuch_threads_t, owner Object) *Threads {
> +	self := &Threads{threads: threads, owner: owner}
> +
> +	runtime.SetFinalizer(self, func(x *Threads) {
> +		if (x.threads != nil) {
> +			C.notmuch_threads_destroy(x.threads)
> +		}
> +	})
> +
> +	return self
>  }
>  
>  type Thread struct {
>  	thread *C.notmuch_thread_t
> +	owner Object
> +}
> +
> +func createThread(thread *C.notmuch_thread_t, owner Object) *Thread {
> +	self := &Thread{thread: thread, owner: owner}
> +
> +	runtime.SetFinalizer(self, func(x *Thread) {
> +		if (x.thread != nil) {
> +			C.notmuch_thread_destroy(x.thread)
> +		}
> +	})
> +
> +	return self
>  }
>  
>  type Messages struct {
>  	messages *C.notmuch_messages_t
> +	owner Object
> +}
> +
> +func createMessages(messages *C.notmuch_messages_t, owner Object) *Messages {
> +	self := &Messages{messages: messages, owner: owner}
> +
> +	return self
>  }
>  
>  type Message struct {
>  	message *C.notmuch_message_t
> +	owner Object
> +}
> +
> +func createMessage(message *C.notmuch_message_t, owner Object) *Message {
> +	self := &Message{message: message, owner: owner}
> +
> +	runtime.SetFinalizer(self, func(x *Message) {
> +		if (x.message != nil) {
> +			C.notmuch_message_destroy(x.message)
> +		}
> +	})
> +
> +	return self
>  }
>  
>  type Tags struct {
>  	tags *C.notmuch_tags_t
> +	owner Object
> +}
> +
> +func createTags(tags *C.notmuch_tags_t, owner Object) *Tags {
> +	self := &Tags{tags: tags, owner: owner}
> +
> +	runtime.SetFinalizer(self, func(x *Tags) {
> +		if (x.tags != nil) {
> +			C.notmuch_tags_destroy(x.tags)
> +		}
> +	})
> +
> +	return self
>  }
>  
>  type Directory struct {
>  	dir *C.notmuch_directory_t
> +	owner Object
> +}
> +
> +func createDirectory(directory *C.notmuch_directory_t, owner Object) *Directory {
> +	self := &Directory{dir: directory, owner: owner}
> +
> +	runtime.SetFinalizer(self, func(x *Directory) {
> +		if (x.dir != nil) {
> +			C.notmuch_directory_destroy(x.dir)
> +		}
> +	})
> +
> +	return self
>  }
>  
>  type Filenames struct {
>  	fnames *C.notmuch_filenames_t
> +	owner Object
> +}
> +
> +func createFilenames(filenames *C.notmuch_filenames_t, owner Object) *Filenames {
> +	self := &Filenames{fnames: filenames, owner: owner}
> +
> +	runtime.SetFinalizer(self, func(x *Filenames) {
> +		if (x.fnames != nil) {
> +			C.notmuch_filenames_destroy(x.fnames)
> +		}
> +	})
> +
> +	return self
>  }
>  
>  type DatabaseMode C.notmuch_database_mode_t
> @@ -100,12 +213,13 @@ func NewDatabase(path string) (*Database, Status) {
>  		return nil, STATUS_OUT_OF_MEMORY
>  	}
>  
> -	self := &Database{db: nil}
> -	st := Status(C.notmuch_database_create(c_path, &self.db))
> +	var db *C.notmuch_database_t;
> +	st := Status(C.notmuch_database_create(c_path, &db))
>  	if st != STATUS_SUCCESS {
>  		return nil, st
>  	}
> -	return self, st
> +
> +	return createDatabase(db), st
>  }
>  
>  /* Open an existing notmuch database located at 'path'.
> @@ -134,12 +248,13 @@ func OpenDatabase(path string, mode DatabaseMode) (*Database, Status) {
>  		return nil, STATUS_OUT_OF_MEMORY
>  	}
>  
> -	self := &Database{db: nil}
> -	st := Status(C.notmuch_database_open(c_path, C.notmuch_database_mode_t(mode), &self.db))
> +	var db *C.notmuch_database_t;
> +	st := Status(C.notmuch_database_open(c_path, C.notmuch_database_mode_t(mode), &db))
>  	if st != STATUS_SUCCESS {
>  		return nil, st
>  	}
> -	return self, st
> +
> +	return createDatabase(db), st
>  }
>  
>  /* Close the given notmuch database, freeing all associated
> @@ -204,7 +319,7 @@ func (self *Database) GetDirectory(path string) (*Directory, Status) {
>  	if st != STATUS_SUCCESS || c_dir == nil {
>  		return nil, st
>  	}
> -	return &Directory{dir: c_dir}, st
> +	return createDirectory(c_dir, nil), st

It looks like you have a nil owner for anything whose talloc parent is
the database.  Is this intentional?  Shouldn't the owner be self in
these cases, too?

>  }
>  
>  /* Add a new message to the given notmuch database.
> @@ -258,7 +373,7 @@ func (self *Database) AddMessage(fname string) (*Message, Status) {
>  	var c_msg *C.notmuch_message_t = new(C.notmuch_message_t)
>  	st := Status(C.notmuch_database_add_message(self.db, c_fname, &c_msg))
>  
> -	return &Message{message: c_msg}, st
> +	return createMessage(c_msg, nil), st
>  }
>  
>  /* Remove a message from the given notmuch database.
> @@ -319,12 +434,12 @@ func (self *Database) FindMessage(message_id string) (*Message, Status) {
>  		return nil, STATUS_OUT_OF_MEMORY
>  	}
>  
> -	msg := &Message{message: nil}
> -	st := Status(C.notmuch_database_find_message(self.db, c_msg_id, &msg.message))
> +	var msg *C.notmuch_message_t
> +	st := Status(C.notmuch_database_find_message(self.db, c_msg_id, &msg))
>  	if st != STATUS_SUCCESS {
>  		return nil, st
>  	}
> -	return msg, st
> +	return createMessage(msg, nil), st
>  }
>  
>  /* Return a list of all tags found in the database.
> @@ -339,7 +454,7 @@ func (self *Database) GetAllTags() *Tags {
>  	if tags == nil {
>  		return nil
>  	}
> -	return &Tags{tags: tags}
> +	return createTags(tags, nil)
>  }
>  
>  /* Create a new query for 'database'.
> @@ -379,7 +494,7 @@ func (self *Database) CreateQuery(query string) *Query {
>  	if q == nil {
>  		return nil
>  	}
> -	return &Query{query: q}
> +	return createQuery(q, nil)
>  }
>  
>  /* Sort values for notmuch_query_set_sort */
> @@ -459,7 +574,7 @@ func (self *Query) SearchThreads() *Threads {
>  	if threads == nil {
>  		return nil
>  	}
> -	return &Threads{threads: threads}
> +	return createThreads(threads, self)
>  }
>  
>  /* Execute a query for messages, returning a notmuch_messages_t object
> @@ -505,7 +620,7 @@ func (self *Query) SearchMessages() *Messages {
>  	if msgs == nil {
>  		return nil
>  	}
> -	return &Messages{messages: msgs}
> +	return createMessages(msgs, self)
>  }
>  
>  /* Destroy a notmuch_query_t along with any associated resources.
> @@ -607,7 +722,7 @@ func (self *Messages) Get() *Message {
>  	if msg == nil {
>  		return nil
>  	}
> -	return &Message{message: msg}
> +	return createMessage(msg, self)
>  }
>  
>  /* Move the 'messages' iterator to the next message.
> @@ -659,7 +774,7 @@ func (self *Messages) CollectTags() *Tags {
>  	if tags == nil {
>  		return nil
>  	}
> -	return &Tags{tags: tags}
> +	return createTags(tags, self)
>  }
>  
>  /* Get the message ID of 'message'.
> @@ -739,7 +854,7 @@ func (self *Message) GetReplies() *Messages {
>  	if msgs == nil {
>  		return nil
>  	}
> -	return &Messages{messages: msgs}
> +	return createMessages(msgs, self)
>  }
>  
>  /* Get a filename for the email corresponding to 'message'.
> @@ -871,7 +986,7 @@ func (self *Message) GetTags() *Tags {
>  	if tags == nil {
>  		return nil
>  	}
> -	return &Tags{tags: tags}
> +	return createTags(tags, self)
>  }
>  
>  /* The longest possible tag value. */