Tempfile#unlink changes in Ruby 1.9.1-p152

[Niels Ganser]

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

[Eric Wong]

Niels Ganser <ni...@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

[Niels Ganser]

Eric,

2009/7/19 Eric Wong <normal...@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

[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?

[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 <hon...@phusion.nl>:

[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 ?

[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

[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....

[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.

[Hongli Lai]

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

http://better.rubyforge.org/

Anybody object on making Rack depend on this library?

[Jeremy Kemper]

How about heavily advocating for upstream to accept the fix?

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

jeremy

[masayoshi takahashi]

Hi,

2009/8/21 Jeremy Kemper <jer...@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

[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

[Hongli Lai]

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

[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.

[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