about summary refs log tree commit homepage
diff options
context:
space:
mode:
authorEric Wong <normalperson@yhbt.net>2010-02-13 01:35:55 -0800
committerEric Wong <normalperson@yhbt.net>2010-02-13 01:35:55 -0800
commit4247fafd0f361d2373df7213a1a0028554e93d93 (patch)
treee4e8e3f52b909725264463993422779892e3cdd4
parent883368f745af13a57b3784b834001a82823eee05 (diff)
downloadclogger-4247fafd0f361d2373df7213a1a0028554e93d93.tar.gz
The optional C extension leaked memory due to improper use of
the Ruby API, causing duplicated objects to never be garbage
collected.

I misread the Data_Make_Struct/Data_Wrap_Struct documentation
and ended up passing NULL as the "free" argument instead of -1,
causing the memory to never be freed.

From README.EXT in the MRI source which I misread:
> The free argument is the function to free the pointer
> allocation.  If this is -1, the pointer will be just freed.
> The functions mark and free will be called from garbage
> collector.
-rw-r--r--ext/clogger_ext/clogger.c2
-rw-r--r--test/test_clogger.rb18
2 files changed, 19 insertions, 1 deletions
diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c
index da6b5d2..6e14938 100644
--- a/ext/clogger_ext/clogger.c
+++ b/ext/clogger_ext/clogger.c
@@ -188,7 +188,7 @@ static VALUE clogger_alloc(VALUE klass)
 {
         struct clogger *c;
 
-        return Data_Make_Struct(klass, struct clogger, clogger_mark, 0, c);
+        return Data_Make_Struct(klass, struct clogger, clogger_mark, -1, c);
 }
 
 static struct clogger *clogger_get(VALUE self)
diff --git a/test/test_clogger.rb b/test/test_clogger.rb
index b086bbb..4dab3fc 100644
--- a/test/test_clogger.rb
+++ b/test/test_clogger.rb
@@ -587,4 +587,22 @@ class TestClogger < Test::Unit::TestCase
     assert ! cl.reentrant?
   end
 
+  # so we don't  care about the portability of this test
+  # if it doesn't leak on Linux, it won't leak anywhere else
+  # unless your C compiler or platform is otherwise broken
+  LINUX_PROC_PID_STATUS = "/proc/self/status"
+  def test_memory_leak
+    app = lambda { |env| [ 0, {}, [] ] }
+    clogger = Clogger.new(app, :logger => $stderr)
+    match_rss = /^VmRSS:\s+(\d+)/
+    if File.read(LINUX_PROC_PID_STATUS) =~ match_rss
+      before = $1.to_i
+      1000000.times { clogger.dup }
+      File.read(LINUX_PROC_PID_STATUS) =~ match_rss
+      after = $1.to_i
+      diff = after - before
+      assert(diff < 10000, "memory grew more than 10M: #{diff}")
+    end
+  end if RUBY_PLATFORM =~ /linux/ && test(?r, LINUX_PROC_PID_STATUS)
+
 end