Tempfile#unlink changes in Ruby 1.9.1-p152

Tempfile#unlink changes in Ruby 1.9.1-p152

[Niels Ganser]

[-- Attachment #1.1: Type: text/plain, Size: 1201 bytes --]

Hey everyone,

after updating my Ruby installation recently, I constantly ran into IOErrors
when using rack. After a bit of digging around I could narrow this down to
RewindableInput#make_rewindable, more specifically
lib/rack/rewindable_input.rb:78. After unlinking the tempfile, the IO object
was closed and thus e.g. the @rewindable_io.rewind in line 93 wasn't
possible any more.

I could trace this back to a change in Ruby introduced in revisions 23494
(trunk) and 23537 (branches/ruby_1_9_1). The relevant changes are reproduced
in ruby-core[1] and redmine[2]. As you can see, Tempfile#unlink now calls
#close before actually unlinking the file.

An obvious workaround is attached but surely the whole "if
filesystem_has_posix_semantics?" block should be reworked or this should be
followed up with the ruby core guys. While I'm not exactly sure whether we
can consider closing the IO stream to a file before unlinking it a bug in
itself, Tempfile#unlink now sure is inconsistent with File#unlink. Principle
of least surprise? No, Sir!

What do you guys think?

Best,
Niels

[1] http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/23505
[2] http://redmine.ruby-lang.org/issues/show/1494

[-- Attachment #1.2: Type: text/html, Size: 1408 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 191-152-unlink-workaround.patch --]
[-- Type: text/x-diff, Size: 1093 bytes --]

From d960563139192baf39a380055fce85ed5ecde465 Mon Sep 17 00:00:00 2001
From: Niels Ganser <niels@herimedia.com>
Date: Sat, 18 Jul 2009 21:14:21 +0200
Subject: [PATCH] Workaround for Tempfile#unlink change in 1.9.1-p152

Starting with patchlevel 152, Ruby 1.9 closes a Tempfile
upon unlinking it. In such cases we would run into IOErrors
(closed stream) when trying to keep using @rewindable_io
after unlinking it.
---
 lib/rack/rewindable_input.rb |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/rack/rewindable_input.rb b/lib/rack/rewindable_input.rb
index accd96b..b4d1952 100644
--- a/lib/rack/rewindable_input.rb
+++ b/lib/rack/rewindable_input.rb
@@ -74,7 +74,7 @@ module Rack
       @rewindable_io.chmod(0000)
       @rewindable_io.set_encoding(Encoding::BINARY) if @rewindable_io.respond_to?(:set_encoding)
       @rewindable_io.binmode
-      if filesystem_has_posix_semantics?
+      if filesystem_has_posix_semantics? && "#{RUBY_VERSION}.#{RUBY_PATCHLEVEL}" < "1.9.1.152"
         @rewindable_io.unlink
         @unlinked = true
       end
-- 
1.6.0.4

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Eric Wong]

Niels Ganser <niels@herimedia.com> wrote:
> Hey everyone,
> 
> after updating my Ruby installation recently, I constantly ran into IOErrors
> when using rack. After a bit of digging around I could narrow this down to
> RewindableInput#make_rewindable, more specifically
> lib/rack/rewindable_input.rb:78. After unlinking the tempfile, the IO object
> was closed and thus e.g. the @rewindable_io.rewind in line 93 wasn't
> possible any more.
> 
> I could trace this back to a change in Ruby introduced in revisions 23494
> (trunk) and 23537 (branches/ruby_1_9_1). The relevant changes are reproduced
> in ruby-core[1] and redmine[2]. As you can see, Tempfile#unlink now calls
> #close before actually unlinking the file.

A part of me died when I read that.

> An obvious workaround is attached but surely the whole "if
> filesystem_has_posix_semantics?" block should be reworked or this should be
> followed up with the ruby core guys. While I'm not exactly sure whether we
> can consider closing the IO stream to a file before unlinking it a bug in
> itself, Tempfile#unlink now sure is inconsistent with File#unlink. Principle
> of least surprise? No, Sir!
> 
> What do you guys think?

Thanks for the heads up, I just posted my thoughts on Redmine about this.
Hopefully the Ruby team is willing to fix it.

> [1] http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/23505
> [2] http://redmine.ruby-lang.org/issues/show/1494

> diff --git a/lib/rack/rewindable_input.rb b/lib/rack/rewindable_input.rb
> index accd96b..b4d1952 100644
> --- a/lib/rack/rewindable_input.rb
> +++ b/lib/rack/rewindable_input.rb
> @@ -74,7 +74,7 @@ module Rack
>        @rewindable_io.chmod(0000)
>        @rewindable_io.set_encoding(Encoding::BINARY) if @rewindable_io.respond_to?(:set_encoding)
>        @rewindable_io.binmode
> -      if filesystem_has_posix_semantics?
> +      if filesystem_has_posix_semantics? && "#{RUBY_VERSION}.#{RUBY_PATCHLEVEL}" < "1.9.1.152"
>          @rewindable_io.unlink
>          @unlinked = true
>        end

If we have to go this route, what about just explicitly unlinking it?
I'm not going to go as far as undefining the finalizer that Tempfile
defines since the finalizer checks if the file exists before unlinking
anyways.

diff --git a/lib/rack/rewindable_input.rb b/lib/rack/rewindable_input.rb
index accd96b..fcd6d06 100644
--- a/lib/rack/rewindable_input.rb
+++ b/lib/rack/rewindable_input.rb
@@ -75,7 +75,7 @@ module Rack
       @rewindable_io.set_encoding(Encoding::BINARY) if @rewindable_io.respond_to?(:set_encoding)
       @rewindable_io.binmode
       if filesystem_has_posix_semantics?
-        @rewindable_io.unlink
+        File.unlink(@rewindable_io.path)
         @unlinked = true
       end

-- 
Eric Wong

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Niels Ganser]

Eric,

2009/7/19 Eric Wong <normalperson@yhbt.net>
>
> Thanks for the heads up, I just posted my thoughts on Redmine about this.
> Hopefully the Ruby team is willing to fix it.

Thanks for putting this to their attention.

I've updated the issue with links to the original discussion on
ruby-core in 2004. Both the its author and matz decided the patch, as
recently introduced, wouldn't be a good idea:
http://blade.nagaokaut.ac.jp/cgi-bin/vframe.rb/ruby/ruby-core/2848?2697-2915+split-mode-vertical.

> If we have to go this route, what about just explicitly unlinking it?
> I'm not going to go as far as undefining the finalizer that Tempfile
> defines since the finalizer checks if the file exists before unlinking
> anyways.

I don't think any change in the rack codebase will be necessary as I
fully expect this to be fixed in Ruby before the next release.
Considering that many people are even still using 1.8 I don't think
too many compile from trunk or any other svn branch for that matter.
Those who do would probably check the list here when in trouble, no?

Cheers,
Niels

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Hongli Lai]

It seems that this issue still exists in 1.9.1-p243:
http://code.google.com/p/phusion-passenger/issues/detail?id=340

Are they going to fix it?

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Niels Ganser]

Presumably so. I'll wait until the weekend to see if there's activity
on the bug report [1] and, if there isn't any, then bring this to the
attention of ruby-core to see what can be done.

Will report back here.

- Niels.

[1] http://redmine.ruby-lang.org/issues/show/1494

2009/7/22 Hongli Lai <hongli@phusion.nl>:
>
> It seems that this issue still exists in 1.9.1-p243:
> http://code.google.com/p/phusion-passenger/issues/detail?id=340
>
> Are they going to fix it?
>

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Taylor luk]

I have the same problem with Ruby 1.9 + Passenger,

Is there any chance that the workaround provided above will be
included in the coming 1.0.1 release ?


On Jul 23, 12:41 am, Niels Ganser <ni...@herimedia.com> wrote:
> Presumably so. I'll wait until the weekend to see if there's activity
> on the bug report [1] and, if there isn't any, then bring this to the
> attention of ruby-core to see what can be done.
>
> Will report back here.
>
> - Niels.
>
> [1]http://redmine.ruby-lang.org/issues/show/1494
>
> 2009/7/22 Hongli Lai <hon...@phusion.nl>:
>
>
>
> > It seems that this issue still exists in 1.9.1-p243:
> >http://code.google.com/p/phusion-passenger/issues/detail?id=340
>
> > Are they going to fix it?
>
>

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Taylor luk]

Hi guys,

I think this is not getting enough attention as it deserves, The issue
isn't about people using svn trunk release of ruby, But Ruby 1.9.1-
p243 which is the latest stable version of ruby people can download
from http://ruby-lang.org


Passenger issue tracker have marked this issue invalid in their bug
tracker,I have tried patch described in the links above and no
success.

Tempfile class is been patched in Rack. Perphas the upcoming 1.0.1
release, Anyone who can provide some status update or point me to the
right direction ?


Cheers..

Taylor Luk








On Aug 8, 3:49 pm, Taylor luk <subject...@gmail.com> wrote:
> I have the same problem with Ruby 1.9 + Passenger,
>
> Is there any chance that the workaround provided above will be
> included in the coming 1.0.1 release ?
>
> On Jul 23, 12:41 am, Niels Ganser <ni...@herimedia.com> wrote:
>
> > Presumably so. I'll wait until the weekend to see if there's activity
> > on the bug report [1] and, if there isn't any, then bring this to the
> > attention of ruby-core to see what can be done.
>
> > Will report back here.
>
> > - Niels.
>
> > [1]http://redmine.ruby-lang.org/issues/show/1494
>
> > 2009/7/22 Hongli Lai <hon...@phusion.nl>:
>
> > > It seems that this issue still exists in 1.9.1-p243:
> > >http://code.google.com/p/phusion-passenger/issues/detail?id=340
>
> > > Are they going to fix it?
>
>

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Ryan Long]

Niels,

I just wanted to add: after reading that I decided to grab the latest
stable ruby from svn. I'm way over my head with this stuff, but after
successfully installing ruby and attempting to use Passenger, it
continued to fail with this same error. I also cloned the bleeding-
edge rack overwriting my rack gem (I didn't know if that would work or
not, but I know rack is a dependency of Rails so I did'nt know how
else to do it...) with no luck. I did have some trouble with that
version of Ruby and sqlite3-ruby, so I've reverted back to 1.9.1-p243

Ryan

On Jul 18, 6:43 pm, Niels Ganser <ni...@herimedia.com> wrote:
> Eric,
>
> 2009/7/19 Eric Wong <normalper...@yhbt.net>
>
>
>
> > Thanks for the heads up, I just posted my thoughts on Redmine about this.
> > Hopefully the Ruby team is willing to fix it.
>
> Thanks for putting this to their attention.
>
> I've updated the issue with links to the original discussion on
> ruby-core in 2004. Both the its author and matz decided the patch, as
> recently introduced, wouldn't be a good idea:http://blade.nagaokaut.ac.jp/cgi-bin/vframe.rb/ruby/ruby-core/2848?26....
>
> > If we have to go this route, what about just explicitly unlinking it?
> > I'm not going to go as far as undefining the finalizer that Tempfile
> > defines since the finalizer checks if the file exists before unlinking
> > anyways.
>
> I don't think any change in the rack codebase will be necessary as I
> fully expect this to be fixed in Ruby before the next release.
> Considering that many people are even still using 1.8 I don't think
> too many compile from trunk or any other svn branch for that matter.
> Those who do would probably check the list here when in trouble, no?
>
> Cheers,
> Niels

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Hongli Lai]

On Aug 12, 7:13 am, Taylor luk <subject...@gmail.com> wrote:
> Hi guys,
>
> I think this is not getting enough attention as it deserves, The issue
> isn't about people using svn trunk release of ruby, But Ruby 1.9.1-
> p243 which is the latest stable version of ruby people can download
> fromhttp://ruby-lang.org
>
> Passenger issue tracker have marked this issue invalid in their bug
> tracker,I have tried patch described in the links above and no
> success.
>
> Tempfile class is been patched in Rack. Perphas the upcoming 1.0.1
> release, Anyone who can provide some status update or point me to the
> right direction ?

Well I want to have this issue solved, it's just that right now I'm
buried under tons of client work so I haven't had the time yet.

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Hongli Lai]

OK, that's it. I've forked Tempfile:

http://better.rubyforge.org/

Anybody object on making Rack depend on this library?

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Jeremy Kemper]

On Thu, Aug 20, 2009 at 12:05 PM, Hongli Lai<hongli@phusion.nl> wrote:
>
> OK, that's it. I've forked Tempfile:
>
> http://better.rubyforge.org/
>
> Anybody object on making Rack depend on this library?
>

How about heavily advocating for upstream to accept the fix?

Perhaps they didn't get the message. Try another post to ruby-core.

jeremy

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[masayoshi takahashi]

Hi,

2009/8/21 Jeremy Kemper <jeremy@bitsweat.net>:
>> OK, that's it. I've forked Tempfile:
>>
>> http://better.rubyforge.org/
>>
>> Anybody object on making Rack depend on this library?
>>
>
> How about heavily advocating for upstream to accept the fix?
>
> Perhaps they didn't get the message. Try another post to ruby-core.

I agree with Jeremy (I don't understand the problem except Rack has trouble
because of r23494, but Matz also doesn't seem to know the problem.)

Thanks,

Masayoshi Takahashi

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Hongli Lai]

On Aug 21, 2:23 am, masayoshi takahashi <takahash...@gmail.com> wrote:
> Hi,
>
> 2009/8/21 Jeremy Kemper <jer...@bitsweat.net>:
> > How about heavily advocating for upstream to accept the fix?
> > Perhaps they didn't get the message. Try another post to ruby-core.
>
> I agree with Jeremy (I don't understand the problem except Rack has trouble
> because of r23494, but Matz also doesn't seem to know the problem.)

OK, I posted a follow-up: http://redmine.ruby-lang.org/issues/show/1494#note-10

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Hongli Lai]

I've waited a few days now but the developers don't seem terribly
responsive.

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Hongli Lai]

They've reverted the change: http://redmine.ruby-lang.org/repositories/revision/ruby-19?rev=24662
This issue will be solved in the next patchlevel release of Ruby 1.9.1.

Re: Tempfile#unlink changes in Ruby 1.9.1-p152

[Niels]

On Aug 26, 10:10 am, Hongli Lai <hon...@phusion.nl> wrote:
> They've reverted the change:http://redmine.ruby-lang.org/repositories/revision/ruby-19?rev=24662
> This issue will be solved in the next patchlevel release of Ruby 1.9.1.

Ah, very good.

Thanks for seeing this through! And sorry that I didn't follow up on
this myself, I completely forgot about it.

- Niels