ec/5ca22893220fb8ec4f9746d2e88c50cdc698f8

   1 Return-Path: <adrien@bustany.org>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id D2640431FB6\r
   6         for <notmuch@notmuchmail.org>; Thu, 19 Jul 2012 11:25:16 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: 0\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none]\r
  12         autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id zP9Rpi5XX19Q for <notmuch@notmuchmail.org>;\r
  16         Thu, 19 Jul 2012 11:25:15 -0700 (PDT)\r
  17 Received: from mail.bustany.org (bustany.org [176.31.244.208])\r
  18         by olra.theworths.org (Postfix) with ESMTP id 022F7431FAE\r
  19         for <notmuch@notmuchmail.org>; Thu, 19 Jul 2012 11:25:14 -0700 (PDT)\r
  20 Received: from [192.168.1.147] (91-158-2-79.elisa-laajakaista.fi\r
  21  [91.158.2.79]) by mail.bustany.org (Postfix) with ESMTPSA id 80077140090;\r
  22         Thu, 19 Jul 2012 20:27:41 +0200 (CEST)\r
  23 Message-ID: <50085106.8040804@bustany.org>\r
  24 Date: Thu, 19 Jul 2012 21:25:10 +0300\r
  25 From: Adrien Bustany <adrien@bustany.org>\r
  26 User-Agent: Mozilla/5.0 (X11; Linux x86_64;\r
  27         rv:13.0) Gecko/20120615 Thunderbird/13.0.1\r
  28 MIME-Version: 1.0\r
  29 To: Austin Clements <amdragon@MIT.EDU>\r
  30 Subject: Re: [PATCH 3/7] go: Allow notmuch objects to be garbage collected\r
  31 References: <1342636475-16057-1-git-send-email-adrien@bustany.org>\r
  32         <1342636475-16057-4-git-send-email-adrien@bustany.org>\r
  33         <20120718204001.GT31670@mit.edu>\r
  34 In-Reply-To: <20120718204001.GT31670@mit.edu>\r
  35 Content-Type: text/plain; charset=ISO-8859-1; format=flowed\r
  36 Content-Transfer-Encoding: 8bit\r
  37 Cc: notmuch@notmuchmail.org\r
  38 X-BeenThere: notmuch@notmuchmail.org\r
  39 X-Mailman-Version: 2.1.13\r
  40 Precedence: list\r
  41 List-Id: "Use and development of the notmuch mail system."\r
  42         <notmuch.notmuchmail.org>\r
  43 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  44         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  45 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  46 List-Post: <mailto:notmuch@notmuchmail.org>\r
  47 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  48 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  49         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  50 X-List-Received-Date: Thu, 19 Jul 2012 18:25:17 -0000\r
  51 \r
  52 Le 18/07/2012 23:40, Austin Clements a écrit :\r
  53 > This is subtle enough that I think it deserves a comment in the source\r
  54 > code explaining that tracking the talloc owner reference, combined\r
  55 > with the fact that Go finalizers are run in dependency order, ensures\r
  56 > that the C objects will always be destroyed from the talloc leaves up.\r
  57 \r
  58 Definitely, I tend to comment in the commit message and forget about the \r
  59 code...\r
  60 \r
  61 >\r
  62 > Just one inline comment below.  Otherwise, I think this is all\r
  63 > correct.\r
  64 \r
  65 Agree with the comment, the Database should be the parent. I guess I \r
  66 wasn't sure of the talloc parenting.\r
  67 \r
  68 >\r
  69 > Is reproducing the talloc hierarchy in all of the bindings really the\r
  70 > right approach?  I feel like there has to be a better way (or that the\r
  71 > way we use talloc in the library is slightly broken).  What if the\r
  72 > bindings created an additional talloc reference to each managed\r
  73 > object, just to keep the object alive, and used talloc_unlink instead\r
  74 > of the destroy functions?\r
  75 \r
  76 Reproducing the hierarchy is probably error prone, and not that simple \r
  77 indeed :/\r
  78 I haven't checked at all the way you suggest, but if we use \r
  79 talloc_reference/unlink, we get the same issue no?\r
  80 - If we do for each new wrapped object talloc_reference(NULL, \r
  81 wrapped_object), the the object will be kept alive until we \r
  82 talloc_unlink(NULL, wrapped_object), but what about its parents? For \r
  83 example will doing that on a notmuch_message_t keep the \r
  84 notmuch_messages_t alive?\r
  85 - If we do talloc_reference(parent, wrapped), then we reproduce the \r
  86 hierarchy again?\r
  87 \r
  88 Note that I have 0 experience with talloc, so I might as well be getting \r
  89 things wrong here.\r
  90 \r
  91 >\r
  92 > Quoth Adrien Bustany on Jul 18 at  9:34 pm:\r
  93 >> This makes notmuch appropriately free the underlying notmuch C objects\r
  94 >> when garbage collecting their Go wrappers. To make sure we don't break\r
  95 >> the underlying links between objects (for example, a notmuch_messages_t\r
  96 >> being GC'ed before a notmuch_message_t belonging to it), we add for each\r
  97 >> wraper struct a pointer to the owner object (Go objects with a reference\r
  98 >> pointing to them don't get garbage collected).\r
  99 >> ---\r
 100 >>   bindings/go/src/notmuch/notmuch.go |  153 +++++++++++++++++++++++++++++++-----\r
 101 >>   1 files changed, 134 insertions(+), 19 deletions(-)\r
 102 >>\r
 103 >> diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go\r
 104 >> index 1d77fd2..3f436a0 100644\r
 105 >> --- a/bindings/go/src/notmuch/notmuch.go\r
 106 >> +++ b/bindings/go/src/notmuch/notmuch.go\r
 107 >> @@ -11,6 +11,7 @@ package notmuch\r
 108 >>   #include "notmuch.h"\r
 109 >>   */\r
 110 >>   import "C"\r
 111 >> +import "runtime"\r
 112 >>   import "unsafe"\r
 113 >>\r
 114 >>   // Status codes used for the return values of most functions\r
 115 >> @@ -47,40 +48,152 @@ func (self Status) String() string {\r
 116 >>   /* Various opaque data types. For each notmuch_<foo>_t see the various\r
 117 >>    * notmuch_<foo> functions below. */\r
 118 >>\r
 119 >> +type Object interface {}\r
 120 >> +\r
 121 >>   type Database struct {\r
 122 >>      db *C.notmuch_database_t\r
 123 >>   }\r
 124 >>\r
 125 >> +func createDatabase(db *C.notmuch_database_t) *Database {\r
 126 >> +    self := &Database{db: db}\r
 127 >> +\r
 128 >> +    runtime.SetFinalizer(self, func(x *Database) {\r
 129 >> +            if (x.db != nil) {\r
 130 >> +                    C.notmuch_database_destroy(x.db)\r
 131 >> +            }\r
 132 >> +    })\r
 133 >> +\r
 134 >> +    return self\r
 135 >> +}\r
 136 >> +\r
 137 >>   type Query struct {\r
 138 >>      query *C.notmuch_query_t\r
 139 >> +    owner Object\r
 140 >> +}\r
 141 >> +\r
 142 >> +func createQuery(query *C.notmuch_query_t, owner Object) *Query {\r
 143 >> +    self := &Query{query: query, owner: owner}\r
 144 >> +\r
 145 >> +    runtime.SetFinalizer(self, func(x *Query) {\r
 146 >> +            if (x.query != nil) {\r
 147 >> +                    C.notmuch_query_destroy(x.query)\r
 148 >> +            }\r
 149 >> +    })\r
 150 >> +\r
 151 >> +    return self\r
 152 >>   }\r
 153 >>\r
 154 >>   type Threads struct {\r
 155 >>      threads *C.notmuch_threads_t\r
 156 >> +    owner Object\r
 157 >> +}\r
 158 >> +\r
 159 >> +func createThreads(threads *C.notmuch_threads_t, owner Object) *Threads {\r
 160 >> +    self := &Threads{threads: threads, owner: owner}\r
 161 >> +\r
 162 >> +    runtime.SetFinalizer(self, func(x *Threads) {\r
 163 >> +            if (x.threads != nil) {\r
 164 >> +                    C.notmuch_threads_destroy(x.threads)\r
 165 >> +            }\r
 166 >> +    })\r
 167 >> +\r
 168 >> +    return self\r
 169 >>   }\r
 170 >>\r
 171 >>   type Thread struct {\r
 172 >>      thread *C.notmuch_thread_t\r
 173 >> +    owner Object\r
 174 >> +}\r
 175 >> +\r
 176 >> +func createThread(thread *C.notmuch_thread_t, owner Object) *Thread {\r
 177 >> +    self := &Thread{thread: thread, owner: owner}\r
 178 >> +\r
 179 >> +    runtime.SetFinalizer(self, func(x *Thread) {\r
 180 >> +            if (x.thread != nil) {\r
 181 >> +                    C.notmuch_thread_destroy(x.thread)\r
 182 >> +            }\r
 183 >> +    })\r
 184 >> +\r
 185 >> +    return self\r
 186 >>   }\r
 187 >>\r
 188 >>   type Messages struct {\r
 189 >>      messages *C.notmuch_messages_t\r
 190 >> +    owner Object\r
 191 >> +}\r
 192 >> +\r
 193 >> +func createMessages(messages *C.notmuch_messages_t, owner Object) *Messages {\r
 194 >> +    self := &Messages{messages: messages, owner: owner}\r
 195 >> +\r
 196 >> +    return self\r
 197 >>   }\r
 198 >>\r
 199 >>   type Message struct {\r
 200 >>      message *C.notmuch_message_t\r
 201 >> +    owner Object\r
 202 >> +}\r
 203 >> +\r
 204 >> +func createMessage(message *C.notmuch_message_t, owner Object) *Message {\r
 205 >> +    self := &Message{message: message, owner: owner}\r
 206 >> +\r
 207 >> +    runtime.SetFinalizer(self, func(x *Message) {\r
 208 >> +            if (x.message != nil) {\r
 209 >> +                    C.notmuch_message_destroy(x.message)\r
 210 >> +            }\r
 211 >> +    })\r
 212 >> +\r
 213 >> +    return self\r
 214 >>   }\r
 215 >>\r
 216 >>   type Tags struct {\r
 217 >>      tags *C.notmuch_tags_t\r
 218 >> +    owner Object\r
 219 >> +}\r
 220 >> +\r
 221 >> +func createTags(tags *C.notmuch_tags_t, owner Object) *Tags {\r
 222 >> +    self := &Tags{tags: tags, owner: owner}\r
 223 >> +\r
 224 >> +    runtime.SetFinalizer(self, func(x *Tags) {\r
 225 >> +            if (x.tags != nil) {\r
 226 >> +                    C.notmuch_tags_destroy(x.tags)\r
 227 >> +            }\r
 228 >> +    })\r
 229 >> +\r
 230 >> +    return self\r
 231 >>   }\r
 232 >>\r
 233 >>   type Directory struct {\r
 234 >>      dir *C.notmuch_directory_t\r
 235 >> +    owner Object\r
 236 >> +}\r
 237 >> +\r
 238 >> +func createDirectory(directory *C.notmuch_directory_t, owner Object) *Directory {\r
 239 >> +    self := &Directory{dir: directory, owner: owner}\r
 240 >> +\r
 241 >> +    runtime.SetFinalizer(self, func(x *Directory) {\r
 242 >> +            if (x.dir != nil) {\r
 243 >> +                    C.notmuch_directory_destroy(x.dir)\r
 244 >> +            }\r
 245 >> +    })\r
 246 >> +\r
 247 >> +    return self\r
 248 >>   }\r
 249 >>\r
 250 >>   type Filenames struct {\r
 251 >>      fnames *C.notmuch_filenames_t\r
 252 >> +    owner Object\r
 253 >> +}\r
 254 >> +\r
 255 >> +func createFilenames(filenames *C.notmuch_filenames_t, owner Object) *Filenames {\r
 256 >> +    self := &Filenames{fnames: filenames, owner: owner}\r
 257 >> +\r
 258 >> +    runtime.SetFinalizer(self, func(x *Filenames) {\r
 259 >> +            if (x.fnames != nil) {\r
 260 >> +                    C.notmuch_filenames_destroy(x.fnames)\r
 261 >> +            }\r
 262 >> +    })\r
 263 >> +\r
 264 >> +    return self\r
 265 >>   }\r
 266 >>\r
 267 >>   type DatabaseMode C.notmuch_database_mode_t\r
 268 >> @@ -100,12 +213,13 @@ func NewDatabase(path string) (*Database, Status) {\r
 269 >>              return nil, STATUS_OUT_OF_MEMORY\r
 270 >>      }\r
 271 >>\r
 272 >> -    self := &Database{db: nil}\r
 273 >> -    st := Status(C.notmuch_database_create(c_path, &self.db))\r
 274 >> +    var db *C.notmuch_database_t;\r
 275 >> +    st := Status(C.notmuch_database_create(c_path, &db))\r
 276 >>      if st != STATUS_SUCCESS {\r
 277 >>              return nil, st\r
 278 >>      }\r
 279 >> -    return self, st\r
 280 >> +\r
 281 >> +    return createDatabase(db), st\r
 282 >>   }\r
 283 >>\r
 284 >>   /* Open an existing notmuch database located at 'path'.\r
 285 >> @@ -134,12 +248,13 @@ func OpenDatabase(path string, mode DatabaseMode) (*Database, Status) {\r
 286 >>              return nil, STATUS_OUT_OF_MEMORY\r
 287 >>      }\r
 288 >>\r
 289 >> -    self := &Database{db: nil}\r
 290 >> -    st := Status(C.notmuch_database_open(c_path, C.notmuch_database_mode_t(mode), &self.db))\r
 291 >> +    var db *C.notmuch_database_t;\r
 292 >> +    st := Status(C.notmuch_database_open(c_path, C.notmuch_database_mode_t(mode), &db))\r
 293 >>      if st != STATUS_SUCCESS {\r
 294 >>              return nil, st\r
 295 >>      }\r
 296 >> -    return self, st\r
 297 >> +\r
 298 >> +    return createDatabase(db), st\r
 299 >>   }\r
 300 >>\r
 301 >>   /* Close the given notmuch database, freeing all associated\r
 302 >> @@ -204,7 +319,7 @@ func (self *Database) GetDirectory(path string) (*Directory, Status) {\r
 303 >>      if st != STATUS_SUCCESS || c_dir == nil {\r
 304 >>              return nil, st\r
 305 >>      }\r
 306 >> -    return &Directory{dir: c_dir}, st\r
 307 >> +    return createDirectory(c_dir, nil), st\r
 308 >\r
 309 > It looks like you have a nil owner for anything whose talloc parent is\r
 310 > the database.  Is this intentional?  Shouldn't the owner be self in\r
 311 > these cases, too?\r
 312 >\r
 313 >>   }\r
 314 >>\r
 315 >>   /* Add a new message to the given notmuch database.\r
 316 >> @@ -258,7 +373,7 @@ func (self *Database) AddMessage(fname string) (*Message, Status) {\r
 317 >>      var c_msg *C.notmuch_message_t = new(C.notmuch_message_t)\r
 318 >>      st := Status(C.notmuch_database_add_message(self.db, c_fname, &c_msg))\r
 319 >>\r
 320 >> -    return &Message{message: c_msg}, st\r
 321 >> +    return createMessage(c_msg, nil), st\r
 322 >>   }\r
 323 >>\r
 324 >>   /* Remove a message from the given notmuch database.\r
 325 >> @@ -319,12 +434,12 @@ func (self *Database) FindMessage(message_id string) (*Message, Status) {\r
 326 >>              return nil, STATUS_OUT_OF_MEMORY\r
 327 >>      }\r
 328 >>\r
 329 >> -    msg := &Message{message: nil}\r
 330 >> -    st := Status(C.notmuch_database_find_message(self.db, c_msg_id, &msg.message))\r
 331 >> +    var msg *C.notmuch_message_t\r
 332 >> +    st := Status(C.notmuch_database_find_message(self.db, c_msg_id, &msg))\r
 333 >>      if st != STATUS_SUCCESS {\r
 334 >>              return nil, st\r
 335 >>      }\r
 336 >> -    return msg, st\r
 337 >> +    return createMessage(msg, nil), st\r
 338 >>   }\r
 339 >>\r
 340 >>   /* Return a list of all tags found in the database.\r
 341 >> @@ -339,7 +454,7 @@ func (self *Database) GetAllTags() *Tags {\r
 342 >>      if tags == nil {\r
 343 >>              return nil\r
 344 >>      }\r
 345 >> -    return &Tags{tags: tags}\r
 346 >> +    return createTags(tags, nil)\r
 347 >>   }\r
 348 >>\r
 349 >>   /* Create a new query for 'database'.\r
 350 >> @@ -379,7 +494,7 @@ func (self *Database) CreateQuery(query string) *Query {\r
 351 >>      if q == nil {\r
 352 >>              return nil\r
 353 >>      }\r
 354 >> -    return &Query{query: q}\r
 355 >> +    return createQuery(q, nil)\r
 356 >>   }\r
 357 >>\r
 358 >>   /* Sort values for notmuch_query_set_sort */\r
 359 >> @@ -459,7 +574,7 @@ func (self *Query) SearchThreads() *Threads {\r
 360 >>      if threads == nil {\r
 361 >>              return nil\r
 362 >>      }\r
 363 >> -    return &Threads{threads: threads}\r
 364 >> +    return createThreads(threads, self)\r
 365 >>   }\r
 366 >>\r
 367 >>   /* Execute a query for messages, returning a notmuch_messages_t object\r
 368 >> @@ -505,7 +620,7 @@ func (self *Query) SearchMessages() *Messages {\r
 369 >>      if msgs == nil {\r
 370 >>              return nil\r
 371 >>      }\r
 372 >> -    return &Messages{messages: msgs}\r
 373 >> +    return createMessages(msgs, self)\r
 374 >>   }\r
 375 >>\r
 376 >>   /* Destroy a notmuch_query_t along with any associated resources.\r
 377 >> @@ -607,7 +722,7 @@ func (self *Messages) Get() *Message {\r
 378 >>      if msg == nil {\r
 379 >>              return nil\r
 380 >>      }\r
 381 >> -    return &Message{message: msg}\r
 382 >> +    return createMessage(msg, self)\r
 383 >>   }\r
 384 >>\r
 385 >>   /* Move the 'messages' iterator to the next message.\r
 386 >> @@ -659,7 +774,7 @@ func (self *Messages) CollectTags() *Tags {\r
 387 >>      if tags == nil {\r
 388 >>              return nil\r
 389 >>      }\r
 390 >> -    return &Tags{tags: tags}\r
 391 >> +    return createTags(tags, self)\r
 392 >>   }\r
 393 >>\r
 394 >>   /* Get the message ID of 'message'.\r
 395 >> @@ -739,7 +854,7 @@ func (self *Message) GetReplies() *Messages {\r
 396 >>      if msgs == nil {\r
 397 >>              return nil\r
 398 >>      }\r
 399 >> -    return &Messages{messages: msgs}\r
 400 >> +    return createMessages(msgs, self)\r
 401 >>   }\r
 402 >>\r
 403 >>   /* Get a filename for the email corresponding to 'message'.\r
 404 >> @@ -871,7 +986,7 @@ func (self *Message) GetTags() *Tags {\r
 405 >>      if tags == nil {\r
 406 >>              return nil\r
 407 >>      }\r
 408 >> -    return &Tags{tags: tags}\r
 409 >> +    return createTags(tags, self)\r
 410 >>   }\r
 411 >>\r
 412 >>   /* The longest possible tag value. */\r
 413 \r
 414 \r