Project

General

Profile

Actions

Bug #1494

closed

tempfile#unlink may silently fail on windows

Added by modethree3 (Nicholas Manning) almost 12 years ago. Updated over 8 years ago.

Status:
Third Party's Issue
Priority:
Normal
Assignee:
-
Target version:
ruby -v:
ruby 1.8.6 (2007-09-24 patchlevel 111) [i386-mswin32]
Backport:
[ruby-core:23505]

Description

=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
Actions #1

Updated by normalperson (Eric Wong) almost 12 years ago

=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

Actions #2

Updated by niels (Niels Ganser) almost 12 years ago

=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

Actions #3

Updated by bitsweat (Jeremy Daer) over 11 years ago

=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

Actions #4

Updated by luislavena (Luis Lavena) over 11 years ago

=begin
On Thu, Aug 20, 2009 at 6:24 PM, Jeremy Kemperredmine@ruby-lang.org 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

Actions #5

Updated by shyouhei (Shyouhei Urabe) over 11 years ago

  • 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

Actions #6

Updated by nobu (Nobuyoshi Nakada) over 11 years ago

=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

Actions #7

Updated by matz (Yukihiro Matsumoto) over 11 years ago

=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 nobu@ruby-lang.org 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

Actions #8

Updated by usa (Usaku NAKAMURA) over 11 years ago

=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

Actions #9

Updated by luislavena (Luis Lavena) over 11 years ago

=begin
On Thu, Aug 20, 2009 at 8:52 PM, Yukihiro Matsumotomatz@ruby-lang.org 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 nobu@ruby-lang.org 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

Actions #10

Updated by hongli (Hongli Lai) over 11 years ago

=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:

Please consider accepting it upstream.
=end

Actions #11

Updated by hongli (Hongli Lai) over 11 years ago

=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

Actions #12

Updated by hongli (Hongli Lai) over 11 years ago

=begin
Any updates on this?
=end

Actions #13

Updated by nobu (Nobuyoshi Nakada) over 11 years ago

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

=begin
RubyInline's issue.
=end

Actions #14

Updated by naruse (Yui NARUSE) over 11 years ago

=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

Actions #15

Updated by hongli (Hongli Lai) over 11 years ago

=begin
Great, thanks for reverting.

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

Actions

Also available in: Atom PDF