[PATCH] ruby: make sure the database is closed

[PATCH] ruby: make sure the database is closed

[Felipe Contreras]

If the Ruby code does not manually close the database, we need to make
sure it's closed when garbage collected.

In Ruby, users are not _required_ to close, the garbage collector should
take care of that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/database.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 982fd59..7b2ed47 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -20,10 +20,16 @@
 
 #include "defs.h"
 
+static void
+database_free (void *p)
+{
+    notmuch_database_close (p);
+}
+
 VALUE
 notmuch_rb_database_alloc (VALUE klass)
 {
-    return Data_Wrap_Struct (klass, NULL, NULL, NULL);
+    return Data_Wrap_Struct (klass, NULL, database_free, NULL);
 }
 
 /*
-- 
1.7.10

[PATCH] ruby: make sure the database is closed

[Felipe Contreras]

If the Ruby code does not manually close the database, we need to make
sure it's closed when garbage collected.

In Ruby, users are not _required_ to close, the garbage collector should
take care of that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/database.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 982fd59..7b2ed47 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -20,10 +20,16 @@
 
 #include "defs.h"
 
+static void
+database_free (void *p)
+{
+    notmuch_database_close (p);
+}
+
 VALUE
 notmuch_rb_database_alloc (VALUE klass)
 {
-    return Data_Wrap_Struct (klass, NULL, NULL, NULL);
+    return Data_Wrap_Struct (klass, NULL, database_free, NULL);
 }
 
 /*
-- 
1.7.10

Re: [PATCH] ruby: make sure the database is closed

[Ali Polatel]

2012/4/23 Felipe Contreras <felipe.contreras@gmail.com>:
> If the Ruby code does not manually close the database, we need to make
> sure it's closed when garbage collected.
>
> In Ruby, users are not _required_ to close, the garbage collector should
> take care of that.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  bindings/ruby/database.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
> index 982fd59..7b2ed47 100644
> --- a/bindings/ruby/database.c
> +++ b/bindings/ruby/database.c
> @@ -20,10 +20,16 @@
>
>  #include "defs.h"
>
> +static void
> +database_free (void *p)
> +{
> +    notmuch_database_close (p);
> +}
> +
>  VALUE
>  notmuch_rb_database_alloc (VALUE klass)
>  {
> -    return Data_Wrap_Struct (klass, NULL, NULL, NULL);
> +    return Data_Wrap_Struct (klass, NULL, database_free, NULL);
>  }
>
>  /*
> --
> 1.7.10
>

I'd rather not do this.
Please read: http://comments.gmane.org/gmane.comp.lang.ruby.general/320324

        -alip

Re: [PATCH] ruby: make sure the database is closed

[Felipe Contreras]

On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel <alip@exherbo.org> wrote:

> I'd rather not do this.
> Please read: http://comments.gmane.org/gmane.comp.lang.ruby.general/320324

OK, I've read this.. So?

The order in which Ruby's garbage-collector frees the database and
other objects is irrelevant, because with this patch we are not
manually freeing other objects, only the database.

Sure, it's _better_ if the user calls close(), even better if it's
inside an 'ensure', and even better if blocks are used (which I am
using in most cases), but that's not *required*.

The user might just do:

def foo
  db = Notmuch::Database.new($db_name, :mode => Notmuch::MODE_READ_WRITE)
end

That's perfectly fine in Ruby (although not ideal), since 'db' will
get garbage-collected. But nobody will be able to use the database
again until that process is killed.

You think that's correct?

-- 
Felipe Contreras

Re: [PATCH] ruby: make sure the database is closed

[Ali Polatel]

2012/4/23 Felipe Contreras <felipe.contreras@gmail.com>:
> On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel <alip@exherbo.org> wrote:
>
>> I'd rather not do this.
>> Please read: http://comments.gmane.org/gmane.comp.lang.ruby.general/320324
>
> OK, I've read this.. So?

You are one step close to what I thought you had thought.

> The order in which Ruby's garbage-collector frees the database and
> other objects is irrelevant, because with this patch we are not
> manually freeing other objects, only the database.

What I wanted was to make all objects "depending" on the database to be unusable
once the database object is freed. Seeing that's not practically easy
I decided to leave
it to the user.

> Sure, it's _better_ if the user calls close(), even better if it's
> inside an 'ensure', and even better if blocks are used (which I am
> using in most cases), but that's not *required*.

If you have such a use case, I'm fine with that patch.
I might push it in the next few days or get someone else to push it.

> The user might just do:
>
> def foo
>  db = Notmuch::Database.new($db_name, :mode => Notmuch::MODE_READ_WRITE)
> end
>
> That's perfectly fine in Ruby (although not ideal), since 'db' will
> get garbage-collected. But nobody will be able to use the database
> again until that process is killed.
>
> You think that's correct?

Yes that is correct. I have not thought about this.
I'd say it's a partial misunderstanding on my part due to lack of
(and/or too much) vodka.

> --
> Felipe Contreras

        -alip

Re: [PATCH] ruby: make sure the database is closed

[Felipe Contreras]

On Mon, Apr 23, 2012 at 11:43 PM, Ali Polatel <alip@exherbo.org> wrote:
> 2012/4/23 Felipe Contreras <felipe.contreras@gmail.com>:
>> On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel <alip@exherbo.org> wrote:
>>
>>> I'd rather not do this.
>>> Please read: http://comments.gmane.org/gmane.comp.lang.ruby.general/320324
>>
>> OK, I've read this.. So?
>
> You are one step close to what I thought you had thought.

I don't parse that. I meant that _now_ I've read it.

>> The order in which Ruby's garbage-collector frees the database and
>> other objects is irrelevant, because with this patch we are not
>> manually freeing other objects, only the database.
>
> What I wanted was to make all objects "depending" on the database to be unusable
> once the database object is freed. Seeing that's not practically easy
> I decided to leave
> it to the user.

Well, sure, but if the user has problems, the user can keep the
database object around.

Personally I don't see why an object, like say a query would remain
working correctly after the database is gone, either by calling
.close() directly, or just loosing the pointer to the original object.
I don't think users would expect that, or, even if they somehow found
it useful, that most likely would be very seldom, and hardly worth
worrying about it.

>> Sure, it's _better_ if the user calls close(), even better if it's
>> inside an 'ensure', and even better if blocks are used (which I am
>> using in most cases), but that's not *required*.
>
> If you have such a use case, I'm fine with that patch.
> I might push it in the next few days or get someone else to push it.

I did at some point, not sure if I do now. But that's beside the
point, the point is that it really does happen.

>> The user might just do:
>>
>> def foo
>>  db = Notmuch::Database.new($db_name, :mode => Notmuch::MODE_READ_WRITE)
>> end
>>
>> That's perfectly fine in Ruby (although not ideal), since 'db' will
>> get garbage-collected. But nobody will be able to use the database
>> again until that process is killed.
>>
>> You think that's correct?
>
> Yes that is correct. I have not thought about this.
> I'd say it's a partial misunderstanding on my part due to lack of
> (and/or too much) vodka.

Well, there's only two options.

a) This works:

  def foo
    db = Notmuch::Database.new($db_name, :mode => Notmuch::MODE_READ_WRITE)
  end

(as in; the database gets closed eventually)

b) This works:

  def start_query(search)
    db = Notmuch::Database.new($db_name)
    return db.query(search)
  end

(as in; the query returned would keep working properly)

I personally don't see why anybody would want b), and if they have
problems with code like that, I think it's obvious to see why.

I think we should go for a) and thus apply the patch.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] ruby: make sure the database is closed

[Ali Polatel]

2012/4/24 Felipe Contreras <felipe.contreras@gmail.com>:
> On Mon, Apr 23, 2012 at 11:43 PM, Ali Polatel <alip@exherbo.org> wrote:
>> 2012/4/23 Felipe Contreras <felipe.contreras@gmail.com>:
>>> On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel <alip@exherbo.org> wrote:
>>>
>>>> I'd rather not do this.
>>>> Please read: http://comments.gmane.org/gmane.comp.lang.ruby.general/320324
>>>
>>> OK, I've read this.. So?
>>
>> You are one step close to what I thought you had thought.
>
> I don't parse that. I meant that _now_ I've read it.

Sorry, my subconscious likes to barf on emails every now and then.

>>> The order in which Ruby's garbage-collector frees the database and
>>> other objects is irrelevant, because with this patch we are not
>>> manually freeing other objects, only the database.
>>
>> What I wanted was to make all objects "depending" on the database to be unusable
>> once the database object is freed. Seeing that's not practically easy
>> I decided to leave
>> it to the user.
>
> Well, sure, but if the user has problems, the user can keep the
> database object around.
>
> Personally I don't see why an object, like say a query would remain
> working correctly after the database is gone, either by calling
> .close() directly, or just loosing the pointer to the original object.
> I don't think users would expect that, or, even if they somehow found
> it useful, that most likely would be very seldom, and hardly worth
> worrying about it.

Working correctly is not expected but wouldn't it be more appropriate
to throw an exception rather than dumping core or printing on standard error?

>>> Sure, it's _better_ if the user calls close(), even better if it's
>>> inside an 'ensure', and even better if blocks are used (which I am
>>> using in most cases), but that's not *required*.
>>
>> If you have such a use case, I'm fine with that patch.
>> I might push it in the next few days or get someone else to push it.
>
> I did at some point, not sure if I do now. But that's beside the
> point, the point is that it really does happen.
>
>>> The user might just do:
>>>
>>> def foo
>>>  db = Notmuch::Database.new($db_name, :mode => Notmuch::MODE_READ_WRITE)
>>> end
>>>
>>> That's perfectly fine in Ruby (although not ideal), since 'db' will
>>> get garbage-collected. But nobody will be able to use the database
>>> again until that process is killed.
>>>
>>> You think that's correct?
>>
>> Yes that is correct. I have not thought about this.
>> I'd say it's a partial misunderstanding on my part due to lack of
>> (and/or too much) vodka.
>
> Well, there's only two options.
>
> a) This works:
>
>  def foo
>    db = Notmuch::Database.new($db_name, :mode => Notmuch::MODE_READ_WRITE)
>  end
>
> (as in; the database gets closed eventually)
>
> b) This works:
>
>  def start_query(search)
>    db = Notmuch::Database.new($db_name)
>    return db.query(search)
>  end
>
> (as in; the query returned would keep working properly)
>
> I personally don't see why anybody would want b), and if they have
> problems with code like that, I think it's obvious to see why.
>
> I think we should go for a) and thus apply the patch.

I wonder whether we can make both work somehow.
Maybe by using talloc explicitly and keeping reference pointers?
I don't know whether it's worth bothering.

> Cheers.

Cheers

> --
> Felipe Contreras

        -alip

Re: [PATCH] ruby: make sure the database is closed

[Felipe Contreras]

On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel <alip@exherbo.org> wrote:
> 2012/4/24 Felipe Contreras <felipe.contreras@gmail.com>:

>> Personally I don't see why an object, like say a query would remain
>> working correctly after the database is gone, either by calling
>> .close() directly, or just loosing the pointer to the original object.
>> I don't think users would expect that, or, even if they somehow found
>> it useful, that most likely would be very seldom, and hardly worth
>> worrying about it.
>
> Working correctly is not expected but wouldn't it be more appropriate
> to throw an exception rather than dumping core or printing on standard error?

Sure, if that was possible.

> I wonder whether we can make both work somehow.
> Maybe by using talloc explicitly and keeping reference pointers?
> I don't know whether it's worth bothering.

Maybe, I don't see how, that's just not how C works. Maybe talloc does
have some way to figure out if a pointer has been freed, but I doubt
that, and I can't find it by grepping through the API.

Another option would be hook into talloc's destructor so we know when
an object is freed and taint it, but then we would be overriding
notmuch's destructor, and there's no way around that (unless we tap
into talloc's internal structures). A way to workaround that would be
to modify notmuch's API so that we can specify a destructor for
notmuch objects, but that would be tedious, and I doubt a lof people
beside us would benefit from that.

In the meantime, it doesn't hurt to apply this patch.

-- 
Felipe Contreras

Re: [PATCH] ruby: make sure the database is closed

[Austin Clements]

Quoth Felipe Contreras on Apr 24 at  3:45 am:
> On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel <alip@exherbo.org> wrote:
> > 2012/4/24 Felipe Contreras <felipe.contreras@gmail.com>:
> 
> >> Personally I don't see why an object, like say a query would remain
> >> working correctly after the database is gone, either by calling
> >> .close() directly, or just loosing the pointer to the original object.
> >> I don't think users would expect that, or, even if they somehow found
> >> it useful, that most likely would be very seldom, and hardly worth
> >> worrying about it.
> >
> > Working correctly is not expected but wouldn't it be more appropriate
> > to throw an exception rather than dumping core or printing on standard error?
> 
> Sure, if that was possible.
> 
> > I wonder whether we can make both work somehow.
> > Maybe by using talloc explicitly and keeping reference pointers?
> > I don't know whether it's worth bothering.
> 
> Maybe, I don't see how, that's just not how C works. Maybe talloc does
> have some way to figure out if a pointer has been freed, but I doubt
> that, and I can't find it by grepping through the API.
> 
> Another option would be hook into talloc's destructor so we know when
> an object is freed and taint it, but then we would be overriding
> notmuch's destructor, and there's no way around that (unless we tap
> into talloc's internal structures). A way to workaround that would be
> to modify notmuch's API so that we can specify a destructor for
> notmuch objects, but that would be tedious, and I doubt a lof people
> beside us would benefit from that.

I believe (though I might be wrong) that bindings could simply
maintain their own talloc references to C objects returned by
libnotmuch to prevent them from being freed until the wrapper object
is garbage collected.  This would require modifying all of the
library's _destroy functions to use talloc_find_parent_bytype and
talloc_unlink instead of simply calling talloc_free, but I don't think
this change would be particularly invasive and it certainly wouldn't
affect the library interface.

Re: [PATCH] ruby: make sure the database is closed

[Felipe Contreras]

On Tue, Apr 24, 2012 at 4:15 AM, Austin Clements <amdragon@mit.edu> wrote:
> Quoth Felipe Contreras on Apr 24 at  3:45 am:
>> On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel <alip@exherbo.org> wrote:
>> > 2012/4/24 Felipe Contreras <felipe.contreras@gmail.com>:
>>
>> >> Personally I don't see why an object, like say a query would remain
>> >> working correctly after the database is gone, either by calling
>> >> .close() directly, or just loosing the pointer to the original object.
>> >> I don't think users would expect that, or, even if they somehow found
>> >> it useful, that most likely would be very seldom, and hardly worth
>> >> worrying about it.
>> >
>> > Working correctly is not expected but wouldn't it be more appropriate
>> > to throw an exception rather than dumping core or printing on standard error?
>>
>> Sure, if that was possible.
>>
>> > I wonder whether we can make both work somehow.
>> > Maybe by using talloc explicitly and keeping reference pointers?
>> > I don't know whether it's worth bothering.
>>
>> Maybe, I don't see how, that's just not how C works. Maybe talloc does
>> have some way to figure out if a pointer has been freed, but I doubt
>> that, and I can't find it by grepping through the API.
>>
>> Another option would be hook into talloc's destructor so we know when
>> an object is freed and taint it, but then we would be overriding
>> notmuch's destructor, and there's no way around that (unless we tap
>> into talloc's internal structures). A way to workaround that would be
>> to modify notmuch's API so that we can specify a destructor for
>> notmuch objects, but that would be tedious, and I doubt a lof people
>> beside us would benefit from that.
>
> I believe (though I might be wrong) that bindings could simply
> maintain their own talloc references to C objects returned by
> libnotmuch to prevent them from being freed until the wrapper object
> is garbage collected.  This would require modifying all of the
> library's _destroy functions to use talloc_find_parent_bytype and
> talloc_unlink instead of simply calling talloc_free, but I don't think
> this change would be particularly invasive and it certainly wouldn't
> affect the library interface.

That might work, but still, I don't see why this patch can't be applied.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] ruby: make sure the database is closed

[Ali Polatel]

2012/4/24 Felipe Contreras <felipe.contreras@gmail.com>:
> On Tue, Apr 24, 2012 at 4:15 AM, Austin Clements <amdragon@mit.edu> wrote:
>> Quoth Felipe Contreras on Apr 24 at  3:45 am:
>>> On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel <alip@exherbo.org> wrote:
>>> > 2012/4/24 Felipe Contreras <felipe.contreras@gmail.com>:
>>>
>>> >> Personally I don't see why an object, like say a query would remain
>>> >> working correctly after the database is gone, either by calling
>>> >> .close() directly, or just loosing the pointer to the original object.
>>> >> I don't think users would expect that, or, even if they somehow found
>>> >> it useful, that most likely would be very seldom, and hardly worth
>>> >> worrying about it.
>>> >
>>> > Working correctly is not expected but wouldn't it be more appropriate
>>> > to throw an exception rather than dumping core or printing on standard error?
>>>
>>> Sure, if that was possible.
>>>
>>> > I wonder whether we can make both work somehow.
>>> > Maybe by using talloc explicitly and keeping reference pointers?
>>> > I don't know whether it's worth bothering.
>>>
>>> Maybe, I don't see how, that's just not how C works. Maybe talloc does
>>> have some way to figure out if a pointer has been freed, but I doubt
>>> that, and I can't find it by grepping through the API.
>>>
>>> Another option would be hook into talloc's destructor so we know when
>>> an object is freed and taint it, but then we would be overriding
>>> notmuch's destructor, and there's no way around that (unless we tap
>>> into talloc's internal structures). A way to workaround that would be
>>> to modify notmuch's API so that we can specify a destructor for
>>> notmuch objects, but that would be tedious, and I doubt a lof people
>>> beside us would benefit from that.
>>
>> I believe (though I might be wrong) that bindings could simply
>> maintain their own talloc references to C objects returned by
>> libnotmuch to prevent them from being freed until the wrapper object
>> is garbage collected.  This would require modifying all of the
>> library's _destroy functions to use talloc_find_parent_bytype and
>> talloc_unlink instead of simply calling talloc_free, but I don't think
>> this change would be particularly invasive and it certainly wouldn't
>> affect the library interface.
>
> That might work, but still, I don't see why this patch can't be applied.

I don't have anything against applying this patch.
If this fix has a kind of urgency -and I doubt it does- please get
someone to push the patch.
Below is my confirmation to accept the consequences:
LGTM

Otherwise, I'd rather we fix it properly most probably using the
method described in Austin's previous mail.

> Cheers.
>
> --
> Felipe Contreras

        -alip