From: Adrien Bustany Date: Thu, 19 Jul 2012 18:25:10 +0000 (+0300) Subject: Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=c20b18d1f7db7f38975ca21b89070cbf4d0e7174;p=notmuch-archives.git Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected --- diff --git a/ec/5ca22893220fb8ec4f9746d2e88c50cdc698f8 b/ec/5ca22893220fb8ec4f9746d2e88c50cdc698f8 new file mode 100644 index 000000000..dc3691bc6 --- /dev/null +++ b/ec/5ca22893220fb8ec4f9746d2e88c50cdc698f8 @@ -0,0 +1,414 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id D2640431FB6 + for ; Thu, 19 Jul 2012 11:25:16 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: 0 +X-Spam-Level: +X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] + autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id zP9Rpi5XX19Q for ; + Thu, 19 Jul 2012 11:25:15 -0700 (PDT) +Received: from mail.bustany.org (bustany.org [176.31.244.208]) + by olra.theworths.org (Postfix) with ESMTP id 022F7431FAE + for ; Thu, 19 Jul 2012 11:25:14 -0700 (PDT) +Received: from [192.168.1.147] (91-158-2-79.elisa-laajakaista.fi + [91.158.2.79]) by mail.bustany.org (Postfix) with ESMTPSA id 80077140090; + Thu, 19 Jul 2012 20:27:41 +0200 (CEST) +Message-ID: <50085106.8040804@bustany.org> +Date: Thu, 19 Jul 2012 21:25:10 +0300 +From: Adrien Bustany +User-Agent: Mozilla/5.0 (X11; Linux x86_64; + rv:13.0) Gecko/20120615 Thunderbird/13.0.1 +MIME-Version: 1.0 +To: Austin Clements +Subject: Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected +References: <1342636475-16057-1-git-send-email-adrien@bustany.org> + <1342636475-16057-4-git-send-email-adrien@bustany.org> + <20120718204001.GT31670@mit.edu> +In-Reply-To: <20120718204001.GT31670@mit.edu> +Content-Type: text/plain; charset=ISO-8859-1; format=flowed +Content-Transfer-Encoding: 8bit +Cc: notmuch@notmuchmail.org +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Thu, 19 Jul 2012 18:25:17 -0000 + +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__t see the various +>> * notmuch_ 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. */ + +