Tempfile#stat raises java.lang.NullPointerException when unlinked

Details

Type: Bug
Status: Resolved
Priority: Major
Resolution: Fixed
Affects Version/s: JRuby 1.6.6
Fix Version/s: JRuby 1.7.0.pre1
Component/s: Standard Library
Labels:
None
Environment:
OS X 10.7.3

Number of attachments :
0

Description

require 'tempfile'

Tempfile.open 'test' do |io|
io.unlink

io.write "hi"
io.rewind # flush

p io.stat.size
end

When run with ruby 1.8:

$ ruby -v t.rb
ruby 1.8.7 (2010-01-10 patchlevel 249) [universal-darwin11.0]
2

When run with jruby:

$ jruby -v t.rb
Unable to find a $JAVA_HOME at "/usr", continuing with system-provided Java...
jruby 1.6.6 (ruby-1.8.7-p357) (2012-01-30 5673572) (Java HotSpot(TM) 64-Bit Server VM 1.6.0_29) [darwin-x86_64-java]
RubyFileStat.java:115:in `setup': java.lang.NullPointerException
from RubyFileStat.java:92:in `newFileStat'
from Ruby.java:2894:in `newFileStat'
from RubyFile.java:834:in `stat'
from RubyFile$i$0$0$stat.gen:65535:in `call'
from CachingCallSite.java:292:in `cacheAndCall'
from CachingCallSite.java:135:in `call'
from t.rb:9:in `block_0$RUBY$_file_'
from t$block_0$RUBY$_file_:65535:in `call'
from CompiledBlock.java:112:in `yield'
from CompiledBlock.java:95:in `yield'
from Block.java:130:in `yield'
from RubyTempfile.java:265:in `open'
from RubyTempfile$s$0$1$open.gen:65535:in `call'
from DynamicMethod.java:211:in `call'
from CachingCallSite.java:322:in `cacheAndCall'
from CachingCallSite.java:178:in `callBlock'
from CachingCallSite.java:187:in `callIter'
from t.rb:3:in `_file_'
from t.rb:-1:in `load'
from Ruby.java:695:in `runScript'
from Ruby.java:688:in `runScript'
from Ruby.java:595:in `runNormally'
from Ruby.java:444:in `runFromMain'
from Main.java:344:in `doRunFromMain'
from Main.java:256:in `internalRun'
from Main.java:222:in `run'
from Main.java:206:in `run'
from Main.java:186:in `main'

Issue Links

is related to

JRUBY-6688 Tempfile#{unlink,delete} should warn or actualy do something

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Charles Oliver Nutter added a comment - 20/Feb/12 6:17 PM

Confirmed on master.

Show

Charles Oliver Nutter added a comment - 20/Feb/12 6:17 PM Confirmed on master.

Hide

Permalink

Charles Oliver Nutter added a comment - 20/Feb/12 6:40 PM

So...I can fix the NPE, but this still isn't going to work. JRuby's stat support is always based on a filename, since we have no way to get at real file descriptors (and so we can't call fstat). With the fix below, it doesn't NPE anymore, but it raises ENOENT since the path no longer points to a file on the filesystem.

diff --git a/src/org/jruby/ext/tempfile/Tempfile.java b/src/org/jruby/ext/tempfile/Tempfile.java
index 17911b5..10c6592 100644
--- a/src/org/jruby/ext/tempfile/Tempfile.java
+++ b/src/org/jruby/ext/tempfile/Tempfile.java
@@ -242,7 +242,6 @@ public class Tempfile extends RubyTempfile {
         if (!tmpFile.exists() || tmpFile.delete()) {
             referenceSet.remove(reaper);
             reaper.released = true;
-            path = null;
         }
         return context.getRuntime().getNil();
     }

Where is this pattern being used?

Show

Charles Oliver Nutter added a comment - 20/Feb/12 6:40 PM So...I can fix the NPE, but this still isn't going to work. JRuby's stat support is always based on a filename, since we have no way to get at real file descriptors (and so we can't call fstat). With the fix below, it doesn't NPE anymore, but it raises ENOENT since the path no longer points to a file on the filesystem. diff --git a/src/org/jruby/ext/tempfile/Tempfile.java b/src/org/jruby/ext/tempfile/Tempfile.java index 17911b5..10c6592 100644 --- a/src/org/jruby/ext/tempfile/Tempfile.java +++ b/src/org/jruby/ext/tempfile/Tempfile.java @@ -242,7 +242,6 @@ public class Tempfile extends RubyTempfile { if (!tmpFile.exists() || tmpFile.delete()) { referenceSet.remove(reaper); reaper.released = true; - path = null; } return context.getRuntime().getNil(); } Where is this pattern being used?

Hide

Permalink

Eric Hodel added a comment - 20/Feb/12 7:09 PM

In mechanize a Tempfile is used for large responses. They're unlinked immediately.

See:

https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L807-809

https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L822-824

https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L1051-1053

(I should refactor these!)

The length is used here:

https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L706-714

Show

Eric Hodel added a comment - 20/Feb/12 7:09 PM In mechanize a Tempfile is used for large responses. They're unlinked immediately. See: https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L807-809 https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L822-824 https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L1051-1053 (I should refactor these!) The length is used here: https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L706-714

Hide

Permalink

Eric Hodel added a comment - 20/Feb/12 8:58 PM

I think it is OK to make unlink a no-op for jruby if you can't stat a file descriptor. This would be the same behavior as on CRuby windows users. This doesn't protect Tempfile#stat from users that bypass Tempfile#unlink using File.unlink tempfile.path, but that should be an acceptable trade-off as such use is likely wrong.

PS: I've added a workaround to mechanize that jruby users can use (see the linked mechanize ticket).

Show

Eric Hodel added a comment - 20/Feb/12 8:58 PM I think it is OK to make unlink a no-op for jruby if you can't stat a file descriptor. This would be the same behavior as on CRuby windows users. This doesn't protect Tempfile#stat from users that bypass Tempfile#unlink using File.unlink tempfile.path, but that should be an acceptable trade-off as such use is likely wrong. PS: I've added a workaround to mechanize that jruby users can use (see the linked mechanize ticket).

Hide

Permalink

Eric Hodel added a comment - 20/Feb/12 9:00 PM

Oops, see https://github.com/tenderlove/mechanize/issues/205 for the related mechanize ticket.

Show

Eric Hodel added a comment - 20/Feb/12 9:00 PM Oops, see https://github.com/tenderlove/mechanize/issues/205 for the related mechanize ticket.

Hide

Permalink

Charles Oliver Nutter added a comment - 14/May/12 2:04 PM

I'm going to go with the no-op. close! will still delete the file, as will close(true), and we mark the file as "close on exit" for the JVM. The only cases where the file would leak is if people are closing it without unlinking (which would leak anyway) or if someone exits the process prematurely (which would leak before if you didn't actively call #unlink or an unlinking #close form).

This will be in JRuby 1.7, so we've got the preview and release candidates to shake out any issues.

Show

Charles Oliver Nutter added a comment - 14/May/12 2:04 PM I'm going to go with the no-op. close! will still delete the file, as will close(true), and we mark the file as "close on exit" for the JVM. The only cases where the file would leak is if people are closing it without unlinking (which would leak anyway) or if someone exits the process prematurely (which would leak before if you didn't actively call #unlink or an unlinking #close form). This will be in JRuby 1.7, so we've got the preview and release candidates to shake out any issues.

Hide

Permalink

Charles Oliver Nutter added a comment - 14/May/12 2:20 PM

commit 43bd20f28fa185430f7bf06f529db0221f9e79c3
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Mon May 14 14:11:44 2012 -0500

    Fix JRUBY-6477
    
    Because we can't unlink a file without making it path useless (and
    preventing other file methods that depend on path like #stat from
    working) we make unlink/delete a no-op. Tempfiles that are closed
    properly will still unlink, and normal JVM exit will also delete
    the file. IO's finalization also helps ensure Tempfiles that
    are walked away from still clean up.

Show

Charles Oliver Nutter added a comment - 14/May/12 2:20 PM commit 43bd20f28fa185430f7bf06f529db0221f9e79c3 Author: Charles Oliver Nutter <headius@headius.com> Date: Mon May 14 14:11:44 2012 -0500 Fix JRUBY-6477 Because we can't unlink a file without making it path useless (and preventing other file methods that depend on path like #stat from working) we make unlink/delete a no-op. Tempfiles that are closed properly will still unlink, and normal JVM exit will also delete the file. IO's finalization also helps ensure Tempfiles that are walked away from still clean up.

People

Assignee:

Charles Oliver Nutter

Reporter:

Eric Hodel

Vote (0)

Watch (0)

Dates

Created:

20/Feb/12 6:11 PM

Updated:

30/May/12 11:21 PM

Resolved:

14/May/12 2:20 PM