Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tbhi opened this Issue Jun 18, 2014 · 8 comments

Comments

Projects
None yet
5 participants
@drbrain

This comment has been minimized.

Copy link

drbrain commented Jul 26, 2014

This can cause confusion by users, see:

sparklemotion/mechanize#371

@mrbrdo

This comment has been minimized.

Copy link
Contributor

mrbrdo commented Jul 27, 2014

@stan3 per http://www.ruby-doc.org/stdlib-1.9.3/libdoc/tempfile/rdoc/Tempfile.html it is only acceptable on POSIX systems. Because the JVM abstracts away such OS-specific features, JRuby is (in my opinion) correct to always emit this warning, since it actually cannot unlink the file. In my opinion, MRI should also be emitting such a warning on Windows, where this is not possible.
The reason MRI probably does not do it is because such a huge number of its users are on POSIX systems and they don't care so much about Windows (not that I do).

In my opinion the only acceptable solution would be to actually unlink the file (possibly via spawn or JNI if not otherwise possible) on POSIX systems and not emit this warning in that case. Although I do not know if JVM has any problems reading/writing a file that no longer exists.
On Windows I think JRuby is more right than MRI to emit this warning.

EDIT: However to be fair the Tempfile docs say 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. so one could interpret the silently as in no warning. Personally I like the warning since it is useful in fixing broken programs.

@mrbrdo

This comment has been minimized.

Copy link
Contributor

mrbrdo commented Sep 30, 2014

@headius any idea why JRuby emits this warning even though the Ruby docs say it will be silently ignored? Even though it might be useful it is a deviation from MRI and specs.

@drbrain

This comment has been minimized.

Copy link

drbrain commented Sep 30, 2014

Long ago I tried to stat() on a Tempfile I had called #unlink on with JRuby. (For background, on CRuby this works on both POSIX and Windows as CRuby has the file descriptor or has not unlinked the file, respectively.) For JRuby this failed as the stat system call used required the filename on JRuby (lstat). It could not use the file descriptor call (fstat) as the JVM did not provide access to it.

In order to fix this bug, on JRuby, Tempfile was changed to never unlink the file. Since this matched behavior on Windows it seemed like the least intrusive fix. At the same time the warning was added as the JRuby behavior differed from CRuby behavior.

I asked @headius about this on twitter, here is his reply:

https://twitter.com/headius/status/516633921694744577

@jeremyhaile

This comment has been minimized.

Copy link

jeremyhaile commented Dec 8, 2014

I like @headius' suggestion of making it a verbose mode warning! This warning can be really annoying when it's intended behavior.

@mrbrdo

This comment has been minimized.

Copy link
Contributor

mrbrdo commented Dec 8, 2014

yes that would be cool, @headius seems like a one-liner change right?
initially in this issue I was opposed to this but seeing the official docs of #unlink it seems like the right thing to do

@headius

This comment has been minimized.

Copy link
Member

headius commented Dec 8, 2014

I will make the warning verbose for 1.7.17. For 9k it will usually actually unlink the file and do real fstat against file descriptors, so it's moot when native support is available.

headius added a commit that referenced this issue Dec 8, 2014

@headius headius closed this Dec 8, 2014

@headius headius added this to the JRuby 1.7.17 milestone Dec 8, 2014

@headius headius self-assigned this Dec 8, 2014

@mrbrdo

This comment has been minimized.

Copy link
Contributor

mrbrdo commented Dec 9, 2014

@headius awesome! Really appreciate all this work you are putting in.
Btw since you mention 9k, could you update https://github.com/jruby/jruby/wiki/Roadmap it still says it's planned to release summer 2014. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.