[PATCH] ruby: make sure the database is closed
2012/4/24 Felipe Contreras : > On Tue, Apr 24, 2012 at 4:15 AM, Austin Clements wrote: >> Quoth Felipe Contreras on Apr 24 at ?3:45 am: >>> On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel wrote: >>> > 2012/4/24 Felipe Contreras : >>> >>> >> 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
Re: [PATCH] ruby: make sure the database is closed
2012/4/24 Felipe Contreras felipe.contre...@gmail.com: On Tue, Apr 24, 2012 at 4:15 AM, Austin Clements amdra...@mit.edu wrote: Quoth Felipe Contreras on Apr 24 at 3:45 am: On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel a...@exherbo.org wrote: 2012/4/24 Felipe Contreras felipe.contre...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] ruby: make sure the database is closed
On Tue, Apr 24, 2012 at 4:15 AM, Austin Clements wrote: > Quoth Felipe Contreras on Apr 24 at ?3:45 am: >> On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel wrote: >> > 2012/4/24 Felipe Contreras : >> >> >> 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
[PATCH] ruby: make sure the database is closed
On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel wrote: > 2012/4/24 Felipe Contreras : >> 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
[PATCH] ruby: make sure the database is closed
2012/4/24 Felipe Contreras : > On Mon, Apr 23, 2012 at 11:43 PM, Ali Polatel wrote: >> 2012/4/23 Felipe Contreras : >>> On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel 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
[PATCH] ruby: make sure the database is closed
On Mon, Apr 23, 2012 at 11:43 PM, Ali Polatel wrote: > 2012/4/23 Felipe Contreras : >> On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel 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
On Tue, Apr 24, 2012 at 4:15 AM, Austin Clements amdra...@mit.edu wrote: Quoth Felipe Contreras on Apr 24 at 3:45 am: On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel a...@exherbo.org wrote: 2012/4/24 Felipe Contreras felipe.contre...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] ruby: make sure the database is closed
Quoth Felipe Contreras on Apr 24 at 3:45 am: > On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel wrote: > > 2012/4/24 Felipe Contreras : > > >> 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.
[PATCH] ruby: make sure the database is closed
2012/4/23 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
> ---
> ?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
[PATCH] ruby: make sure the database is closed
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
---
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
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
---
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
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.contre...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] ruby: make sure the database is closed
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.contre...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] ruby: make sure the database is closed
2012/4/23 Felipe Contreras felipe.contre...@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.contre...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] ruby: make sure the database is closed
On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel a...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] ruby: make sure the database is closed
2012/4/23 Felipe Contreras felipe.contre...@gmail.com: On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel a...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] ruby: make sure the database is closed
On Mon, Apr 23, 2012 at 11:43 PM, Ali Polatel a...@exherbo.org wrote: 2012/4/23 Felipe Contreras felipe.contre...@gmail.com: On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel a...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] ruby: make sure the database is closed
2012/4/24 Felipe Contreras felipe.contre...@gmail.com: On Mon, Apr 23, 2012 at 11:43 PM, Ali Polatel a...@exherbo.org wrote: 2012/4/23 Felipe Contreras felipe.contre...@gmail.com: On Mon, Apr 23, 2012 at 5:04 PM, Ali Polatel a...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] ruby: make sure the database is closed
On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel a...@exherbo.org wrote: 2012/4/24 Felipe Contreras felipe.contre...@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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] ruby: make sure the database is closed
Quoth Felipe Contreras on Apr 24 at 3:45 am: On Tue, Apr 24, 2012 at 2:46 AM, Ali Polatel a...@exherbo.org wrote: 2012/4/24 Felipe Contreras felipe.contre...@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. ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
