Feature #7148

Improved Tempfile w/o DelegateClass

Added by Masaki Matsushita almost 3 years ago. Updated about 1 month ago.

[ruby-core:47930]
Status:Assigned
Priority:Normal
Assignee:Masaki Matsushita

Description

I propose improved Tempfile without DelegateClass().
Present Tempfile has following problems.

  1. confusing inspect

    t = Tempfile.new("foo") #=> #<File:/tmp/foo20121012-6762-12w11to>
    t.is_a? File #=> false
    
  2. #dup doesn't duplicate IO

    t = Tempfile.new("foo")
    t.dup.close
    t.read #=> IOError: closed stream
    
  3. finalizer performs unlink even when it has been duplicated

    t = Tempfile.new("foo")
    path = t.path #=> "/tmp/foo20121012-7533-1q537gq"
    File.exist? path #=> true
    tt = t.dup
    t = nil
    GC.start
    File.exist? path #=> false
    

I think these problems caused by using DelegateClass().
Therefore, I made a patch to resolve the problems.
The patched Tempfile class is a subclass of File.

patch.diff Magnifier (3.52 KB) Masaki Matsushita, 10/12/2012 02:04 PM

History

#1 Updated by Anonymous almost 3 years ago

Hi,

In message "Re: [ruby-trunk - Feature #7148][Open] Improved Tempfile w/o DelegateClass"
on Fri, 12 Oct 2012 14:04:08 +0900, "Glass_saga (Masaki Matsushita)" glass.saga@gmail.com writes:

I propose improved Tempfile without DelegateClass().
Present Tempfile has following problems.

  1. confusing inspect
  2. #dup doesn't duplicate IO
  3. finalizer performs unlink even when it has been duplicated

I have no objection about (1), but what we expect when we call
Tempfile#dup might differ, for example, I'd expect it to copy the
underlying temporary file. So making Tempfile a subclass of File
may not be the ideal solution.

                        matz.

#2 Updated by Masaki Matsushita almost 3 years ago

Hello,

Yukihiro Matsumoto wrote:

I'd expect it to copy the underlying temporary file.

Is the behavior of #dup you expect like the following?

def dup
  dupe = self.class.new(@basename)
  IO.copy_stream(self, dupe, 0)
  dupe
end

I think the reason why Tempfile uses DelegateClass is that to implement Tempfile#open without it used to be difficult.
To implement it as subclass of File, self must be reopened with full configuration, mode and opts.
IO#reopen used not to accept them, but now it accepts after r37071.

#3 Updated by Charles Nutter almost 3 years ago

JRuby has been running with Tempfile < File for a couple years now, and have received only minor bug reports about it. It works very well, and has no delegation cost.

#4 Updated by Masaki Matsushita over 2 years ago

Are there some reasons not to make Tempfile a subclass of File?
I think it's a better solution, even if it's not an ideal solution.

#5 Updated by Yusuke Endoh over 2 years ago

  • Status changed from Open to Assigned
  • Target version set to next minor
  • Priority changed from Normal to 3
  • Assignee set to Yukihiro Matsumoto

#6 Updated by Eric Wong 2 months ago

"Glass_saga (Masaki Matsushita)" glass.saga@gmail.com wrote:

Feature #7148: Improved Tempfile w/o DelegateClass
https://bugs.ruby-lang.org/issues/7148

I would still like this for 2.3.0, just hit a snag with IO.copy_stream
using Tempfile :x

Also, Charles hit a similar problem not long ago, too:
[Bug #11015]

#7 Updated by Masaki Matsushita 2 months ago

I think the problem you faced was resolved at r50118, wasn't it?

#8 Updated by Eric Wong 2 months ago

Sadly, r50118 actually caused my problem because I was using src_offset
for IO.copy_stream

#9 Updated by Charles Nutter 2 months ago

It doesn't sound like anyone opposes this idea, so are we just missing implementation?

#10 Updated by Nobuyoshi Nakada about 2 months ago

  • Description updated (diff)

#11 Updated by Yukihiro Matsumoto about 2 months ago

It's OK for me to implement Tempfile without using DelegateClass. If JRuby does similar thing already, it might be a good idea to share implementation. Besides that, we might need to think about killing/redefining some unnecessary/invalid methods of File class, e.g. reopen. I am afraid dup is one of them.

Matz.

#12 Updated by Masaki Matsushita about 1 month ago

  • Assignee changed from Yukihiro Matsumoto to Masaki Matsushita

Also available in: Atom PDF