--- /dev/null
+Return-Path: <adrien@bustany.org>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id D6949431FB6\r
+ for <notmuch@notmuchmail.org>; Mon, 23 Jul 2012 15:03:45 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: 0\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none]\r
+ autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id Gl5AUztnI9xn for <notmuch@notmuchmail.org>;\r
+ Mon, 23 Jul 2012 15:03:44 -0700 (PDT)\r
+Received: from mail.bustany.org (bustany.org [176.31.244.208])\r
+ by olra.theworths.org (Postfix) with ESMTP id 389CF431FAE\r
+ for <notmuch@notmuchmail.org>; Mon, 23 Jul 2012 15:03:44 -0700 (PDT)\r
+Received: from [192.168.1.147] (91-158-2-79.elisa-laajakaista.fi\r
+ [91.158.2.79]) by mail.bustany.org (Postfix) with ESMTPSA id 547F81401C5;\r
+ Tue, 24 Jul 2012 00:06:14 +0200 (CEST)\r
+Message-ID: <500DCA3A.8060407@bustany.org>\r
+Date: Tue, 24 Jul 2012 01:03:38 +0300\r
+From: Adrien Bustany <adrien@bustany.org>\r
+User-Agent: Mozilla/5.0 (X11; Linux x86_64;\r
+ rv:13.0) Gecko/20120615 Thunderbird/13.0.1\r
+MIME-Version: 1.0\r
+To: Austin Clements <amdragon@MIT.EDU>\r
+Subject: Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected\r
+References: <1342636475-16057-1-git-send-email-adrien@bustany.org>\r
+ <1342636475-16057-4-git-send-email-adrien@bustany.org>\r
+ <20120718204001.GT31670@mit.edu> <50085106.8040804@bustany.org>\r
+ <20120720032352.GX31670@mit.edu>\r
+In-Reply-To: <20120720032352.GX31670@mit.edu>\r
+Content-Type: text/plain; charset=ISO-8859-1; format=flowed\r
+Content-Transfer-Encoding: 8bit\r
+Cc: notmuch@notmuchmail.org\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Mon, 23 Jul 2012 22:03:46 -0000\r
+\r
+Le 20/07/2012 06:23, Austin Clements a écrit :\r
+> Quoth Adrien Bustany on Jul 19 at 9:25 pm:\r
+>> Le 18/07/2012 23:40, Austin Clements a écrit :\r
+>>> This is subtle enough that I think it deserves a comment in the source\r
+>>> code explaining that tracking the talloc owner reference, combined\r
+>>> with the fact that Go finalizers are run in dependency order, ensures\r
+>>> that the C objects will always be destroyed from the talloc leaves up.\r
+>>\r
+>> Definitely, I tend to comment in the commit message and forget about\r
+>> the code...\r
+>>\r
+>>>\r
+>>> Just one inline comment below. Otherwise, I think this is all\r
+>>> correct.\r
+>>\r
+>> Agree with the comment, the Database should be the parent. I guess I\r
+>> wasn't sure of the talloc parenting.\r
+>>\r
+>>>\r
+>>> Is reproducing the talloc hierarchy in all of the bindings really the\r
+>>> right approach? I feel like there has to be a better way (or that the\r
+>>> way we use talloc in the library is slightly broken). What if the\r
+>>> bindings created an additional talloc reference to each managed\r
+>>> object, just to keep the object alive, and used talloc_unlink instead\r
+>>> of the destroy functions?\r
+>>\r
+>> Reproducing the hierarchy is probably error prone, and not that\r
+>> simple indeed :/\r
+>> I haven't checked at all the way you suggest, but if we use\r
+>> talloc_reference/unlink, we get the same issue no?\r
+>> - If we do for each new wrapped object talloc_reference(NULL,\r
+>> wrapped_object), the the object will be kept alive until we\r
+>> talloc_unlink(NULL, wrapped_object), but what about its parents? For\r
+>> example will doing that on a notmuch_message_t keep the\r
+>> notmuch_messages_t alive?\r
+>\r
+> Hmm. This is what I was thinking. You have an interesting point; I\r
+> think it's slightly wrong, but it exposes something deeper. I believe\r
+> there are two different things going on here: some of the talloc\r
+> relationships are for convenience, while some are structural. In the\r
+> former case, I'm pretty sure my suggestion will work, but in the\r
+> latter case the objects should *never* be freed by the finalizer!\r
+>\r
+> For example, notmuch_query_search_messages returns a new\r
+> notmuch_messages_t with the query as the talloc parent, but that\r
+> notmuch_messages_t doesn't depend on the query object; this is just so\r
+> you can conveniently delete everything retrieved from the query by\r
+> deleting the query. In this case, you can either use parent\r
+> references like you did---which will prevent a double-free by forcing\r
+> destruction to happen from the leaves up but at the cost of having to\r
+> encode these relationships and of extending the parent object\r
+> lifetimes beyond what's strictly necessary---or you can use my\r
+> suggestion of creating an additional talloc reference.\r
+\r
+Actually, checking the code of notmuch_query_search_messages, it seems \r
+that the notmuch_messages_t (and the notmuch_message_t as well) object \r
+*does* depend on the database and the query... So in that case I think \r
+we need the "owner" Object reference as I currently have (we want the \r
+Messages to keep the Query alive, and the Query keeps the Database alive).\r
+That said, you example below looks valid, and it seems I'll need to add \r
+a flag to createMessage() (and some others) to disable the SetFinalizer \r
+call for certain instances (we probably want to keep it for eg. \r
+SearchMessageByFilename).\r
+\r
+- The candidates I found for adding a tmalloc reference and not a "full" \r
+Go reference (therefore preventing to keep the parent alive too long \r
+needlessly) are GetAllTags, Thread.GetTags, Messages.CollectTags, and \r
+Message.GetTags (those are basically string lists)\r
+\r
+- The methods for which I should remove the SetFinalizer on the wrapper \r
+(as you showed in the example below) while keeping the Go reference are \r
+Threads.Get and Messages.Get\r
+\r
+I would also maybe remove all the Destroy() functions, since they now \r
+seem more dangerous than anything else...\r
+\r
+I tried to write a test using runtime.GC to test the behaviour of the \r
+bindings, but for some reasons some cases which are supposed to crash \r
+don't, which makes me sceptical about the validity of the test :-/\r
+\r
+Cheers\r
+\r
+Adrien\r
+\r
+>\r
+> However, in your example, the notmuch_message_t's are structurally\r
+> related to the notmuch_messages_t from whence they came. They're all\r
+> part of one data structure and hence it *never* makes sense for a\r
+> caller to delete the notmuch_message_t's. For example, even with the\r
+> code in this patch, I think the following could lead to a crash:\r
+>\r
+> 1. Obtain a Messages object, say ms.\r
+> 2. m1 := ms.Get()\r
+> 3. m1 = nil\r
+> 4. m2 := ms.Get()\r
+> 5. m2.whatever()\r
+>\r
+> If a garbage collection happens between steps 3 and 4, the Message in\r
+> m1 will get finalized and destroyed. But step 4 will return the same,\r
+> now dangling, pointer, leading to a potential crash in step 5.\r
+>\r
+> Maybe the answer in the structural case is to include the parent\r
+> pointer in the Go struct and not set a finalizer on the child? That\r
+> way, if there's a Go reference to the parent wrapper, it won't go away\r
+> and the children won't get destroyed (collecting wrappers of children\r
+> is fine) and if there's a Go reference to the child wrapper, it will\r
+> keep the parent alive so it won't get destroyed and neither will the\r
+> child.\r
+>\r
+>> - If we do talloc_reference(parent, wrapped), then we reproduce the\r
+>> hierarchy again?\r
+>>\r
+>> Note that I have 0 experience with talloc, so I might as well be\r
+>> getting things wrong here.\r
+>>\r
+>>>\r
+>>> Quoth Adrien Bustany on Jul 18 at 9:34 pm:\r
+>>>> This makes notmuch appropriately free the underlying notmuch C objects\r
+>>>> when garbage collecting their Go wrappers. To make sure we don't break\r
+>>>> the underlying links between objects (for example, a notmuch_messages_t\r
+>>>> being GC'ed before a notmuch_message_t belonging to it), we add for each\r
+>>>> wraper struct a pointer to the owner object (Go objects with a reference\r
+>>>> pointing to them don't get garbage collected).\r
+>>>> ---\r
+>>>> bindings/go/src/notmuch/notmuch.go | 153 +++++++++++++++++++++++++++++++-----\r
+>>>> 1 files changed, 134 insertions(+), 19 deletions(-)\r
+>>>>\r
+>>>> diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go\r
+>>>> index 1d77fd2..3f436a0 100644\r
+>>>> --- a/bindings/go/src/notmuch/notmuch.go\r
+>>>> +++ b/bindings/go/src/notmuch/notmuch.go\r
+>>>> @@ -11,6 +11,7 @@ package notmuch\r
+>>>> #include "notmuch.h"\r
+>>>> */\r
+>>>> import "C"\r
+>>>> +import "runtime"\r
+>>>> import "unsafe"\r
+>>>>\r
+>>>> // Status codes used for the return values of most functions\r
+>>>> @@ -47,40 +48,152 @@ func (self Status) String() string {\r
+>>>> /* Various opaque data types. For each notmuch_<foo>_t see the various\r
+>>>> * notmuch_<foo> functions below. */\r
+>>>>\r
+>>>> +type Object interface {}\r
+>>>> +\r
+>>>> type Database struct {\r
+>>>> db *C.notmuch_database_t\r
+>>>> }\r
+>>>>\r
+>>>> +func createDatabase(db *C.notmuch_database_t) *Database {\r
+>>>> + self := &Database{db: db}\r
+>>>> +\r
+>>>> + runtime.SetFinalizer(self, func(x *Database) {\r
+>>>> + if (x.db != nil) {\r
+>>>> + C.notmuch_database_destroy(x.db)\r
+>>>> + }\r
+>>>> + })\r
+>>>> +\r
+>>>> + return self\r
+>>>> +}\r
+>>>> +\r
+>>>> type Query struct {\r
+>>>> query *C.notmuch_query_t\r
+>>>> + owner Object\r
+>>>> +}\r
+>>>> +\r
+>>>> +func createQuery(query *C.notmuch_query_t, owner Object) *Query {\r
+>>>> + self := &Query{query: query, owner: owner}\r
+>>>> +\r
+>>>> + runtime.SetFinalizer(self, func(x *Query) {\r
+>>>> + if (x.query != nil) {\r
+>>>> + C.notmuch_query_destroy(x.query)\r
+>>>> + }\r
+>>>> + })\r
+>>>> +\r
+>>>> + return self\r
+>>>> }\r
+>>>>\r
+>>>> type Threads struct {\r
+>>>> threads *C.notmuch_threads_t\r
+>>>> + owner Object\r
+>>>> +}\r
+>>>> +\r
+>>>> +func createThreads(threads *C.notmuch_threads_t, owner Object) *Threads {\r
+>>>> + self := &Threads{threads: threads, owner: owner}\r
+>>>> +\r
+>>>> + runtime.SetFinalizer(self, func(x *Threads) {\r
+>>>> + if (x.threads != nil) {\r
+>>>> + C.notmuch_threads_destroy(x.threads)\r
+>>>> + }\r
+>>>> + })\r
+>>>> +\r
+>>>> + return self\r
+>>>> }\r
+>>>>\r
+>>>> type Thread struct {\r
+>>>> thread *C.notmuch_thread_t\r
+>>>> + owner Object\r
+>>>> +}\r
+>>>> +\r
+>>>> +func createThread(thread *C.notmuch_thread_t, owner Object) *Thread {\r
+>>>> + self := &Thread{thread: thread, owner: owner}\r
+>>>> +\r
+>>>> + runtime.SetFinalizer(self, func(x *Thread) {\r
+>>>> + if (x.thread != nil) {\r
+>>>> + C.notmuch_thread_destroy(x.thread)\r
+>>>> + }\r
+>>>> + })\r
+>>>> +\r
+>>>> + return self\r
+>>>> }\r
+>>>>\r
+>>>> type Messages struct {\r
+>>>> messages *C.notmuch_messages_t\r
+>>>> + owner Object\r
+>>>> +}\r
+>>>> +\r
+>>>> +func createMessages(messages *C.notmuch_messages_t, owner Object) *Messages {\r
+>>>> + self := &Messages{messages: messages, owner: owner}\r
+>>>> +\r
+>>>> + return self\r
+>>>> }\r
+>>>>\r
+>>>> type Message struct {\r
+>>>> message *C.notmuch_message_t\r
+>>>> + owner Object\r
+>>>> +}\r
+>>>> +\r
+>>>> +func createMessage(message *C.notmuch_message_t, owner Object) *Message {\r
+>>>> + self := &Message{message: message, owner: owner}\r
+>>>> +\r
+>>>> + runtime.SetFinalizer(self, func(x *Message) {\r
+>>>> + if (x.message != nil) {\r
+>>>> + C.notmuch_message_destroy(x.message)\r
+>>>> + }\r
+>>>> + })\r
+>>>> +\r
+>>>> + return self\r
+>>>> }\r
+>>>>\r
+>>>> type Tags struct {\r
+>>>> tags *C.notmuch_tags_t\r
+>>>> + owner Object\r
+>>>> +}\r
+>>>> +\r
+>>>> +func createTags(tags *C.notmuch_tags_t, owner Object) *Tags {\r
+>>>> + self := &Tags{tags: tags, owner: owner}\r
+>>>> +\r
+>>>> + runtime.SetFinalizer(self, func(x *Tags) {\r
+>>>> + if (x.tags != nil) {\r
+>>>> + C.notmuch_tags_destroy(x.tags)\r
+>>>> + }\r
+>>>> + })\r
+>>>> +\r
+>>>> + return self\r
+>>>> }\r
+>>>>\r
+>>>> type Directory struct {\r
+>>>> dir *C.notmuch_directory_t\r
+>>>> + owner Object\r
+>>>> +}\r
+>>>> +\r
+>>>> +func createDirectory(directory *C.notmuch_directory_t, owner Object) *Directory {\r
+>>>> + self := &Directory{dir: directory, owner: owner}\r
+>>>> +\r
+>>>> + runtime.SetFinalizer(self, func(x *Directory) {\r
+>>>> + if (x.dir != nil) {\r
+>>>> + C.notmuch_directory_destroy(x.dir)\r
+>>>> + }\r
+>>>> + })\r
+>>>> +\r
+>>>> + return self\r
+>>>> }\r
+>>>>\r
+>>>> type Filenames struct {\r
+>>>> fnames *C.notmuch_filenames_t\r
+>>>> + owner Object\r
+>>>> +}\r
+>>>> +\r
+>>>> +func createFilenames(filenames *C.notmuch_filenames_t, owner Object) *Filenames {\r
+>>>> + self := &Filenames{fnames: filenames, owner: owner}\r
+>>>> +\r
+>>>> + runtime.SetFinalizer(self, func(x *Filenames) {\r
+>>>> + if (x.fnames != nil) {\r
+>>>> + C.notmuch_filenames_destroy(x.fnames)\r
+>>>> + }\r
+>>>> + })\r
+>>>> +\r
+>>>> + return self\r
+>>>> }\r
+>>>>\r
+>>>> type DatabaseMode C.notmuch_database_mode_t\r
+>>>> @@ -100,12 +213,13 @@ func NewDatabase(path string) (*Database, Status) {\r
+>>>> return nil, STATUS_OUT_OF_MEMORY\r
+>>>> }\r
+>>>>\r
+>>>> - self := &Database{db: nil}\r
+>>>> - st := Status(C.notmuch_database_create(c_path, &self.db))\r
+>>>> + var db *C.notmuch_database_t;\r
+>>>> + st := Status(C.notmuch_database_create(c_path, &db))\r
+>>>> if st != STATUS_SUCCESS {\r
+>>>> return nil, st\r
+>>>> }\r
+>>>> - return self, st\r
+>>>> +\r
+>>>> + return createDatabase(db), st\r
+>>>> }\r
+>>>>\r
+>>>> /* Open an existing notmuch database located at 'path'.\r
+>>>> @@ -134,12 +248,13 @@ func OpenDatabase(path string, mode DatabaseMode) (*Database, Status) {\r
+>>>> return nil, STATUS_OUT_OF_MEMORY\r
+>>>> }\r
+>>>>\r
+>>>> - self := &Database{db: nil}\r
+>>>> - st := Status(C.notmuch_database_open(c_path, C.notmuch_database_mode_t(mode), &self.db))\r
+>>>> + var db *C.notmuch_database_t;\r
+>>>> + st := Status(C.notmuch_database_open(c_path, C.notmuch_database_mode_t(mode), &db))\r
+>>>> if st != STATUS_SUCCESS {\r
+>>>> return nil, st\r
+>>>> }\r
+>>>> - return self, st\r
+>>>> +\r
+>>>> + return createDatabase(db), st\r
+>>>> }\r
+>>>>\r
+>>>> /* Close the given notmuch database, freeing all associated\r
+>>>> @@ -204,7 +319,7 @@ func (self *Database) GetDirectory(path string) (*Directory, Status) {\r
+>>>> if st != STATUS_SUCCESS || c_dir == nil {\r
+>>>> return nil, st\r
+>>>> }\r
+>>>> - return &Directory{dir: c_dir}, st\r
+>>>> + return createDirectory(c_dir, nil), st\r
+>>>\r
+>>> It looks like you have a nil owner for anything whose talloc parent is\r
+>>> the database. Is this intentional? Shouldn't the owner be self in\r
+>>> these cases, too?\r
+>>>\r
+>>>> }\r
+>>>>\r
+>>>> /* Add a new message to the given notmuch database.\r
+>>>> @@ -258,7 +373,7 @@ func (self *Database) AddMessage(fname string) (*Message, Status) {\r
+>>>> var c_msg *C.notmuch_message_t = new(C.notmuch_message_t)\r
+>>>> st := Status(C.notmuch_database_add_message(self.db, c_fname, &c_msg))\r
+>>>>\r
+>>>> - return &Message{message: c_msg}, st\r
+>>>> + return createMessage(c_msg, nil), st\r
+>>>> }\r
+>>>>\r
+>>>> /* Remove a message from the given notmuch database.\r
+>>>> @@ -319,12 +434,12 @@ func (self *Database) FindMessage(message_id string) (*Message, Status) {\r
+>>>> return nil, STATUS_OUT_OF_MEMORY\r
+>>>> }\r
+>>>>\r
+>>>> - msg := &Message{message: nil}\r
+>>>> - st := Status(C.notmuch_database_find_message(self.db, c_msg_id, &msg.message))\r
+>>>> + var msg *C.notmuch_message_t\r
+>>>> + st := Status(C.notmuch_database_find_message(self.db, c_msg_id, &msg))\r
+>>>> if st != STATUS_SUCCESS {\r
+>>>> return nil, st\r
+>>>> }\r
+>>>> - return msg, st\r
+>>>> + return createMessage(msg, nil), st\r
+>>>> }\r
+>>>>\r
+>>>> /* Return a list of all tags found in the database.\r
+>>>> @@ -339,7 +454,7 @@ func (self *Database) GetAllTags() *Tags {\r
+>>>> if tags == nil {\r
+>>>> return nil\r
+>>>> }\r
+>>>> - return &Tags{tags: tags}\r
+>>>> + return createTags(tags, nil)\r
+>>>> }\r
+>>>>\r
+>>>> /* Create a new query for 'database'.\r
+>>>> @@ -379,7 +494,7 @@ func (self *Database) CreateQuery(query string) *Query {\r
+>>>> if q == nil {\r
+>>>> return nil\r
+>>>> }\r
+>>>> - return &Query{query: q}\r
+>>>> + return createQuery(q, nil)\r
+>>>> }\r
+>>>>\r
+>>>> /* Sort values for notmuch_query_set_sort */\r
+>>>> @@ -459,7 +574,7 @@ func (self *Query) SearchThreads() *Threads {\r
+>>>> if threads == nil {\r
+>>>> return nil\r
+>>>> }\r
+>>>> - return &Threads{threads: threads}\r
+>>>> + return createThreads(threads, self)\r
+>>>> }\r
+>>>>\r
+>>>> /* Execute a query for messages, returning a notmuch_messages_t object\r
+>>>> @@ -505,7 +620,7 @@ func (self *Query) SearchMessages() *Messages {\r
+>>>> if msgs == nil {\r
+>>>> return nil\r
+>>>> }\r
+>>>> - return &Messages{messages: msgs}\r
+>>>> + return createMessages(msgs, self)\r
+>>>> }\r
+>>>>\r
+>>>> /* Destroy a notmuch_query_t along with any associated resources.\r
+>>>> @@ -607,7 +722,7 @@ func (self *Messages) Get() *Message {\r
+>>>> if msg == nil {\r
+>>>> return nil\r
+>>>> }\r
+>>>> - return &Message{message: msg}\r
+>>>> + return createMessage(msg, self)\r
+>>>> }\r
+>>>>\r
+>>>> /* Move the 'messages' iterator to the next message.\r
+>>>> @@ -659,7 +774,7 @@ func (self *Messages) CollectTags() *Tags {\r
+>>>> if tags == nil {\r
+>>>> return nil\r
+>>>> }\r
+>>>> - return &Tags{tags: tags}\r
+>>>> + return createTags(tags, self)\r
+>>>> }\r
+>>>>\r
+>>>> /* Get the message ID of 'message'.\r
+>>>> @@ -739,7 +854,7 @@ func (self *Message) GetReplies() *Messages {\r
+>>>> if msgs == nil {\r
+>>>> return nil\r
+>>>> }\r
+>>>> - return &Messages{messages: msgs}\r
+>>>> + return createMessages(msgs, self)\r
+>>>> }\r
+>>>>\r
+>>>> /* Get a filename for the email corresponding to 'message'.\r
+>>>> @@ -871,7 +986,7 @@ func (self *Message) GetTags() *Tags {\r
+>>>> if tags == nil {\r
+>>>> return nil\r
+>>>> }\r
+>>>> - return &Tags{tags: tags}\r
+>>>> + return createTags(tags, self)\r
+>>>> }\r
+>>>>\r
+>>>> /* The longest possible tag value. */\r
+\r
+\r
projects / notmuch-archives.git / commitdiff