Project

General

Profile

Feature #7148

Improved Tempfile w/o DelegateClass

Added by Glass_saga (Masaki Matsushita) over 6 years ago. Updated 3 months ago.

Status:
Assigned
Priority:
Normal
Target version:
-
[ruby-core:47930]

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.


Files

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

History

Updated by Anonymous over 6 years ago

Hi,

In message "Re: [ruby-core:47930] [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.

Updated by Glass_saga (Masaki Matsushita) over 6 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.

Updated by headius (Charles Nutter) over 6 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.

Updated by Glass_saga (Masaki Matsushita) over 6 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.

Updated by mame (Yusuke Endoh) over 6 years ago

  • Status changed from Open to Assigned
  • Assignee set to matz (Yukihiro Matsumoto)
  • Priority changed from Normal to 3
  • Target version set to 2.6

Updated by normalperson (Eric Wong) about 4 years 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:
[ruby-core:68700] [Bug #11015]

Updated by Glass_saga (Masaki Matsushita) about 4 years ago

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

Updated by normalperson (Eric Wong) about 4 years ago

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

Updated by headius (Charles Nutter) about 4 years ago

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

Updated by nobu (Nobuyoshi Nakada) about 4 years ago

  • Description updated (diff)

Updated by matz (Yukihiro Matsumoto) about 4 years 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.

Updated by Glass_saga (Masaki Matsushita) about 4 years ago

  • Assignee changed from matz (Yukihiro Matsumoto) to Glass_saga (Masaki Matsushita)

Updated by sowieso (So Wieso) 3 months ago

I guess this is related:

tf = Tempfile.new
tf.unlink
tf2 = tf.dup
puts tf.fileno
# => 10
puts tf2.fileno
# => 11
tf2.close

print tf2.fileno
# => 11
print tf.fileno
IOError: closed stream
from /usr/local/rvm/rubies/ruby-2.6.0/lib/ruby/2.6.0/delegate.rb:349:in `fileno'

It closes the wrong fd. Using normal File instead of Tmpfile makes it work like usual.

Libraries that accept an IO object and duplicate it internally don't work correctly with Tempfile (in my case rubyzip). This really needs a fix.

Also available in: Atom PDF