Bug #1494: tempfile#unlink may silently fail on windows

• Status: Third Party's Issue
• Assignee: -
• Target version: 1.9.1
• ruby -v: ruby 1.8.6 (2007-09-24 patchlevel 111) [i386-mswin32]

[modethree3 (Nicholas Manning)]

=begin
There is an unlink method that was causing an exception when running it on my windows machine. The method will call the 'rescue' block but then will do nothing. This is problematic when things like RubyInline uses this method to create temp files and then delete them on windows.

Suggested patch (from http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/2848) (thanks to Florian Frank who wrote it).

--- lib/tempfile.rb 23 Jul 2003 16:37:35 -0000 1.19
+++ lib/tempfile.rb 5 May 2004 23:33:57 -0000
@@ -106,7 +106,10 @@ class Tempfile < SimpleDelegator
# file.
def unlink
# keep this order for thread safeness

• File.unlink(@tmpname) if File.exist?(@tmpname)

• if File.exist?(@tmpname)

•  closed? or close
  

•  File.unlink(@tmpname)
  

• end
  @@cleanlist.delete(@tmpname) if @@cleanlist
  end
  alias delete unlink
  =end

[normalperson (Eric Wong)]

=begin
This patch totally breaks UNIX applications that rely on this behavior. It's
widely considered good practice to unlink temp files ASAP on platforms
that support it for several reasons:

1. reduced chance of conflicts/retries when other temp files are created

2. less chance a fatal error in the application causing disk space to
   be filled up because finalizers didn't get run

3. improved security in case an accidental chmod hits the file, or
   the user is running another misbehaving application.

=end

[niels (Niels Ganser)]

=begin
In fact, back in 2004, only seconds after posting his patch, Florian already "came to the conclusion that this patch sucks": http://blade.nagaokaut.ac.jp/cgi-bin/vframe.rb/ruby/ruby-core/2849?2697-2915+split-mode-vertical

Matz chimed in shortly thereafter and consensus seemed to be to simply rescue (and discard) EACCESS as the file will be garbage collected after unlinking anyways: http://blade.nagaokaut.ac.jp/cgi-bin/vframe.rb/ruby/ruby-core/2850?2697-2915+split-mode-vertical

As Eric noted above, the current implementation goes contrary to what many UNIX programmers have learned for years.

=end

[bitsweat (Jeremy Daer)]

=begin
Please revert this change. It's causing major issues for everyone BUT windows.

http://groups.google.com/group/rack-devel/browse_thread/thread/a2aab3a4720f34c4

Please restore the previous, sane behavior and come up a different, non-regressive fix for the original concern.

It's bad enough, and there's no response to the issue, to fork the tempfile stdlib: http://github.com/FooBarWidget/better/blob/master/lib/better/tempfile.rb#L28
=end

[luislavena (Luis Lavena)]

=begin
On Thu, Aug 20, 2009 at 6:24 PM, Jeremy [e-mail] wrote:

Issue #1494 has been updated by Jeremy Kemper.

Please revert this change. It's causing major issues for everyone BUT windows.

http://groups.google.com/group/rack-devel/browse_thread/thread/a2aab3a4720f34c4

Please restore the previous, sane behavior and come up a different, non-regressive fix for the original concern.

It's bad enough, and there's no response to the issue, to fork the tempfile stdlib: http://github.com/FooBarWidget/better/blob/master/lib/better/tempfile.rb#L28

Sorry to chime in, but why when it rescue the Errno::EACCES, it
doesn't close and then retry? Instead of just silently fail.

http://github.com/FooBarWidget/better/blob/master/lib/better/tempfile.rb#L336

If the error is known, and the workaround is also known, why we should
keep rescuing this in our when when the library is just plainly
ignoring it?

--
Luis Lavena
AREA 17

Perfection in design is achieved not when there is nothing more to add,
but rather when there is nothing more to take away.
Antoine de Saint-Exupéry

=end

[shyouhei (Shyouhei Urabe)]

• Category set to lib
• Assignee set to matz (Yukihiro Matsumoto)
• Priority changed from 3 to 5
• Target version set to 1.9.1

=begin
Seems it's (currently) 1.9.x specific so I moved this issue to 1.9 category.

I've also assigned it to matz because revision r23494 was checed in by him.

Matz, can I revert this? Or do you want to do so?
=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Fri, 21 Aug 2009 07:36:27 +0900,
Shyouhei Urabe wrote in [ruby-core:25008]:

Seems it's (currently) 1.9.x specific so I moved this issue to 1.9 category.

I've also assigned it to matz because revision r23494 was checed in by him.

Matz, can I revert this? Or do you want to do so?

I'd vote for Luis's suggestion.

--
Nobu Nakada

=end

[matz (Yukihiro Matsumoto)]

=begin
Hi,

In message "Re: [ruby-core:25011] Re: [Bug #1494] tempfile#unlink may silently fail on windows"
on Fri, 21 Aug 2009 08:37:35 +0900, Nobuyoshi Nakada [e-mail] writes:

|At Fri, 21 Aug 2009 07:36:27 +0900,
|Shyouhei Urabe wrote in [ruby-core:25008]:
|> Seems it's (currently) 1.9.x specific so I moved this issue to 1.9 category.
|>
|> I've also assigned it to matz because revision r23494 was checed in by him.
|>
|> Matz, can I revert this? Or do you want to do so?
|
|I'd vote for Luis's suggestion.

I am not sure what he exactly wants, and what would happen on Windows.
But if you think it's OK, please check in the fix.

						matz.

=end

[usa (Usaku NAKAMURA)]

=begin
To begin with, What situation did the original post encount?
I doubt that they tried to check the existence of the file after unlink.

About Tempfile, it is nonsense to check the existence.
There are another proper ways only if they wants the path name to check whether there is a directory-entry on the filesystem or not.

In conclusion, I recommend to revert the patch.

=end

[luislavena (Luis Lavena)]

=begin
On Thu, Aug 20, 2009 at 8:52 PM, Yukihiro [e-mail] wrote:

Hi,

In message "Re: [ruby-core:25011] Re: [Bug #1494] tempfile#unlink may silently fail on windows"
   on Fri, 21 Aug 2009 08:37:35 +0900, Nobuyoshi Nakada [e-mail] writes:

|At Fri, 21 Aug 2009 07:36:27 +0900,
|Shyouhei Urabe wrote in [ruby-core:25008]:
|>
|> Matz, can I revert this?  Or do you want to do so?
|
|I'd vote for Luis's suggestion.

I am not sure what he exactly wants, and what would happen on Windows.
But if you think it's OK, please check in the fix.

Hello Matz,

Basically as Jeremy Kemper pointed, the current 1.9.x implementation is flawed.

On this thread:
http://groups.google.com/group/rack-devel/browse_thread/thread/a2aab3a4720f34c4

A workaround is use "better" external dependency to override Ruby's
one, which has been mentioned in the above thread and it's on GitHub:

http://github.com/FooBarWidget/better/blob/master/lib/better/tempfile.rb#L28

More precisely, the approach shown here:

http://github.com/FooBarWidget/better/blob/master/lib/better/tempfile.rb#L328-340

Seems to rescue Errno::EACCES on Windows, and silently fail.

On our scenario (Windows), that will leave open descriptors.

My suggestion was: if we are now taking the time to rescue that
exception why we don't actually solve the issue?

 rescue Errno::EACCES
   close
   retry
 end

In theory that will reduce lot of Errno:EACCES and platform specific
conditions in end-users scripts.

The only thing under this could fail is if another process has locked
that file too.

--
Luis Lavena
AREA 17

Perfection in design is achieved not when there is nothing more to add,
but rather when there is nothing more to take away.
Antoine de Saint-Exupéry

=end

[hongli (Hongli Lai)]

=begin
Hi guys. Let me re-explain the issue:

There are times when you just need a large anonymous disk-backed byte buffer. For example, when you're writing a web server and you want to buffer the client's upload data. In cases like this, the filename of the buffer temp file you're using doesn't matter, it just matters that you have a file handle open; you're not going to pass the filename to another process or anything.

POSIX systems allow you to do just this. You create a file, unlink it from the filesystem, but keep the file handle open. The result is a file handle that only you can access - no other processes can access it because there's no filesystem entry anymore. This doesn't work on Microsoft Windows however, because on Windows one may only unlink a file if nobody has it open.

Portable applications that need a disk-backed byte buffer will want to do something like this on POSIX:

1. Create the file.
2. Unlink it. This will always succeed.
3. Use the file handle to do things.
4. When done, close the file handle.

On Windows on the other hand they'd want to do this instead:

1. Create the file.
2. Use the file handle to do things.
3. When done, close the file handle.
4. Unlink the file.

Tempfile can support this in a portable manner by recommending applications to use it as follows:

1. Create the file.
2. Unlink it if possible (i.e. only on POSIX systems).
3. Use the file handle to do things. It doesn't matter whether the previous unlink action succeeded, but the file handle must remain open because the app is going to do some work with it.
4. When done, close the file handle.
5. If the unlink action in #2 failed, then unlink it now.

Making the #unlink call close the file handle is not acceptable; it will totally destroy step #3. When calling #unlink, the programmer does not want to close it; it just wants the filesystem entry to disappear, if platform allows it, but the file handle must remain open.

Part of the problem is that not everybody knows this trick or that POSIX and Windows behave differently. The current Tempfile documentation documents this very sparsely.

My version of Tempfile:

• Supports the aforementioned use case: http://github.com/FooBarWidget/better/blob/master/lib/better/tempfile.rb#L307-327
• Has extensive documentation which, among other things, explains the unlink-after-closing trick and all the cross-platform caveats that come with it.
  http://github.com/FooBarWidget/better/blob/master/lib/better/tempfile.rb#L307-327 and
  http://github.com/FooBarWidget/better/blob/master/lib/better/tempfile.rb#L76-91
• Has extensive documentation for everything else as well.
• Fixes a few bugs. These are explained in the 'Comparison' section in the overview documentation of the class.
• Has many unit tests. Ruby's Tempfile currently has almost no unit tests.

Please consider accepting it upstream.
=end

[hongli (Hongli Lai)]

=begin
Luis Lavena:

Seems to rescue Errno::EACCES on Windows, and silently fail.

Actually, Ruby 1.9's Tempfile already does that. I just removed the 'close' call in the #unlink method. I'd rather advocate letting the exception propagate and explicitly documenting the exception in the API documentation, but I want to preserve Tempfile's current behavior as much as possible so I preserved that code and introduced #unlinked? instead.
=end

[hongli (Hongli Lai)]

=begin
Any updates on this?
=end

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Third Party's Issue
• Assignee deleted (matz (Yukihiro Matsumoto))

=begin
RubyInline's issue.
=end

[naruse (Yui NARUSE)]

=begin
The revert commit is r24662.
Yugui will backport this to 1.9.1.

If you have some enhancement request, create new ticket per issue and attach patches for ruby's trunk.
=end

[hongli (Hongli Lai)]

=begin
Great, thanks for reverting.

I've opened an enhancement request for my Tempfile improvements: http://redmine.ruby-lang.org/issues/show/1999
=end