Created
March 6, 2010 13:04
-
-
Save vvs/323667 to your computer and use it in GitHub Desktop.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
commit 7050bc18a592b779d8fb102ef7ce583902763e3a | |
Author: Vladimir Sizikov <vsizikov@gmail.com> | |
Date: Sat Mar 6 13:04:33 2010 +0100 | |
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. | |
diff --git a/src/org/jruby/RubyTempfile.java b/src/org/jruby/RubyTempfile.java | |
index b77b9f4..874b68d 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); | |
@@ -116,6 +127,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) { | |
@@ -203,6 +215,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(); | |
@@ -210,7 +224,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(); | |
} | |
@@ -241,4 +258,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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment