jruby: warning: Tempfile#unlink or delete called on open file; ignoring #371

Closed
stan3 opened this Issue Mar 31, 2014 · 19 comments

Comments

Projects
None yet
5 participants

stan3 commented Mar 31, 2014

Getting this when using mechanize:

bundle/jruby/1.9/gems/mechanize-2.7.3/lib/mechanize/http/agent.rb:1219 warning: Tempfile#unlink or delete called on open file; ignoring

Code does appear to unlink after creating/opening the temp file.

Owner

leejarvis commented Mar 31, 2014

Thanks, maybe you could create a patch for this?

Owner

drbrain commented Mar 31, 2014

This is a JRuby warning and there is no way around it. You'll need to ask JRuby to stop emitting this warning.

drbrain closed this Mar 31, 2014

mrbrdo commented Jul 26, 2014

@drbrain that's not entirely true. While MRI may not emit a warning, it is still wrong to do this.

http://www.ruby-doc.org/stdlib-1.9.3/libdoc/tempfile/rdoc/Tempfile.html#method-i-unlink
"One should always unlink the file after using it, as is explained in the “Explicit close” good practice section in the Tempfile overview"
"unlink-before-close may not be supported on non-POSIX operating systems. Microsoft Windows is the most notable case: unlinking a non-closed file will result in an error, which this method will silently ignore."

So yes you can do it on *nix but not on Windows. Since JRuby is platform-independent it is correct to emit the warning. There are some very specific scenarios the docs mention when you actually want to do this, but I don't think they apply in this case. Also I feel that mechanize is supposed to work on Windows, so I suggest to re-open this issue.

Owner

drbrain commented Jul 26, 2014

How does the scenario where it is acceptable not apply to Mechanize's use of Tempfile?

Owner

drbrain commented Jul 26, 2014

… Note that mechanize works fine on Windows. The Tempfile or StringIO is closed at the appropriate time which will delete the backing file in the case of Tempfile.

If you have an issue with the warning, again, you will need to take it up with JRuby.

mrbrdo commented Jul 26, 2014

Because you don't need this do this if you do not want any other processes to be able to read from or write to the Tempfile, and you do not need to know the Tempfile’s filename either. Deleting a file that is being used is simply impossible on Windows, so you saying it works there is just wrong. If you are trying to unlink the file before you close it, it will not be deleted on Windows, this is a simple fact of how the filesystem works on Windows and is also stated in Ruby docs. Since the warning is being displayed you are obviously trying to do this which is without any doubt invalid on Windows, and that means your code does not work as you intended. Let me state again it is not possible to delete a file that is in use on Windows. You think it works because you did not read the docs fully, i.e. Microsoft Windows is the most notable case: unlinking a non-closed file will result in an error, which this method will silently ignore. In other words you are not deleting the tempfiles on Windows.

I had this warning before from my own code, it can be very easily fixed. The problem is in mechanize's code not in JRuby.

TL;DR you're doing it wrong.

Owner

drbrain commented Jul 26, 2014

I do not want other processes to be able to read or write from the Tempfile as that exposes a security hole where other processes can inject contents into a download and it does not allow automatic cleanup if ruby crashes or mechanize is terminated abnormally.

I do not need to know the Tempfile's filename, it's just a place to dump download contents that does not consume process memory. (See agent.rb.)

When I say that it works I mean that mechanize can use the Tempfile regardless of platform. I don't care if CRuby on Windows or JRuby do not support unlinking as the error is ignored. Previously this did not work on JRuby so I filed JRUBY-6477 which resulted in the present behavior.

With CRuby on Windows this warning is not displayed.

With JRuby on all platforms the warning is displayed.

The downside of behavior on CRuby on Windows and JRuby is that if the process crashes or is force-killed you may have a very large file in your temporary files directory you need to delete by hand. (I suppose the warning on JRuby alerts you to the need to perform such cleanup upon abnormal exit.) You may also have a security issue where other processes can read or write to the tempfile. You can alter mechanize's configuration to avoid these issues.

If you find that failed unlinking on either platform does not allow the opening process to read or write to the file as it did in the past that is a regression you should report to the respective platform.

mrbrdo commented Jul 27, 2014

What I am saying is that what you're doing does not work on Windows and temp files are not removed. Same on JRuby.
In other words what you're doing only works on MRI on *nix and nowhere else.

Basically, you are getting the warning because you are doing something platform-specific, and JRuby (and Windows, for that matter) does not support that. So it's not JRuby that is wrong but you are. Except for a specific combination of OS and interpreter, of course. The temp files are not even unlinked after they are not needed anymore (unless I missed it in code).

Owner

drbrain commented Jul 27, 2014

CRuby Windows supports #unlink by ignoring the attempt to unlink. JRuby does the same, if it displays a warning that is not my concern, you need to take it up with JRuby.

There are hundreds of platforms that support unlinking an open file descriptor, all UNIX-based. If two don't, Windows and Java, that's fine with me.

The tempfile is closed here for Content-Encoding: gzip or here for Content-Encoding: inflate or here for generic Content-Encoding or here when a URL is downloaded. Otherwise it is held open until the Mechanize::Page is garbage collected and the finalizer runs (here) as the page content needs to be accessible for the lifetime of the Page object.

Can you show how #unlink creates any problem besides the warning on JRuby? When I wrote this code I was not able to find one, nor upon review am I able to now. A problem would be leaving a file named "mechanize-raw-*" in your temporary files directory, or having mechanize download files existing in your temporary files directory after mechanize terminates either normally or from a ruby exception.

The warning for JRuby is not a problem. "mechanize-raw-*" files in the temporary files directory on platforms that do not support unlinking of an open file handle while the object referencing it is alive is not a problem.

mrbrdo commented Jul 27, 2014

Well, I don't agree, I think the warning is a useful reminder (in case you assume the file is actually unlinked). Would you accept a PR with a conditional on RUBY_PLATFORM?

Owner

drbrain commented Jul 28, 2014

If the JVM gains the ability to stat file descriptors that have been unlinked the warning will disappear but mechanize will not be able to take advantage of the updated behavior. This means an extra maintenance burden on mechanize for a single platform. I don't see the benefit.

mrbrdo commented Jul 28, 2014

Sad to hear that. Also I understand that you don't see the benefit, since you don't have to stare at that fucking warning every day of your life. So I guess it's a perfectly understandable decision. Thanks for your time.

I'm with mrbrdo on this. With the warning being thrown is causing a lot of my build verification tests which are driven by a JRuby automation framework to be marked as "error" via Octopus Deploy and putting all builds into a warning state. Not handling this error properly for Windows environments will cause me to have to refactor much of BVT code and drop mechanize as a web driver. I urge you to reconsider.

Owner

drbrain commented Sep 29, 2014

@dturnergs you'll need to open an issue with JRuby. If JRuby deviates from CRuby then it is CRuby's issue to fix.

On CRuby mechanize works just fine without issue. JRuby issues this warning on all platforms while CRuby silently works (whether or not the file can be unlinked) on all platforms.

@drbrain So what's the ramifications if I simply comment out line 1216 in agent.rb? If io.unlink doesn't occur everything is peachy on my end. It's not happening anyway on Windows in JRuby so I couldn't imagine the impact being any worse than it is now except I won't get the warning anymore.

Just as an update for everyone on the thread, I see where @drbrain is coming from on the mention of this being a JRuby issue specifically. According to the doc the best practice is to unlink after create on POSIX systems however Windows will cause a warning to be thrown as we've seen. However, Ruby is supposed to silently suppress the warning. I'll check to see if the issue is known with JRuby and if not report it.

Unlink-before-close

On POSIX systems it’s possible to unlink a file before closing it. This practice is explained in detail in the Tempfile overview (section “Unlink after creation”); please refer there for more information.

However, unlink-before-close may not be supported on non-POSIX operating systems. Microsoft Windows is the most notable case: unlinking a non-closed file will result in an error, which this method will silently ignore. If you want to practice unlink-before-close whenever possible, then you should write code like this...
http://www.ruby-doc.org/stdlib-1.9.3/libdoc/tempfile/rdoc/Tempfile.html#method-i-unlink

stan3 commented Sep 29, 2014

@dturnergs I've raised jruby/jruby#1752 with jruby

Thanks @stan3

Owner

drbrain commented Sep 29, 2014

If you comment out the unlink and mechanize crashes then, on CRuby on POSIX platforms, you will have left-over temporary files of arbitrary size you need to clean up by hand. On CRuby on Windows you will have no difference in behavior when commenting the line out. On JRuby you will have the same behavior as CRuby on Windows with a (scary but IMO pointless) warning.

On JRuby on all platforms and CRuby on Windows the unlink does nothing. On JRuby it issues a warning, though.

PS: I don't see why mechanize should have conditional code for JRuby which is why I'm pushing this off to JRuby.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment