[ruby-dev:13051] Re: patch for ext/gdbm

新井です。

In message "[ruby-dev:12220] Re: patch for ext/gdbm"
  on Tue, 13 Feb 2001 06:57:27 +0900,
  Koji Arai <[e-mail]> wrote:
> 新井です。

> さらに、今気が付いたのですが GDBM はメモリリークしてますね。
> # gdbm_fetch() 後はfree()しないといけない
> 
> 例えば以下は，メモリを確保し続けます
> 
>   require 'gdbm'
> 
>   db = GDBM.new("/tmp/hoge2")
>   db["foo"] = "x" * 1024 * 1024
>   loop {
>     db.index "foo"
>     sleep 0.5
>   }

GDBM にまだメモリリークしてる箇所がありました。
gdbm_firstkey() で取得した key は free しなくてはいけないようです。
GDBM の Info の例には

        key = gdbm_firstkey ( dbf );
        while ( key.dptr ) {
           nextkey = gdbm_nextkey ( dbf, key );
           if ( some condition ) {
              gdbm_delete ( dbf, key );
              free ( key.dptr );
           }
           key = nextkey;
        }

とあるのですが

        key = gdbm_firstkey ( dbf );
        dptr = key.dptr; /* Is it really needed ? */
        while ( key.dptr ) {
           nextkey = gdbm_nextkey ( dbf, key );
           if ( some condition ) {
              gdbm_delete ( dbf, key );
              free ( key.dptr );
           }
           key = nextkey;
        }

	if (dptr) free(dptr); /* Is it really needed? */

な気がします。というのも以下はリークしているようなので

	require 'gdbm'

	gdm = GDBM.new("foo")
	loop {
	  gdbm["x" * 1024 * 1024] = 0
	  gdbm.delete_if { nil }
	  sleep 0.1
	}

これの対処はThreadやブロックでの再入を考慮しなくては行けなかっ
たので gdbm_fetch のときのような lazy な手法が一部使えません
でした。ですので、ちょっと見苦しいソースになってます。何かよ
り良い案があればどなたか御指摘願います。


# gc.c はなんか「ついで」みたいです。(^^;
# いつ直したか自分でも覚えてない

Index: gc.c
===================================================================
RCS file: /src/ruby/gc.c,v
retrieving revision 1.65
diff -u -p -u -r1.65 gc.c
--- gc.c	2001/03/26 08:57:10	1.65
+++ gc.c	2001/04/28 12:31:40
@@ -1147,7 +1147,7 @@ rm_final(os, proc)
 static VALUE
 finals()
 {
-    rb_warn("ObjectSpace::finals is deprecated");
+    rb_warn("ObjectSpace::finalizers is deprecated");
     return finalizers;
 }
 
Index: ext/gdbm/gdbm.c
===================================================================
RCS file: /src/ruby/ext/gdbm/gdbm.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 gdbm.c
--- ext/gdbm/gdbm.c	2001/02/13 06:05:14	1.14
+++ ext/gdbm/gdbm.c	2001/04/28 12:31:46
@@ -148,21 +148,43 @@ fgdbm_close(obj)
 }
 
 static datum
-gdbm_fetch_1(dbm, key)
+gdbm_fetch_with_free(dbm, key)
     GDBM_FILE dbm;
     datum key;
 {
-    static char *ptr;
+    static char *last_dptr;
     datum val;
 
     val = gdbm_fetch(dbm, key);
-    if (ptr) free(ptr);
-    ptr = val.dptr;
+    if (last_dptr) free(last_dptr);
+    last_dptr = val.dptr;
 
     return val;
 }
 
+static datum
+gdbm_firstkey_with_free(dbm)
+    GDBM_FILE dbm;
+{
+    static char *last_dptr;
+    datum key;
+
+    key = gdbm_firstkey(dbm);
+    if (last_dptr) free(last_dptr);
+    last_dptr = key.dptr;
+
+    return key;
+}
+
 static VALUE
+gdbm_dptr_free(dptr)
+    char **dptr;
+{
+    if (*dptr) free(*dptr);
+    return Qnil;
+}
+
+static VALUE
 fgdbm_fetch(obj, keystr, ifnone)
     VALUE obj, keystr, ifnone;
 {
@@ -176,7 +198,7 @@ fgdbm_fetch(obj, keystr, ifnone)
 
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
-    value = gdbm_fetch_1(dbm, key);
+    value = gdbm_fetch_with_free(dbm, key);
     if (value.dptr == 0) {
 	if (ifnone == Qnil && rb_block_given_p())
 	    return rb_yield(rb_tainted_str_new(key.dptr, key.dsize));
@@ -222,8 +244,8 @@ fgdbm_index(obj, valstr)
 
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	if (val.dsize == RSTRING(valstr)->len &&
 	    memcmp(val.dptr, RSTRING(valstr)->ptr, val.dsize) == 0)
 	    return rb_tainted_str_new(key.dptr, key.dsize);
@@ -292,9 +314,9 @@ fgdbm_shift(obj)
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
 
-    key = gdbm_firstkey(dbm); 
+    key = gdbm_firstkey_with_free(dbm);
     if (!key.dptr) return Qnil;
-    val = gdbm_fetch_1(dbm, key);
+    val = gdbm_fetch_with_free(dbm, key);
     gdbm_delete(dbm, key);
 
     keystr = rb_tainted_str_new(key.dptr, key.dsize);
@@ -303,19 +325,23 @@ fgdbm_shift(obj)
 }
 
 static VALUE
-fgdbm_delete_if(obj)
-    VALUE obj;
+fgdbm_delete_if(args)
+    VALUE *args;
 {
     datum key, val;
     struct dbmdata *dbmp;
     GDBM_FILE dbm;
     VALUE keystr, valstr;
+    VALUE obj = args[0];
+    char **dptr = (char**)args[1];
 
     rb_secure(4);
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    key = gdbm_firstkey(dbm);
+    *dptr = key.dptr;
+    for (; key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	keystr = rb_tainted_str_new(key.dptr, key.dsize);
 	valstr = rb_tainted_str_new(val.dptr, val.dsize);
 	if (RTEST(rb_yield(rb_assoc_new(keystr, valstr)))) {
@@ -328,6 +354,20 @@ fgdbm_delete_if(obj)
 }
 
 static VALUE
+fgdbm_delete_if_m(obj)
+    VALUE obj;
+{
+    VALUE args[2];
+    char *dptr;
+
+    args[0] = obj;
+    args[1] = (VALUE)&dptr;
+
+    return rb_ensure(fgdbm_delete_if, (VALUE)args,
+                     gdbm_dptr_free, (VALUE)&dptr);
+}
+
+static VALUE
 fgdbm_clear(obj)
     VALUE obj;
 {
@@ -339,7 +379,7 @@ fgdbm_clear(obj)
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
     dbmp->di_size = -1;
-    for (key = gdbm_firstkey(dbm); key.dptr; key = nextkey) {
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = nextkey) {
 	nextkey = gdbm_nextkey(dbm, key);
 	if (gdbm_delete(dbm, key)) {
 	    rb_raise(rb_eGDBMError, "%s", gdbm_strerror(gdbm_errno));
@@ -360,8 +400,8 @@ fgdbm_invert(obj)
 
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	keystr = rb_tainted_str_new(key.dptr, key.dsize);
 	valstr = rb_tainted_str_new(val.dptr, val.dsize);
 	rb_hash_aset(hash, valstr, keystr);
@@ -449,7 +489,7 @@ fgdbm_length(obj)
     if (dbmp->di_size > 0) return INT2FIX(dbmp->di_size);
     dbm = dbmp->di_dbm;
 
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
 	i++;
     }
     dbmp->di_size = i;
@@ -470,7 +510,7 @@ fgdbm_empty_p(obj)
     if (dbmp->di_size < 0) {
 	dbm = dbmp->di_dbm;
 
-	for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+	for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
 	    i++;
 	}
     }
@@ -482,52 +522,94 @@ fgdbm_empty_p(obj)
 }
 
 static VALUE
-fgdbm_each_value(obj)
-    VALUE obj;
+fgdbm_each_value(args)
+    VALUE *args;
 {
     datum key, val;
     struct dbmdata *dbmp;
     GDBM_FILE dbm;
+    VALUE obj = args[0];
+    char **dptr = (char**)args[1];
 
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    key = gdbm_firstkey(dbm);
+    *dptr = key.dptr;
+
+    for (; key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	rb_yield(rb_tainted_str_new(val.dptr, val.dsize));
     }
     return obj;
 }
 
 static VALUE
-fgdbm_each_key(obj)
+fgdbm_each_value_m(obj)
     VALUE obj;
 {
+    VALUE args[2];
+    char *dptr;
+
+    args[0] = obj;
+    args[1] = (VALUE)&dptr;
+
+    return rb_ensure(fgdbm_each_value, (VALUE)args,
+                     gdbm_dptr_free, (VALUE)&dptr);
+}
+
+static VALUE
+fgdbm_each_key(args)
+    VALUE *args;
+{
     datum key;
     struct dbmdata *dbmp;
     GDBM_FILE dbm;
+    VALUE obj = args[0];
+    char **dptr = (char **)args[1];
 
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+
+    key = gdbm_firstkey(dbm);
+    *dptr = key.dptr;
+    for (; key.dptr; key = gdbm_nextkey(dbm, key)) {
 	rb_yield(rb_tainted_str_new(key.dptr, key.dsize));
     }
     return obj;
 }
 
 static VALUE
-fgdbm_each_pair(obj)
+fgdbm_each_key_m(obj)
     VALUE obj;
 {
+    VALUE args[2];
+    char *dptr;
+
+    args[0] = obj;
+    args[1] = (VALUE)&dptr;
+
+    return rb_ensure(fgdbm_each_key, (VALUE)args,
+                     gdbm_dptr_free, (VALUE)&dptr);
+}
+
+static VALUE
+fgdbm_each_pair(args)
+    VALUE *args;
+{
     datum key, val;
     GDBM_FILE dbm;
     struct dbmdata *dbmp;
     VALUE keystr, valstr;
+    VALUE obj = args[0];
+    char **dptr = (char **)args[1];
 
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
 
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    key = gdbm_firstkey(dbm);
+    *dptr = key.dptr;
+    for (; key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	keystr = rb_tainted_str_new(key.dptr, key.dsize);
 	valstr = rb_tainted_str_new(val.dptr, val.dsize);
 	rb_yield(rb_assoc_new(keystr, valstr));
@@ -537,6 +619,20 @@ fgdbm_each_pair(obj)
 }
 
 static VALUE
+fgdbm_each_pair_m(obj)
+    VALUE obj;
+{
+    VALUE args[2];
+    char *dptr;
+
+    args[0] = obj;
+    args[1] = (VALUE)&dptr;
+
+    return rb_ensure(fgdbm_each_pair, (VALUE)args,
+                     gdbm_dptr_free, (VALUE)&dptr);
+}
+
+static VALUE
 fgdbm_keys(obj)
     VALUE obj;
 {
@@ -549,7 +645,7 @@ fgdbm_keys(obj)
     dbm = dbmp->di_dbm;
 
     ary = rb_ary_new();
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
 	rb_ary_push(ary, rb_tainted_str_new(key.dptr, key.dsize));
     }
 
@@ -569,8 +665,8 @@ fgdbm_values(obj)
     dbm = dbmp->di_dbm;
 
     ary = rb_ary_new();
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	rb_ary_push(ary, rb_tainted_str_new(val.dptr, val.dsize));
     }
 
@@ -610,8 +706,8 @@ fgdbm_has_value(obj, valstr)
 
     GetDBM(obj, dbmp);
     dbm = dbmp->di_dbm;
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	if (val.dsize == RSTRING(valstr)->len &&
 	    memcmp(val.dptr, RSTRING(valstr)->ptr, val.dsize) == 0)
 	    return Qtrue;
@@ -632,8 +728,8 @@ fgdbm_to_a(obj)
     dbm = dbmp->di_dbm;
 
     ary = rb_ary_new();
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	rb_ary_push(ary, rb_assoc_new(rb_tainted_str_new(key.dptr, key.dsize),
 				rb_tainted_str_new(val.dptr, val.dsize)));
     }
@@ -750,8 +846,8 @@ fgdbm_to_hash(obj)
     dbm = dbmp->di_dbm;
 
     hash = rb_hash_new();
-    for (key = gdbm_firstkey(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
-	val = gdbm_fetch_1(dbm, key);
+    for (key = gdbm_firstkey_with_free(dbm); key.dptr; key = gdbm_nextkey(dbm, key)) {
+	val = gdbm_fetch_with_free(dbm, key);
 	rb_hash_aset(hash, rb_tainted_str_new(key.dptr, key.dsize),
 		           rb_tainted_str_new(val.dptr, val.dsize));
     }
@@ -789,14 +885,14 @@ Init_gdbm()
     rb_define_alias(cGDBM,  "size", "length");
     rb_define_method(cGDBM, "empty?", fgdbm_empty_p, 0);
     rb_define_method(cGDBM, "each", fgdbm_each_pair, 0);
-    rb_define_method(cGDBM, "each_value", fgdbm_each_value, 0);
-    rb_define_method(cGDBM, "each_key", fgdbm_each_key, 0);
-    rb_define_method(cGDBM, "each_pair", fgdbm_each_pair, 0);
+    rb_define_method(cGDBM, "each_value", fgdbm_each_value_m, 0);
+    rb_define_method(cGDBM, "each_key", fgdbm_each_key_m, 0);
+    rb_define_method(cGDBM, "each_pair", fgdbm_each_pair_m, 0);
     rb_define_method(cGDBM, "keys", fgdbm_keys, 0);
     rb_define_method(cGDBM, "values", fgdbm_values, 0);
     rb_define_method(cGDBM, "shift", fgdbm_shift, 1);
     rb_define_method(cGDBM, "delete", fgdbm_delete, 1);
-    rb_define_method(cGDBM, "delete_if", fgdbm_delete_if, 0);
+    rb_define_method(cGDBM, "delete_if", fgdbm_delete_if_m, 0);
     rb_define_method(cGDBM, "reject!", fgdbm_delete_if, 0);
     rb_define_method(cGDBM, "reject", fgdbm_reject, 0);
     rb_define_method(cGDBM, "clear", fgdbm_clear, 0);

--
新井康司 (Koji Arai)