From: Austin Clements Date: Wed, 18 Jul 2012 20:40:01 +0000 (+2000) 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=8ceb3bb7c6b09e29fe75667caf3399d755a11b68;p=notmuch-archives.git Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected --- diff --git a/4d/d192b7ac41c985783395b707590ef0536b1d16 b/4d/d192b7ac41c985783395b707590ef0536b1d16 new file mode 100644 index 000000000..fab9110ce --- /dev/null +++ b/4d/d192b7ac41c985783395b707590ef0536b1d16 @@ -0,0 +1,413 @@ +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 D128C431FAF + for ; Wed, 18 Jul 2012 13:40:05 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 QnCXkpR3SVQ9 for ; + Wed, 18 Jul 2012 13:40:05 -0700 (PDT) +Received: from dmz-mailsec-scanner-4.mit.edu (DMZ-MAILSEC-SCANNER-4.MIT.EDU + [18.9.25.15]) + by olra.theworths.org (Postfix) with ESMTP id C3DEE431FAE + for ; Wed, 18 Jul 2012 13:40:04 -0700 (PDT) +X-AuditID: 1209190f-b7f306d0000008b4-27-50071f2441b5 +Received: from mailhub-auth-4.mit.edu ( [18.7.62.39]) + by dmz-mailsec-scanner-4.mit.edu (Symantec Messaging Gateway) with SMTP + id 42.8A.02228.42F17005; Wed, 18 Jul 2012 16:40:04 -0400 (EDT) +Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) + by mailhub-auth-4.mit.edu (8.13.8/8.9.2) with ESMTP id q6IKe3VR000780; + Wed, 18 Jul 2012 16:40:03 -0400 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id q6IKe1Gm002688 + (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); + Wed, 18 Jul 2012 16:40:02 -0400 (EDT) +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) + (envelope-from ) + id 1Srb2P-0001Cd-AG; Wed, 18 Jul 2012 16:40:01 -0400 +Date: Wed, 18 Jul 2012 16:40:01 -0400 +From: Austin Clements +To: Adrien Bustany +Subject: Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected +Message-ID: <20120718204001.GT31670@mit.edu> +References: <1342636475-16057-1-git-send-email-adrien@bustany.org> + <1342636475-16057-4-git-send-email-adrien@bustany.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +Content-Disposition: inline +In-Reply-To: <1342636475-16057-4-git-send-email-adrien@bustany.org> +User-Agent: Mutt/1.5.21 (2010-09-15) +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFmpkleLIzCtJLcpLzFFi42IRYrdT11WRZw8w+LpH2GL9nbVsFtdvzmR2 + YPL4eOAek8ezVbeYA5iiuGxSUnMyy1KL9O0SuDLOv7EqWOJbcXVaN1sDY7tNFyMHh4SAicTy + xZZdjJxAppjEhXvr2UBsIYF9jBLPWmW6GLmA7A2MEndWzGOHcE4ySUy/uokRwlnCKNG4cDkr + SAuLgKrExtNzWEBsNgENiW37lzOC2CIC6hI7OtvBbGYBaYlvv5uZQGxhAS+J25t7mEFsXgEd + iXOTl7BCrK6W2HZqBRtEXFDi5MwnLBC9WhI3/r1kArkaZM7yfxwgYU4BZ4kpUxaBtYoKqEhM + ObmNbQKj0Cwk3bOQdM9C6F7AyLyKUTYlt0o3NzEzpzg1Wbc4OTEvL7VI10QvN7NELzWldBMj + KKQ5Jfl3MH47qHSIUYCDUYmH98Eu1gAh1sSy4srcQ4ySHExKoryfhNgDhPiS8lMqMxKLM+KL + SnNSiw8xSnAwK4nwPhAEyvGmJFZWpRblw6SkOViUxHmvptz0FxJITyxJzU5NLUgtgsnKcHAo + SfA6ywE1ChalpqdWpGXmlCCkmTg4QYbzAA23BKnhLS5IzC3OTIfIn2JUlBLnjQJJCIAkMkrz + 4HphKecVozjQK8K8NiBVPMB0Bdf9CmgwE9Bg7mI2kMEliQgpqQZGq6d/jZKvCazZqsqV//r4 + m71TN+206DNQ2xPhfrCP8Qgnf2/Mu3ULrBpsbAL81scsEZ38XPCiUpS5o3jZvw378/PPsIq0 + TrLrP+T7k7P6xrFm9sf9Zl8TTIv2TeBaeKVmHVv6g+6ab9UP55/8/mDyqqmCIZHeN5leMDNy + Xymfu+qp/WoN6U/7lFiKMxINtZiLihMB2JbsphQDAAA= +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: Wed, 18 Jul 2012 20:40:06 -0000 + +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__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. */