[jruby~main:547bceee] JRUBY-4623: Tempfile does not clean up on GC run

[nicksieger@kenai.com]

Project:    jruby
Repository: main
Revision:   547bceeed5fbc208620d981c79fcc87cb5993d5c
Author:     nicksieger
Date:       2010-03-06 12:04:33 UTC
Link:       

Log Message:
------------
JRUBY-4623: Tempfile does not clean up on GC run
Added unit test as well, but didn't wire it to the index,
since these GC-related issues are not 100% reliably reproducible.


Revisions:
----------
547bceeed5fbc208620d981c79fcc87cb5993d5c


Modified Paths:
---------------
src/org/jruby/RubyTempfile.java


Added Paths:
------------
test/test_tempfile_cleanup.rb


Diffs:
------
diff --git a/src/org/jruby/RubyTempfile.java b/src/org/jruby/RubyTempfile.java
index ef8a42d..bec3b6b 100644
--- a/src/org/jruby/RubyTempfile.java
+++ b/src/org/jruby/RubyTempfile.java
@@ -30,6 +30,9 @@ package org.jruby;
 
 import java.io.File;
 import java.io.IOException;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
 import org.jruby.anno.JRubyClass;
 import org.jruby.anno.JRubyMethod;
 import org.jruby.platform.Platform;
@@ -38,14 +41,22 @@ import org.jruby.runtime.ObjectAllocator;
 import org.jruby.runtime.ThreadContext;
 import org.jruby.runtime.Visibility;
 import org.jruby.runtime.builtin.IRubyObject;
+import org.jruby.util.ReferenceReaper;
 import org.jruby.util.io.InvalidValueException;
 import org.jruby.util.io.ModeFlags;
+import org.jruby.util.io.OpenFile;
 
 /**
  * An implementation of tempfile.rb in Java.
  */
 @JRubyClass(name="Tempfile", parent="File")
 public class RubyTempfile extends RubyFile {
+
+    /** Keep strong references to the Reaper until cleanup */
+    private static final ConcurrentMap<Reaper, Boolean> referenceSet
+            = new ConcurrentHashMap<Reaper, Boolean>();
+    private transient volatile Reaper reaper;
+
     private static ObjectAllocator TEMPFILE_ALLOCATOR = new 
ObjectAllocator() {
         public IRubyObject allocate(Ruby runtime, RubyClass klass) {
             RubyFile instance = new RubyTempfile(runtime, klass);
@@ -117,6 +128,7 @@ public class RubyTempfile extends RubyFile {
                         path = tmp.getPath();
                         tmpFile.deleteOnExit();
                         initializeOpen();
+                        referenceSet.put(reaper = new Reaper(this, runtime, 
tmpFile, openFile), Boolean.TRUE);
                         return this;
                     }
                 } catch (IOException e) {
@@ -204,6 +216,8 @@ public class RubyTempfile extends RubyFile {
 
     @JRubyMethod(name = "close!", frame = true, visibility = 
Visibility.PUBLIC)
     public IRubyObject close_bang(ThreadContext context) {
+         referenceSet.remove(reaper);
+         reaper.released = true;
         _close(context);
         tmpFile.delete();
         return context.getRuntime().getNil();
@@ -211,7 +225,10 @@ public class RubyTempfile extends RubyFile {
 
     @JRubyMethod(name = {"unlink", "delete"}, frame = true)
     public IRubyObject unlink(ThreadContext context) {
-        if (tmpFile.exists()) tmpFile.delete();
+        if (!tmpFile.exists() || tmpFile.delete()) {
+            referenceSet.remove(reaper);
+            reaper.released = true;
+        }
         return context.getRuntime().getNil();
     }
 
@@ -242,4 +259,44 @@ public class RubyTempfile extends RubyFile {
 
         return tempfile;
     }
+
+    private static final class Reaper extends 
ReferenceReaper.Phantom<RubyTempfile> implements Runnable {
+        private volatile boolean released = false;
+        private final Ruby runtime;
+        private final File tmpFile;
+        private final OpenFile openFile;
+
+        Reaper(RubyTempfile file, Ruby runtime, File tmpFile, OpenFile 
openFile) {
+            super(file);
+            this.runtime = runtime;
+            this.tmpFile = tmpFile;
+            this.openFile = openFile;
+        }
+
+        public final void run() {
+            referenceSet.remove(this);
+            release();
+            clear();
+        }
+
+        final void release() {
+            if (!released) {
+                released = true;
+                if (openFile != null) {
+                    openFile.cleanup(runtime, false);
+                }
+                if (tmpFile.exists()) {
+                    boolean deleted = tmpFile.delete();
+                    if (runtime.getDebug().isTrue()) {
+                        String msg = "removing " + tmpFile.getPath() + " ... 
";
+                        if (deleted) {
+                            runtime.getErr().println(msg + "done");
+                        } else {
+                            runtime.getErr().println(msg + "can't delete");
+                        }
+                    }
+                }
+            }
+        }
+    }
 }
diff --git a/test/test_tempfile_cleanup.rb b/test/test_tempfile_cleanup.rb
new file mode 100644
index 0000000..f95fff0
--- /dev/null
+++ b/test/test_tempfile_cleanup.rb
@@ -0,0 +1,35 @@
+require 'tempfile'
+require 'java' if defined?(JRUBY_VERSION)
+require 'test/unit'
+require 'fileutils'
+
+class TestTempfilesCleanUp < Test::Unit::TestCase
+
+  def setup
+    @tmpdir = "tmp_#{$$}"
+    FileUtils.rm_f @tmpdir rescue nil
+    Dir.mkdir @tmpdir rescue nil
+  end
+
+  def teardown
+    FileUtils.rm_f @tmpdir
+  end
+
+  def test_cleanup
+    10.times { Tempfile.open('blah', @tmpdir) }
+
+    # check that files were created
+    assert Dir["#{@tmpdir}/*"].size > 0
+
+    100.times do
+      if defined?(JRUBY_VERSION)
+        java.lang.System.gc
+      else
+        GC.start
+      end
+    end
+
+    # test that the files are gone
+    assert_equal 0, Dir["#{@tmpdir}/*"].size, 'Files were not cleaned up'
+  end
+end