From 53f939284042117f013909dd451be940ea14b694 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Fri, 20 Jul 2012 23:23:52 +2000 Subject: [PATCH] Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected --- cc/e135535f0cbbe4bf66bc0bd3e501114f070787 | 484 ++++++++++++++++++++++ 1 file changed, 484 insertions(+) create mode 100644 cc/e135535f0cbbe4bf66bc0bd3e501114f070787 diff --git a/cc/e135535f0cbbe4bf66bc0bd3e501114f070787 b/cc/e135535f0cbbe4bf66bc0bd3e501114f070787 new file mode 100644 index 000000000..578d2e112 --- /dev/null +++ b/cc/e135535f0cbbe4bf66bc0bd3e501114f070787 @@ -0,0 +1,484 @@ +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 30B1C431FB6 + for ; Thu, 19 Jul 2012 20:24:02 -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 361fwEsHskcC for ; + Thu, 19 Jul 2012 20:24:01 -0700 (PDT) +Received: from dmz-mailsec-scanner-6.mit.edu (DMZ-MAILSEC-SCANNER-6.MIT.EDU + [18.7.68.35]) + by olra.theworths.org (Postfix) with ESMTP id C5B70431FAE + for ; Thu, 19 Jul 2012 20:24:00 -0700 (PDT) +X-AuditID: 12074423-b7f396d0000008f4-10-5008cf4ff7a1 +Received: from mailhub-auth-3.mit.edu ( [18.9.21.43]) + by dmz-mailsec-scanner-6.mit.edu (Symantec Messaging Gateway) with SMTP + id 1F.EE.02292.F4FC8005; Thu, 19 Jul 2012 23:23:59 -0400 (EDT) +Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) + by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id q6K3Nwe1027864; + Thu, 19 Jul 2012 23:23:58 -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 q6K3Nrli023977 + (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NOT); + Thu, 19 Jul 2012 23:23:54 -0400 (EDT) +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.77) + (envelope-from ) + id 1Ss3om-0005qn-Kc; Thu, 19 Jul 2012 23:23:52 -0400 +Date: Thu, 19 Jul 2012 23:23:52 -0400 +From: Austin Clements +To: Adrien Bustany +Subject: Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected +Message-ID: <20120720032352.GX31670@mit.edu> +References: <1342636475-16057-1-git-send-email-adrien@bustany.org> + <1342636475-16057-4-git-send-email-adrien@bustany.org> + <20120718204001.GT31670@mit.edu> <50085106.8040804@bustany.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=iso-8859-1 +Content-Disposition: inline +Content-Transfer-Encoding: 8bit +In-Reply-To: <50085106.8040804@bustany.org> +User-Agent: Mutt/1.5.21 (2010-09-15) +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFprNKsWRmVeSWpSXmKPExsUixCmqret/niPA4E2LuMX6O2vZLK7fnMns + wOTx8cA9Jo9nq24xBzBFcdmkpOZklqUW6dslcGUsadrEVNCSU9HXvJG5gXFiaBcjJ4eEgInE + 2u+/WCBsMYkL99azdTFycQgJ7GOUePpmLyOEs4FRYsWDHawQzkkmiYXPN0I5Sxgl/v+8zArS + zyKgKvHy+luwWWwCGhLb9i9nBLFFBNQldnS2g9nMAtIS3343M4HYwgJeErc39zB3MXJw8Aro + SExosAQJCwnsZ5T4cCIIxOYVEJQ4OfMJC0SrjsTOrXfYQMpBxiz/xwERlpdo3jqbGcTmFNCW + 2PFrL1i5qICKxJST29gmMArPQjJpFpJJsxAmzUIyaQEjyypG2ZTcKt3cxMyc4tRk3eLkxLy8 + 1CJdM73czBK91JTSTYygOGB3Ud7B+Oeg0iFGAQ5GJR7eSUkcAUKsiWXFlbmHGCU5mJREeZtP + A4X4kvJTKjMSizPii0pzUosPMUpwMCuJ8H4/BZTjTUmsrEotyodJSXOwKInzXku56S8kkJ5Y + kpqdmlqQWgSTleHgUJLgvXcOqFGwKDU9tSItM6cEIc3EwQkynAdoeDZIDW9xQWJucWY6RP4U + o6KUOK8NSEIAJJFRmgfXC0tTrxjFgV4R5l0FUsUDTHFw3a+ABjMBDeYuZgMZXJKIkJJqYGxm + 4IzjW3+kt/V639ndtz4yKPC8YnvNquG8zF9L08s9r8pxGZN60NUX4dOyHgXKF6nFal5Uspp6 + 9650aFDXu9lfKsuVTnXdiJnSZOCZu433fFTun5mWHcevGi+Xk3RR2Xrk6Y/sKd8uP5yyexOL + vo2eX7FTo8S5d2orU+7KWjH4+C6o/1B5VImlOCPRUIu5qDgRALJAgyYuAwAA +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: Fri, 20 Jul 2012 03:24:02 -0000 + +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__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. */ -- 2.26.2