[PATCH 0/7] Various fixes for the Go bindings

[PATCH 0/7] Various fixes for the Go bindings

[Adrien Bustany]

The following patches fix some serious memory management issues with
the Go bindings, and add some missing functions as well.

Adrien Bustany (7):
  go: Use iota in enum bindings
  go: Add missing MESSAGE_FLAG_EXCLUDED in Flag enum
  go: Allow notmuch objects to be garbage collected
  go: Make Destroy functions safe to call several times
  go: Partially bind notmuch_database_upgrade
  go: Bind notmuch_database_find_message_by_filename
  go: Bind notmuch_thread_t functions

 bindings/go/src/notmuch/notmuch.go |  471 ++++++++++++++++++++++++++++++++++--
 1 files changed, 447 insertions(+), 24 deletions(-)

-- 
1.7.7.6

[PATCH 1/7] go: Use iota in enum bindings

[Adrien Bustany]

Using iota is the correct way to get the values in the enum increment
automatically. The old code would just set all the enum values to 0.
---
 bindings/go/src/notmuch/notmuch.go |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
index 00bd53a..ecd7418 100644
--- a/bindings/go/src/notmuch/notmuch.go
+++ b/bindings/go/src/notmuch/notmuch.go
@@ -86,7 +86,7 @@ type Filenames struct {
 type DatabaseMode C.notmuch_database_mode_t
 
 const (
-	DATABASE_MODE_READ_ONLY DatabaseMode = 0
+	DATABASE_MODE_READ_ONLY DatabaseMode = iota
 	DATABASE_MODE_READ_WRITE
 )
 
@@ -386,7 +386,7 @@ func (self *Database) CreateQuery(query string) *Query {
 type Sort C.notmuch_sort_t
 
 const (
-	SORT_OLDEST_FIRST Sort = 0
+	SORT_OLDEST_FIRST Sort = iota
 	SORT_NEWEST_FIRST
 	SORT_MESSAGE_ID
 	SORT_UNSORTED
@@ -774,7 +774,7 @@ func (self *Message) GetFileName() string {
 type Flag C.notmuch_message_flag_t
 
 const (
-	MESSAGE_FLAG_MATCH Flag = 0
+	MESSAGE_FLAG_MATCH Flag = iota
 )
 
 /* Get a value of a flag for the email corresponding to 'message'. */
-- 
1.7.7.6

Re: [PATCH 1/7] go: Use iota in enum bindings

[Austin Clements]

Hah.  I guess nobody has tried to modify a notmuch database using the
Go bindings.

Could this instead assign the constants to
C.NOTMUCH_DATABASE_MODE_READ_ONLY, etc, rather than duplicating their
values?  It would be nice to do that for the Status values as well
(which are correctly using iota, at least).

Quoth Adrien Bustany on Jul 18 at  9:34 pm:
> Using iota is the correct way to get the values in the enum increment
> automatically. The old code would just set all the enum values to 0.
> ---
>  bindings/go/src/notmuch/notmuch.go |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
> index 00bd53a..ecd7418 100644
> --- a/bindings/go/src/notmuch/notmuch.go
> +++ b/bindings/go/src/notmuch/notmuch.go
> @@ -86,7 +86,7 @@ type Filenames struct {
>  type DatabaseMode C.notmuch_database_mode_t
>  
>  const (
> -	DATABASE_MODE_READ_ONLY DatabaseMode = 0
> +	DATABASE_MODE_READ_ONLY DatabaseMode = iota
>  	DATABASE_MODE_READ_WRITE
>  )
>  
> @@ -386,7 +386,7 @@ func (self *Database) CreateQuery(query string) *Query {
>  type Sort C.notmuch_sort_t
>  
>  const (
> -	SORT_OLDEST_FIRST Sort = 0
> +	SORT_OLDEST_FIRST Sort = iota
>  	SORT_NEWEST_FIRST
>  	SORT_MESSAGE_ID
>  	SORT_UNSORTED
> @@ -774,7 +774,7 @@ func (self *Message) GetFileName() string {
>  type Flag C.notmuch_message_flag_t
>  
>  const (
> -	MESSAGE_FLAG_MATCH Flag = 0
> +	MESSAGE_FLAG_MATCH Flag = iota
>  )
>  
>  /* Get a value of a flag for the email corresponding to 'message'. */

Re: [PATCH 1/7] go: Use iota in enum bindings

[Sebastien Binet]

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

Austin Clements <amdragon@MIT.EDU> writes:

> Hah.  I guess nobody has tried to modify a notmuch database using the
> Go bindings.
>
> Could this instead assign the constants to
> C.NOTMUCH_DATABASE_MODE_READ_ONLY, etc, rather than duplicating their
> values?  It would be nice to do that for the Status values as well
> (which are correctly using iota, at least).

yep.
I have a few fixes like that in my repo:
https://github.com/sbinet/notmuch/commits/dev/go-bindings/bindings/go/src/notmuch

https://github.com/sbinet/notmuch/commit/38713de1b90a29b66b2a7f310065a3ffcf527d9b#L1L88

-s

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

[PATCH 2/7] go: Add missing MESSAGE_FLAG_EXCLUDED in Flag enum

[Adrien Bustany]

---
 bindings/go/src/notmuch/notmuch.go |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
index ecd7418..1d77fd2 100644
--- a/bindings/go/src/notmuch/notmuch.go
+++ b/bindings/go/src/notmuch/notmuch.go
@@ -775,6 +775,7 @@ type Flag C.notmuch_message_flag_t
 
 const (
 	MESSAGE_FLAG_MATCH Flag = iota
+	MESSAGE_FLAG_EXCLUDED
 )
 
 /* Get a value of a flag for the email corresponding to 'message'. */
-- 
1.7.7.6

[PATCH 3/7] go: Allow notmuch objects to be garbage collected

[Adrien Bustany]

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
 }
 
 /* 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. */
-- 
1.7.7.6

Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected

[Austin Clements]

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. */

Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected

[Adrien Bustany]

Le 18/07/2012 23:40, Austin Clements a écrit :
> 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.

Definitely, I tend to comment in the commit message and forget about the 
code...

>
> Just one inline comment below.  Otherwise, I think this is all
> correct.

Agree with the comment, the Database should be the parent. I guess I 
wasn't sure of the talloc parenting.

>
> 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?

Reproducing the hierarchy is probably error prone, and not that simple 
indeed :/
I haven't checked at all the way you suggest, but if we use 
talloc_reference/unlink, we get the same issue no?
- If we do for each new wrapped object talloc_reference(NULL, 
wrapped_object), the the object will be kept alive until we 
talloc_unlink(NULL, wrapped_object), but what about its parents? For 
example will doing that on a notmuch_message_t keep the 
notmuch_messages_t alive?
- If we do talloc_reference(parent, wrapped), then we reproduce the 
hierarchy again?

Note that I have 0 experience with talloc, so I might as well be getting 
things wrong here.

>
> 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. */

Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected

[Austin Clements]

Quoth Adrien Bustany on Jul 19 at  9:25 pm:
> Le 18/07/2012 23:40, Austin Clements a écrit :
> >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.
> 
> Definitely, I tend to comment in the commit message and forget about
> the code...
> 
> >
> >Just one inline comment below.  Otherwise, I think this is all
> >correct.
> 
> Agree with the comment, the Database should be the parent. I guess I
> wasn't sure of the talloc parenting.
> 
> >
> >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?
> 
> Reproducing the hierarchy is probably error prone, and not that
> simple indeed :/
> I haven't checked at all the way you suggest, but if we use
> talloc_reference/unlink, we get the same issue no?
> - If we do for each new wrapped object talloc_reference(NULL,
> wrapped_object), the the object will be kept alive until we
> talloc_unlink(NULL, wrapped_object), but what about its parents? For
> example will doing that on a notmuch_message_t keep the
> notmuch_messages_t alive?

Hmm.  This is what I was thinking.  You have an interesting point; I
think it's slightly wrong, but it exposes something deeper.  I believe
there are two different things going on here: some of the talloc
relationships are for convenience, while some are structural.  In the
former case, I'm pretty sure my suggestion will work, but in the
latter case the objects should *never* be freed by the finalizer!

For example, notmuch_query_search_messages returns a new
notmuch_messages_t with the query as the talloc parent, but that
notmuch_messages_t doesn't depend on the query object; this is just so
you can conveniently delete everything retrieved from the query by
deleting the query.  In this case, you can either use parent
references like you did---which will prevent a double-free by forcing
destruction to happen from the leaves up but at the cost of having to
encode these relationships and of extending the parent object
lifetimes beyond what's strictly necessary---or you can use my
suggestion of creating an additional talloc reference.

However, in your example, the notmuch_message_t's are structurally
related to the notmuch_messages_t from whence they came.  They're all
part of one data structure and hence it *never* makes sense for a
caller to delete the notmuch_message_t's.  For example, even with the
code in this patch, I think the following could lead to a crash:

1. Obtain a Messages object, say ms.
2. m1 := ms.Get()
3. m1 = nil
4. m2 := ms.Get()
5. m2.whatever()

If a garbage collection happens between steps 3 and 4, the Message in
m1 will get finalized and destroyed.  But step 4 will return the same,
now dangling, pointer, leading to a potential crash in step 5.

Maybe the answer in the structural case is to include the parent
pointer in the Go struct and not set a finalizer on the child?  That
way, if there's a Go reference to the parent wrapper, it won't go away
and the children won't get destroyed (collecting wrappers of children
is fine) and if there's a Go reference to the child wrapper, it will
keep the parent alive so it won't get destroyed and neither will the
child.

> - If we do talloc_reference(parent, wrapped), then we reproduce the
> hierarchy again?
> 
> Note that I have 0 experience with talloc, so I might as well be
> getting things wrong here.
> 
> >
> >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. */

Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected

[Adrien Bustany]

Le 20/07/2012 06:23, Austin Clements a écrit :
> Quoth Adrien Bustany on Jul 19 at  9:25 pm:
>> Le 18/07/2012 23:40, Austin Clements a écrit :
>>> 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.
>>
>> Definitely, I tend to comment in the commit message and forget about
>> the code...
>>
>>>
>>> Just one inline comment below.  Otherwise, I think this is all
>>> correct.
>>
>> Agree with the comment, the Database should be the parent. I guess I
>> wasn't sure of the talloc parenting.
>>
>>>
>>> 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?
>>
>> Reproducing the hierarchy is probably error prone, and not that
>> simple indeed :/
>> I haven't checked at all the way you suggest, but if we use
>> talloc_reference/unlink, we get the same issue no?
>> - If we do for each new wrapped object talloc_reference(NULL,
>> wrapped_object), the the object will be kept alive until we
>> talloc_unlink(NULL, wrapped_object), but what about its parents? For
>> example will doing that on a notmuch_message_t keep the
>> notmuch_messages_t alive?
>
> Hmm.  This is what I was thinking.  You have an interesting point; I
> think it's slightly wrong, but it exposes something deeper.  I believe
> there are two different things going on here: some of the talloc
> relationships are for convenience, while some are structural.  In the
> former case, I'm pretty sure my suggestion will work, but in the
> latter case the objects should *never* be freed by the finalizer!
>
> For example, notmuch_query_search_messages returns a new
> notmuch_messages_t with the query as the talloc parent, but that
> notmuch_messages_t doesn't depend on the query object; this is just so
> you can conveniently delete everything retrieved from the query by
> deleting the query.  In this case, you can either use parent
> references like you did---which will prevent a double-free by forcing
> destruction to happen from the leaves up but at the cost of having to
> encode these relationships and of extending the parent object
> lifetimes beyond what's strictly necessary---or you can use my
> suggestion of creating an additional talloc reference.

Actually, checking the code of notmuch_query_search_messages, it seems 
that the notmuch_messages_t (and the notmuch_message_t as well) object 
*does* depend on the database and the query... So in that case I think 
we need the "owner" Object reference as I currently have (we want the 
Messages to keep the Query alive, and the Query keeps the Database alive).
That said, you example below looks valid, and it seems I'll need to add 
a flag to createMessage() (and some others) to disable the SetFinalizer 
call for certain instances (we probably want to keep it for eg. 
SearchMessageByFilename).

- The candidates I found for adding a tmalloc reference and not a "full" 
Go reference (therefore preventing to keep the parent alive too long 
needlessly) are GetAllTags, Thread.GetTags, Messages.CollectTags, and 
Message.GetTags (those are basically string lists)

- The methods for which I should remove the SetFinalizer on the wrapper 
(as you showed in the example below) while keeping the Go reference are 
Threads.Get and Messages.Get

I would also maybe remove all the Destroy() functions, since they now 
seem more dangerous than anything else...

I tried to write a test using runtime.GC to test the behaviour of the 
bindings, but for some reasons some cases which are supposed to crash 
don't, which makes me sceptical about the validity of the test :-/

Cheers

Adrien

>
> However, in your example, the notmuch_message_t's are structurally
> related to the notmuch_messages_t from whence they came.  They're all
> part of one data structure and hence it *never* makes sense for a
> caller to delete the notmuch_message_t's.  For example, even with the
> code in this patch, I think the following could lead to a crash:
>
> 1. Obtain a Messages object, say ms.
> 2. m1 := ms.Get()
> 3. m1 = nil
> 4. m2 := ms.Get()
> 5. m2.whatever()
>
> If a garbage collection happens between steps 3 and 4, the Message in
> m1 will get finalized and destroyed.  But step 4 will return the same,
> now dangling, pointer, leading to a potential crash in step 5.
>
> Maybe the answer in the structural case is to include the parent
> pointer in the Go struct and not set a finalizer on the child?  That
> way, if there's a Go reference to the parent wrapper, it won't go away
> and the children won't get destroyed (collecting wrappers of children
> is fine) and if there's a Go reference to the child wrapper, it will
> keep the parent alive so it won't get destroyed and neither will the
> child.
>
>> - If we do talloc_reference(parent, wrapped), then we reproduce the
>> hierarchy again?
>>
>> Note that I have 0 experience with talloc, so I might as well be
>> getting things wrong here.
>>
>>>
>>> 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. */

Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected

[Austin Clements]

Quoth Adrien Bustany on Jul 24 at  1:03 am:
> Le 20/07/2012 06:23, Austin Clements a écrit :
> >Quoth Adrien Bustany on Jul 19 at  9:25 pm:
> >>Le 18/07/2012 23:40, Austin Clements a écrit :
> >>>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.
> >>
> >>Definitely, I tend to comment in the commit message and forget about
> >>the code...
> >>
> >>>
> >>>Just one inline comment below.  Otherwise, I think this is all
> >>>correct.
> >>
> >>Agree with the comment, the Database should be the parent. I guess I
> >>wasn't sure of the talloc parenting.
> >>
> >>>
> >>>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?
> >>
> >>Reproducing the hierarchy is probably error prone, and not that
> >>simple indeed :/
> >>I haven't checked at all the way you suggest, but if we use
> >>talloc_reference/unlink, we get the same issue no?
> >>- If we do for each new wrapped object talloc_reference(NULL,
> >>wrapped_object), the the object will be kept alive until we
> >>talloc_unlink(NULL, wrapped_object), but what about its parents? For
> >>example will doing that on a notmuch_message_t keep the
> >>notmuch_messages_t alive?
> >
> >Hmm.  This is what I was thinking.  You have an interesting point; I
> >think it's slightly wrong, but it exposes something deeper.  I believe
> >there are two different things going on here: some of the talloc
> >relationships are for convenience, while some are structural.  In the
> >former case, I'm pretty sure my suggestion will work, but in the
> >latter case the objects should *never* be freed by the finalizer!
> >
> >For example, notmuch_query_search_messages returns a new
> >notmuch_messages_t with the query as the talloc parent, but that
> >notmuch_messages_t doesn't depend on the query object; this is just so
> >you can conveniently delete everything retrieved from the query by
> >deleting the query.  In this case, you can either use parent
> >references like you did---which will prevent a double-free by forcing
> >destruction to happen from the leaves up but at the cost of having to
> >encode these relationships and of extending the parent object
> >lifetimes beyond what's strictly necessary---or you can use my
> >suggestion of creating an additional talloc reference.
> 
> Actually, checking the code of notmuch_query_search_messages, it
> seems that the notmuch_messages_t (and the notmuch_message_t as
> well) object *does* depend on the database and the query... So in
> that case I think we need the "owner" Object reference as I
> currently have (we want the Messages to keep the Query alive, and
> the Query keeps the Database alive).

It does depends on the database (I think just about everything depends
on the database, directly or indirectly, so I suppose everything will
need some parent pointer), but could you explain how it depends on the
query?  It uses the MSet derived from the query, but Xapian internally
handles the sharing and referencing counting of all of its objects.

> That said, you example below looks valid, and it seems I'll need to
> add a flag to createMessage() (and some others) to disable the
> SetFinalizer call for certain instances (we probably want to keep it
> for eg. SearchMessageByFilename).
> 
> - The candidates I found for adding a tmalloc reference and not a
> "full" Go reference (therefore preventing to keep the parent alive
> too long needlessly) are GetAllTags, Thread.GetTags,
> Messages.CollectTags, and Message.GetTags (those are basically
> string lists)

Sounds reasonable (but I haven't gone through carefully).

> - The methods for which I should remove the SetFinalizer on the
> wrapper (as you showed in the example below) while keeping the Go
> reference are Threads.Get and Messages.Get

Sounds right.  I think those are the only cases where the object is
still owned by a container, other than strings (which Go has to copy
anyway).

> I would also maybe remove all the Destroy() functions, since they
> now seem more dangerous than anything else...

Yeah, probably.

> I tried to write a test using runtime.GC to test the behaviour of
> the bindings, but for some reasons some cases which are supposed to
> crash don't, which makes me sceptical about the validity of the test
> :-/

Hmm.  Go's collector is partially conservative, IIRC, so maybe it's
following a technically dead pointer?

> Cheers
> 
> Adrien
> 
> >
> >However, in your example, the notmuch_message_t's are structurally
> >related to the notmuch_messages_t from whence they came.  They're all
> >part of one data structure and hence it *never* makes sense for a
> >caller to delete the notmuch_message_t's.  For example, even with the
> >code in this patch, I think the following could lead to a crash:
> >
> >1. Obtain a Messages object, say ms.
> >2. m1 := ms.Get()
> >3. m1 = nil
> >4. m2 := ms.Get()
> >5. m2.whatever()
> >
> >If a garbage collection happens between steps 3 and 4, the Message in
> >m1 will get finalized and destroyed.  But step 4 will return the same,
> >now dangling, pointer, leading to a potential crash in step 5.
> >
> >Maybe the answer in the structural case is to include the parent
> >pointer in the Go struct and not set a finalizer on the child?  That
> >way, if there's a Go reference to the parent wrapper, it won't go away
> >and the children won't get destroyed (collecting wrappers of children
> >is fine) and if there's a Go reference to the child wrapper, it will
> >keep the parent alive so it won't get destroyed and neither will the
> >child.
> >
> >>- If we do talloc_reference(parent, wrapped), then we reproduce the
> >>hierarchy again?
> >>
> >>Note that I have 0 experience with talloc, so I might as well be
> >>getting things wrong here.
> >>
> >>>
> >>>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. */
> 
> 

-- 
Austin Clements                                      MIT/'06/PhD/CSAIL
amdragon@mit.edu                           http://web.mit.edu/amdragon
       Somewhere in the dream we call reality you will find me,
              searching for the reality we call dreams.

Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected

[Ethan Glasser-Camp]

Adrien Bustany <adrien@bustany.org> writes:

> 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).

Hi Adrien! This whole series is marked moreinfo, but I don't think
that's just. It looks like there were some unresolved issues about
reference tracking and garbage collection, and some suggestions to use
the C values of enums instead of regenerating them with iota, but
there's definitely valid code that I assume would be useful if anyone
ever wanted to write in Go ;). Are you figuring to clean this series up?

This comment should s/wraper/wrapper/.

Ethan

Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected

[Adrien Bustany]

Le 19/10/2012 06:55, Ethan Glasser-Camp a écrit :
> Adrien Bustany <adrien@bustany.org> writes:
>
>> 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).
>
> Hi Adrien! This whole series is marked moreinfo, but I don't think
> that's just. It looks like there were some unresolved issues about
> reference tracking and garbage collection, and some suggestions to use
> the C values of enums instead of regenerating them with iota, but
> there's definitely valid code that I assume would be useful if anyone
> ever wanted to write in Go ;). Are you figuring to clean this series up?
>
> This comment should s/wraper/wrapper/.
>
> Ethan
>

Hello Ethan,

thanks for the heads up, I still have this on my table, and yes there is 
additional work to do for the patches to be really clean. I can't give 
an estimate for now, let's hope sooner than later :/

Cheers

Adrien

[PATCH 4/7] go: Make Destroy functions safe to call several times

[Adrien Bustany]

Those methods were already checking if the underlying C object was NULL,
but they were not setting the pointer to NULL after destroying it.
---
 bindings/go/src/notmuch/notmuch.go |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
index 3f436a0..d8b2739 100644
--- a/bindings/go/src/notmuch/notmuch.go
+++ b/bindings/go/src/notmuch/notmuch.go
@@ -634,6 +634,7 @@ func (self *Query) SearchMessages() *Messages {
 func (self *Query) Destroy() {
 	if self.query != nil {
 		C.notmuch_query_destroy(self.query)
+		self.query = nil
 	}
 }
 
@@ -680,6 +681,7 @@ func (self *Threads) Valid() bool {
 func (self *Threads) Destroy() {
 	if self.threads != nil {
 		C.notmuch_threads_destroy(self.threads)
+		self.threads = nil
 	}
 }
 
@@ -1143,6 +1145,7 @@ func (self *Message) Destroy() {
 		return
 	}
 	C.notmuch_message_destroy(self.message)
+	self.message = nil
 }
 
 /* Is the given 'tags' iterator pointing at a valid tag.
@@ -1214,6 +1217,7 @@ func (self *Tags) Destroy() {
 		return
 	}
 	C.notmuch_tags_destroy(self.tags)
+	self.tags = nil
 }
 
 // TODO: wrap notmuch_directory_<fct>
@@ -1224,6 +1228,7 @@ func (self *Directory) Destroy() {
 		return
 	}
 	C.notmuch_directory_destroy(self.dir)
+	self.dir = nil
 }
 
 // TODO: wrap notmuch_filenames_<fct>
@@ -1242,6 +1247,7 @@ func (self *Filenames) Destroy() {
 		return
 	}
 	C.notmuch_filenames_destroy(self.fnames)
+	self.fnames = nil
 }
 
 /* EOF */
-- 
1.7.7.6

[PATCH 5/7] go: Partially bind notmuch_database_upgrade

[Adrien Bustany]

This binding does not handle the progress callback, but at least allows
opening and upgrading a database if needed.
---
 bindings/go/src/notmuch/notmuch.go |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
index d8b2739..384d5a5 100644
--- a/bindings/go/src/notmuch/notmuch.go
+++ b/bindings/go/src/notmuch/notmuch.go
@@ -296,7 +296,18 @@ func (self *Database) NeedsUpgrade() bool {
 	return true
 }
 
-// TODO: notmuch_database_upgrade
+// TODO: Proper notmuch_database_upgrade
+/* Upgrade the current database.
+ *
+ * After opening a database in read-write mode, the client should
+ * check if an upgrade is needed (notmuch_database_needs_upgrade) and
+ * if so, upgrade with this function before making any modifications.
+ */
+func (self *Database) Upgrade() Status {
+	st := Status(C.notmuch_database_upgrade(self.db, nil, nil));
+
+	return st;
+}
 
 /* Retrieve a directory object from the database for 'path'.
  *
-- 
1.7.7.6

Re: [PATCH 5/7] go: Partially bind notmuch_database_upgrade

[Austin Clements]

Quoth Adrien Bustany on Jul 18 at  9:34 pm:
> This binding does not handle the progress callback, but at least allows
> opening and upgrading a database if needed.
> ---
>  bindings/go/src/notmuch/notmuch.go |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
> index d8b2739..384d5a5 100644
> --- a/bindings/go/src/notmuch/notmuch.go
> +++ b/bindings/go/src/notmuch/notmuch.go
> @@ -296,7 +296,18 @@ func (self *Database) NeedsUpgrade() bool {
>  	return true
>  }
>  
> -// TODO: notmuch_database_upgrade
> +// TODO: Proper notmuch_database_upgrade
> +/* Upgrade the current database.
> + *
> + * After opening a database in read-write mode, the client should
> + * check if an upgrade is needed (notmuch_database_needs_upgrade) and
> + * if so, upgrade with this function before making any modifications.
> + */
> +func (self *Database) Upgrade() Status {
> +	st := Status(C.notmuch_database_upgrade(self.db, nil, nil));
> +
> +	return st;

*gasp*  Semicolons!

> +}
>  
>  /* Retrieve a directory object from the database for 'path'.
>   *

[PATCH 6/7] go: Bind notmuch_database_find_message_by_filename

[Adrien Bustany]

---
 bindings/go/src/notmuch/notmuch.go |   39 ++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
index 384d5a5..be4cb8c 100644
--- a/bindings/go/src/notmuch/notmuch.go
+++ b/bindings/go/src/notmuch/notmuch.go
@@ -453,6 +453,45 @@ func (self *Database) FindMessage(message_id string) (*Message, Status) {
 	return createMessage(msg, nil), st
 }
 
+/* Find a message with the given filename.
+ *
+ * If the database contains a message with the given filename then, on
+ * successful return (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to
+ * a message object. The caller should call notmuch_message_destroy when done
+ * with the message.
+ *
+ * On any failure or when the message is not found, this function initializes
+ * '*message' to NULL. This means, when NOTMUCH_STATUS_SUCCESS is returned, the
+ * caller is supposed to check '*message' for NULL to find out whether the
+ * message with the given filename is found.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successful return, check '*message'
+ *
+ * NOTMUCH_STATUS_NULL_POINTER: The given 'message' argument is NULL
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory, creating the message object
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred
+ */
+func (self *Database) FindMessageByFilename(filename string) (*Message, Status) {
+
+	var c_msg_filename *C.char = C.CString(filename)
+	defer C.free(unsafe.Pointer(c_msg_filename))
+
+	if c_msg_filename == nil {
+		return nil, STATUS_OUT_OF_MEMORY
+	}
+
+	var msg *C.notmuch_message_t
+	st := Status(C.notmuch_database_find_message_by_filename(self.db, c_msg_filename, &msg))
+	if st != STATUS_SUCCESS {
+		return nil, st
+	}
+	return createMessage(msg, nil), st
+}
+
 /* Return a list of all tags found in the database.
  *
  * This function creates a list of all tags found in the database. The
-- 
1.7.7.6

[PATCH 7/7] go: Bind notmuch_thread_t functions

[Adrien Bustany]

---
 bindings/go/src/notmuch/notmuch.go |  253 +++++++++++++++++++++++++++++++++++-
 1 files changed, 252 insertions(+), 1 deletions(-)

diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
index be4cb8c..f667dbb 100644
--- a/bindings/go/src/notmuch/notmuch.go
+++ b/bindings/go/src/notmuch/notmuch.go
@@ -12,6 +12,8 @@ package notmuch
 */
 import "C"
 import "runtime"
+import "strings"
+import "time"
 import "unsafe"
 
 // Status codes used for the return values of most functions
@@ -700,7 +702,20 @@ func (self *Query) CountMessages() uint {
 	return uint(C.notmuch_query_count_messages(self.query))
 }
 
-// TODO: wrap threads and thread
+/* Return the number of threads matching a search.
+ *
+ * This function performs a search and returns the number of unique thread IDs
+ * in the matching messages. This is the same as number of threads matching a
+ * search.
+ *
+ * Note that this is a significantly heavier operation than
+ * notmuch_query_count_messages().
+ *
+ * If an error occurs, this function may return 0.
+ */
+func (self *Query) CountThreads() uint {
+	return uint(C.notmuch_query_count_threads(self.query))
+}
 
 /* Is the given 'threads' iterator pointing at a valid thread.
  *
@@ -722,6 +737,45 @@ func (self *Threads) Valid() bool {
 	return true
 }
 
+/* Get the current thread from 'threads' as a notmuch_thread_t.
+ *
+ * Note: The returned thread belongs to 'threads' and has a lifetime
+ * identical to it (and the query to which it belongs).
+ *
+ * See the documentation of notmuch_query_search_threads for example
+ * code showing how to iterate over a notmuch_threads_t object.
+ *
+ * If an out-of-memory situation occurs, this function will return
+ * NULL.
+ */
+func (self *Threads) Get() *Thread {
+	if self.threads == nil {
+		return nil
+	}
+	thread := C.notmuch_threads_get(self.threads)
+	if thread == nil {
+		return nil
+	}
+	return createThread(thread, self)
+}
+
+/* Move the 'threads' iterator to the next thread.
+ *
+ * If 'threads' is already pointing at the last thread then the
+ * iterator will be moved to a point just beyond that last thread,
+ * (where notmuch_threads_valid will return FALSE and
+ * notmuch_threads_get will return NULL).
+ *
+ * See the documentation of notmuch_query_search_threads for example
+ * code showing how to iterate over a notmuch_threads_t object.
+ */
+func (self *Threads) MoveToNext() {
+	if self.threads == nil {
+		return
+	}
+	C.notmuch_threads_move_to_next(self.threads)
+}
+
 /* Destroy a notmuch_threads_t object.
  *
  * It's not strictly necessary to call this function. All memory from
@@ -735,6 +789,203 @@ func (self *Threads) Destroy() {
 	}
 }
 
+/* Get the thread ID of 'thread'.
+ *
+ * The returned string belongs to 'thread' and as such, should not be
+ * modified by the caller and will only be valid for as long as the
+ * thread is valid, (which is until notmuch_thread_destroy or until
+ * the query from which it derived is destroyed).
+ */
+func (self *Thread) GetThreadId() string {
+	if self.thread == nil {
+		return ""
+	}
+	id := C.notmuch_thread_get_thread_id(self.thread)
+
+	if id == nil {
+		return ""
+	}
+
+	return C.GoString(id)
+}
+
+/* Get the total number of messages in 'thread'.
+ *
+ * This count consists of all messages in the database belonging to
+ * this thread. Contrast with notmuch_thread_get_matched_messages() .
+ */
+func (self *Thread) GetTotalMessages() int {
+	if self.thread == nil {
+		return 0
+	}
+	return int(C.notmuch_thread_get_total_messages(self.thread))
+}
+
+/* Get a notmuch_messages_t iterator for the top-level messages in
+ * 'thread'.
+ *
+ * This iterator will not necessarily iterate over all of the messages
+ * in the thread. It will only iterate over the messages in the thread
+ * which are not replies to other messages in the thread.
+ *
+ * To iterate over all messages in the thread, the caller will need to
+ * iterate over the result of notmuch_message_get_replies for each
+ * top-level message (and do that recursively for the resulting
+ * messages, etc.).
+ */
+func (self *Thread) GetToplevelMessages() *Messages {
+	if self.thread == nil {
+		return nil
+	}
+	msgs := C.notmuch_thread_get_toplevel_messages(self.thread)
+	if msgs == nil {
+		return nil
+	}
+	return createMessages(msgs, self)
+}
+
+/* Get a notmuch_messages_t iterator for the top-level messages in
+ * 'thread'.
+ *
+ * This iterator will not necessarily iterate over all of the messages
+ * in the thread. It will only iterate over the messages in the thread
+ * which are not replies to other messages in the thread.
+ *
+ * To iterate over all messages in the thread, the caller will need to
+ * iterate over the result of notmuch_message_get_replies for each
+ * top-level message (and do that recursively for the resulting
+ * messages, etc.).
+ */
+func (self *Thread) GetMatchedMessages() int {
+	if self.thread == nil {
+		return 0
+	}
+	return int(C.notmuch_thread_get_matched_messages(self.thread))
+}
+
+/* Get a notmuch_messages_t iterator for the top-level messages in
+ * 'thread'.
+ *
+ * This iterator will not necessarily iterate over all of the messages
+ * in the thread. It will only iterate over the messages in the thread
+ * which are not replies to other messages in the thread.
+ *
+ * To iterate over all messages in the thread, the caller will need to
+ * iterate over the result of notmuch_message_get_replies for each
+ * top-level message (and do that recursively for the resulting
+ * messages, etc.).
+ */
+func (self *Thread) GetAuthors() []string {
+	if self.thread == nil {
+		return make([]string, 0)
+	}
+	authors_str := C.notmuch_thread_get_authors(self.thread)
+
+	if authors_str == nil {
+		return make([]string, 0)
+	}
+
+	return strings.Split(C.GoString(authors_str), ", ")
+}
+
+/* Get the subject of 'thread'
+ *
+ * The subject is taken from the first message (according to the query
+ * order---see notmuch_query_set_sort) in the query results that
+ * belongs to this thread.
+ *
+ * The returned string belongs to 'thread' and as such, should not be
+ * modified by the caller and will only be valid for as long as the
+ * thread is valid, (which is until notmuch_thread_destroy or until
+ * the query from which it derived is destroyed).
+ */
+func (self *Thread) GetSubject() string {
+	if self.thread == nil {
+		return ""
+	}
+	subject := C.notmuch_thread_get_subject(self.thread)
+
+	if subject == nil {
+		return ""
+	}
+
+	return C.GoString(subject)
+}
+
+/* Get the date of the oldest message in 'thread' as a time_t value.
+ */
+func (self *Thread) GetOldestDate() time.Time {
+	if self.thread == nil {
+		return time.Unix(0, 0)
+	}
+	return time.Unix(int64(C.notmuch_thread_get_oldest_date(self.thread)), 0)
+}
+
+/* Get the date of the newest message in 'thread' as a time_t value.
+ */
+func (self *Thread) GetNewestDate() time.Time {
+	if self.thread == nil {
+		return time.Unix(0, 0)
+	}
+	return time.Unix(int64(C.notmuch_thread_get_oldest_date(self.thread)), 0)
+}
+
+/* Get the tags for 'thread', returning a notmuch_tags_t object which
+ * can be used to iterate over all tags.
+ *
+ * Note: In the Notmuch database, tags are stored on individual
+ * messages, not on threads. So the tags returned here will be all
+ * tags of the messages which matched the search and which belong to
+ * this thread.
+ *
+ * The tags object is owned by the thread and as such, will only be
+ * valid for as long as the thread is valid, (for example, until
+ * notmuch_thread_destroy or until the query from which it derived is
+ * destroyed).
+ *
+ * Typical usage might be:
+ *
+ *     notmuch_thread_t *thread;
+ *     notmuch_tags_t *tags;
+ *     const char *tag;
+ *
+ *     thread = notmuch_threads_get (threads);
+ *
+ *     for (tags = notmuch_thread_get_tags (thread);
+ *          notmuch_tags_valid (tags);
+ *          notmuch_result_move_to_next (tags))
+ *     {
+ *         tag = notmuch_tags_get (tags);
+ *         ....
+ *     }
+ *
+ *     notmuch_thread_destroy (thread);
+ *
+ * Note that there's no explicit destructor needed for the
+ * notmuch_tags_t object. (For consistency, we do provide a
+ * notmuch_tags_destroy function, but there's no good reason to call
+ * it if the message is about to be destroyed).
+ */
+func (self *Thread) GetTags() *Tags {
+	if self.thread == nil {
+		return nil
+	}
+	tags := C.notmuch_thread_get_tags(self.thread)
+	if tags == nil {
+		return nil
+	}
+	return createTags(tags, self)
+}
+
+/* Destroy a notmuch_thread_t object. */
+func (self *Thread) Destroy() {
+	if self.thread == nil {
+		return
+	}
+	C.notmuch_thread_destroy(self.thread)
+	self.thread = nil
+}
+
 /* Is the given 'messages' iterator pointing at a valid message.
  *
  * When this function returns TRUE, notmuch_messages_get will return a
-- 
1.7.7.6

Re: [PATCH 7/7] go: Bind notmuch_thread_t functions

[Austin Clements]

Quoth Adrien Bustany on Jul 18 at  9:34 pm:
> ---
>  bindings/go/src/notmuch/notmuch.go |  253 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 252 insertions(+), 1 deletions(-)
> 
> diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
> index be4cb8c..f667dbb 100644
> --- a/bindings/go/src/notmuch/notmuch.go
> +++ b/bindings/go/src/notmuch/notmuch.go
> @@ -12,6 +12,8 @@ package notmuch
>  */
>  import "C"
>  import "runtime"
> +import "strings"
> +import "time"
>  import "unsafe"
>  
>  // Status codes used for the return values of most functions
> @@ -700,7 +702,20 @@ func (self *Query) CountMessages() uint {
>  	return uint(C.notmuch_query_count_messages(self.query))
>  }
>  
> -// TODO: wrap threads and thread
> +/* Return the number of threads matching a search.
> + *
> + * This function performs a search and returns the number of unique thread IDs
> + * in the matching messages. This is the same as number of threads matching a
> + * search.
> + *
> + * Note that this is a significantly heavier operation than
> + * notmuch_query_count_messages().
> + *
> + * If an error occurs, this function may return 0.
> + */
> +func (self *Query) CountThreads() uint {
> +	return uint(C.notmuch_query_count_threads(self.query))
> +}
>  
>  /* Is the given 'threads' iterator pointing at a valid thread.
>   *
> @@ -722,6 +737,45 @@ func (self *Threads) Valid() bool {
>  	return true
>  }
>  
> +/* Get the current thread from 'threads' as a notmuch_thread_t.
> + *
> + * Note: The returned thread belongs to 'threads' and has a lifetime
> + * identical to it (and the query to which it belongs).
> + *
> + * See the documentation of notmuch_query_search_threads for example
> + * code showing how to iterate over a notmuch_threads_t object.
> + *
> + * If an out-of-memory situation occurs, this function will return
> + * NULL.
> + */
> +func (self *Threads) Get() *Thread {
> +	if self.threads == nil {
> +		return nil
> +	}
> +	thread := C.notmuch_threads_get(self.threads)
> +	if thread == nil {
> +		return nil
> +	}
> +	return createThread(thread, self)
> +}
> +
> +/* Move the 'threads' iterator to the next thread.
> + *
> + * If 'threads' is already pointing at the last thread then the
> + * iterator will be moved to a point just beyond that last thread,
> + * (where notmuch_threads_valid will return FALSE and
> + * notmuch_threads_get will return NULL).
> + *
> + * See the documentation of notmuch_query_search_threads for example
> + * code showing how to iterate over a notmuch_threads_t object.
> + */
> +func (self *Threads) MoveToNext() {
> +	if self.threads == nil {
> +		return
> +	}
> +	C.notmuch_threads_move_to_next(self.threads)
> +}
> +
>  /* Destroy a notmuch_threads_t object.
>   *
>   * It's not strictly necessary to call this function. All memory from
> @@ -735,6 +789,203 @@ func (self *Threads) Destroy() {
>  	}
>  }
>  
> +/* Get the thread ID of 'thread'.
> + *
> + * The returned string belongs to 'thread' and as such, should not be
> + * modified by the caller and will only be valid for as long as the
> + * thread is valid, (which is until notmuch_thread_destroy or until
> + * the query from which it derived is destroyed).
> + */
> +func (self *Thread) GetThreadId() string {
> +	if self.thread == nil {
> +		return ""
> +	}
> +	id := C.notmuch_thread_get_thread_id(self.thread)
> +
> +	if id == nil {
> +		return ""
> +	}
> +
> +	return C.GoString(id)
> +}
> +
> +/* Get the total number of messages in 'thread'.
> + *
> + * This count consists of all messages in the database belonging to
> + * this thread. Contrast with notmuch_thread_get_matched_messages() .
> + */
> +func (self *Thread) GetTotalMessages() int {
> +	if self.thread == nil {
> +		return 0
> +	}
> +	return int(C.notmuch_thread_get_total_messages(self.thread))
> +}
> +
> +/* Get a notmuch_messages_t iterator for the top-level messages in
> + * 'thread'.
> + *
> + * This iterator will not necessarily iterate over all of the messages
> + * in the thread. It will only iterate over the messages in the thread
> + * which are not replies to other messages in the thread.
> + *
> + * To iterate over all messages in the thread, the caller will need to
> + * iterate over the result of notmuch_message_get_replies for each
> + * top-level message (and do that recursively for the resulting
> + * messages, etc.).
> + */
> +func (self *Thread) GetToplevelMessages() *Messages {
> +	if self.thread == nil {
> +		return nil
> +	}
> +	msgs := C.notmuch_thread_get_toplevel_messages(self.thread)
> +	if msgs == nil {
> +		return nil
> +	}
> +	return createMessages(msgs, self)
> +}
> +
> +/* Get a notmuch_messages_t iterator for the top-level messages in
> + * 'thread'.
> + *
> + * This iterator will not necessarily iterate over all of the messages
> + * in the thread. It will only iterate over the messages in the thread
> + * which are not replies to other messages in the thread.
> + *
> + * To iterate over all messages in the thread, the caller will need to
> + * iterate over the result of notmuch_message_get_replies for each
> + * top-level message (and do that recursively for the resulting
> + * messages, etc.).
> + */

Wrong comment.  (Same for the next two methods.)

> +func (self *Thread) GetMatchedMessages() int {
> +	if self.thread == nil {
> +		return 0
> +	}
> +	return int(C.notmuch_thread_get_matched_messages(self.thread))
> +}
> +
> +/* Get a notmuch_messages_t iterator for the top-level messages in
> + * 'thread'.
> + *
> + * This iterator will not necessarily iterate over all of the messages
> + * in the thread. It will only iterate over the messages in the thread
> + * which are not replies to other messages in the thread.
> + *
> + * To iterate over all messages in the thread, the caller will need to
> + * iterate over the result of notmuch_message_get_replies for each
> + * top-level message (and do that recursively for the resulting
> + * messages, etc.).
> + */
> +func (self *Thread) GetAuthors() []string {
> +	if self.thread == nil {
> +		return make([]string, 0)
> +	}
> +	authors_str := C.notmuch_thread_get_authors(self.thread)
> +
> +	if authors_str == nil {
> +		return make([]string, 0)
> +	}
> +
> +	return strings.Split(C.GoString(authors_str), ", ")
> +}
> +
> +/* Get the subject of 'thread'
> + *
> + * The subject is taken from the first message (according to the query
> + * order---see notmuch_query_set_sort) in the query results that
> + * belongs to this thread.
> + *
> + * The returned string belongs to 'thread' and as such, should not be
> + * modified by the caller and will only be valid for as long as the
> + * thread is valid, (which is until notmuch_thread_destroy or until
> + * the query from which it derived is destroyed).
> + */
> +func (self *Thread) GetSubject() string {
> +	if self.thread == nil {
> +		return ""
> +	}
> +	subject := C.notmuch_thread_get_subject(self.thread)
> +
> +	if subject == nil {
> +		return ""
> +	}
> +
> +	return C.GoString(subject)
> +}
> +
> +/* Get the date of the oldest message in 'thread' as a time_t value.
> + */
> +func (self *Thread) GetOldestDate() time.Time {
> +	if self.thread == nil {
> +		return time.Unix(0, 0)
> +	}
> +	return time.Unix(int64(C.notmuch_thread_get_oldest_date(self.thread)), 0)
> +}
> +
> +/* Get the date of the newest message in 'thread' as a time_t value.
> + */
> +func (self *Thread) GetNewestDate() time.Time {
> +	if self.thread == nil {
> +		return time.Unix(0, 0)
> +	}
> +	return time.Unix(int64(C.notmuch_thread_get_oldest_date(self.thread)), 0)
> +}
> +
> +/* Get the tags for 'thread', returning a notmuch_tags_t object which
> + * can be used to iterate over all tags.
> + *
> + * Note: In the Notmuch database, tags are stored on individual
> + * messages, not on threads. So the tags returned here will be all
> + * tags of the messages which matched the search and which belong to
> + * this thread.
> + *
> + * The tags object is owned by the thread and as such, will only be
> + * valid for as long as the thread is valid, (for example, until
> + * notmuch_thread_destroy or until the query from which it derived is
> + * destroyed).
> + *
> + * Typical usage might be:
> + *
> + *     notmuch_thread_t *thread;
> + *     notmuch_tags_t *tags;
> + *     const char *tag;
> + *
> + *     thread = notmuch_threads_get (threads);
> + *
> + *     for (tags = notmuch_thread_get_tags (thread);
> + *          notmuch_tags_valid (tags);
> + *          notmuch_result_move_to_next (tags))
> + *     {
> + *         tag = notmuch_tags_get (tags);
> + *         ....
> + *     }
> + *
> + *     notmuch_thread_destroy (thread);

I wonder if this example should be updated for Go?  OTOH, all of the
comments seem to be copied verbatim, to the point of referring to C
function names and status symbols.

> + *
> + * Note that there's no explicit destructor needed for the
> + * notmuch_tags_t object. (For consistency, we do provide a
> + * notmuch_tags_destroy function, but there's no good reason to call
> + * it if the message is about to be destroyed).
> + */
> +func (self *Thread) GetTags() *Tags {
> +	if self.thread == nil {
> +		return nil
> +	}
> +	tags := C.notmuch_thread_get_tags(self.thread)
> +	if tags == nil {
> +		return nil
> +	}
> +	return createTags(tags, self)
> +}
> +
> +/* Destroy a notmuch_thread_t object. */
> +func (self *Thread) Destroy() {
> +	if self.thread == nil {
> +		return
> +	}
> +	C.notmuch_thread_destroy(self.thread)
> +	self.thread = nil
> +}
> +
>  /* Is the given 'messages' iterator pointing at a valid message.
>   *
>   * When this function returns TRUE, notmuch_messages_get will return a

Re: [PATCH 0/7] Various fixes for the Go bindings

[Austin Clements]

This series looks good to me other than the few things I commented on.
It's nice to see the Go bindings get a bit of love!

Quoth Adrien Bustany on Jul 18 at  9:34 pm:
> The following patches fix some serious memory management issues with
> the Go bindings, and add some missing functions as well.
> 
> Adrien Bustany (7):
>   go: Use iota in enum bindings
>   go: Add missing MESSAGE_FLAG_EXCLUDED in Flag enum
>   go: Allow notmuch objects to be garbage collected
>   go: Make Destroy functions safe to call several times
>   go: Partially bind notmuch_database_upgrade
>   go: Bind notmuch_database_find_message_by_filename
>   go: Bind notmuch_thread_t functions
> 
>  bindings/go/src/notmuch/notmuch.go |  471 ++++++++++++++++++++++++++++++++++--
>  1 files changed, 447 insertions(+), 24 deletions(-)
> 

Re: [PATCH 0/7] Various fixes for the Go bindings

[Adrien Bustany]

Well, thanks for the quick review :)

I have updated all the patches to take your fixes into account, there is 
still the discussion left about the GC integration. Once we get that 
sorted out, I'll send the updated version.

Le 18/07/2012 23:51, Austin Clements a écrit :
> This series looks good to me other than the few things I commented on.
> It's nice to see the Go bindings get a bit of love!
>
> Quoth Adrien Bustany on Jul 18 at  9:34 pm:
>> The following patches fix some serious memory management issues with
>> the Go bindings, and add some missing functions as well.
>>
>> Adrien Bustany (7):
>>    go: Use iota in enum bindings
>>    go: Add missing MESSAGE_FLAG_EXCLUDED in Flag enum
>>    go: Allow notmuch objects to be garbage collected
>>    go: Make Destroy functions safe to call several times
>>    go: Partially bind notmuch_database_upgrade
>>    go: Bind notmuch_database_find_message_by_filename
>>    go: Bind notmuch_thread_t functions
>>
>>   bindings/go/src/notmuch/notmuch.go |  471 ++++++++++++++++++++++++++++++++++--
>>   1 files changed, 447 insertions(+), 24 deletions(-)
>>