Skip to content

Commit 753ab16

Browse files
committed
Add FinalizerManager to manage finalizers
As @jeremyevans pointed out for commit eb2d8b1: > Each Tempfile instance has a separate File instance and file descriptor: > > t = Tempfile.new > t.to_i # => 6 > t.dup.to_i => 7 FinalizerManager will keep track of the open File objects for the particular file and will only unlink the file when all of the File objects have been closed.
1 parent 37bee10 commit 753ab16

File tree

2 files changed

+58
-35
lines changed

2 files changed

+58
-35
lines changed

lib/tempfile.rb

+24-34
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,6 @@ def initialize(basename="", tmpdir=nil, mode: 0, **options)
221221

222222
@unlinked = false
223223
@mode = mode|File::RDWR|File::CREAT|File::EXCL
224-
@finalizer_obj = Object.new
225224
tmpfile = nil
226225
::Dir::Tmpname.create(basename, tmpdir, **options) do |tmpname, n, opts|
227226
opts[:perm] = 0600
@@ -231,29 +230,27 @@ def initialize(basename="", tmpdir=nil, mode: 0, **options)
231230

232231
super(tmpfile)
233232

234-
define_finalizers
235-
end
236-
237-
private def define_finalizers
238-
ObjectSpace.define_finalizer(@finalizer_obj, Closer.new(__getobj__))
239-
ObjectSpace.define_finalizer(@finalizer_obj, Remover.new(__getobj__.path))
233+
@finalizer_manager = FinalizerManager.new(__getobj__.path)
234+
@finalizer_manager.register(self, __getobj__)
240235
end
241236

242237
def initialize_dup(other) # :nodoc:
243238
initialize_copy_iv(other)
244239
super(other)
240+
@finalizer_manager.register(self, __getobj__)
245241
end
246242

247243
def initialize_clone(other) # :nodoc:
248244
initialize_copy_iv(other)
249245
super(other)
246+
@finalizer_manager.register(self, __getobj__)
250247
end
251248

252249
private def initialize_copy_iv(other) # :nodoc:
253250
@unlinked = other.unlinked
254251
@mode = other.mode
255252
@opts = other.opts
256-
@finalizer_obj = other.finalizer_obj
253+
@finalizer_manager = other.finalizer_manager
257254
end
258255

259256
# Opens or reopens the file with mode "r+".
@@ -263,8 +260,7 @@ def open
263260
mode = @mode & ~(File::CREAT|File::EXCL)
264261
__setobj__(File.open(__getobj__.path, mode, **@opts))
265262

266-
ObjectSpace.undefine_finalizer(@finalizer_obj)
267-
define_finalizers
263+
@finalizer_manager.register(self, __getobj__)
268264

269265
__getobj__
270266
end
@@ -334,9 +330,6 @@ def unlink
334330
return
335331
end
336332

337-
ObjectSpace.undefine_finalizer(@finalizer_obj)
338-
ObjectSpace.define_finalizer(@finalizer_obj, Closer.new(__getobj__))
339-
340333
@unlinked = true
341334
end
342335
alias delete unlink
@@ -370,35 +363,32 @@ def inspect
370363

371364
protected
372365

373-
attr_reader :unlinked, :mode, :opts, :finalizer_obj
374-
375-
class Closer # :nodoc:
376-
def initialize(tmpfile)
377-
@tmpfile = tmpfile
378-
end
379-
380-
def call(*args)
381-
@tmpfile.close
382-
end
383-
end
366+
attr_reader :unlinked, :mode, :opts, :finalizer_manager
384367

385-
class Remover # :nodoc:
368+
class FinalizerManager # :nodoc:
386369
def initialize(path)
387-
@pid = Process.pid
370+
@open_files = {}
388371
@path = path
372+
@pid = Process.pid
389373
end
390374

391-
def call(*args)
392-
return if @pid != Process.pid
375+
def register(obj, file)
376+
ObjectSpace.undefine_finalizer(obj)
377+
ObjectSpace.define_finalizer(obj, self)
378+
@open_files[obj.object_id] = file
379+
end
393380

394-
$stderr.puts "removing #{@path}..." if $DEBUG
381+
def call(object_id)
382+
@open_files.delete(object_id).close
395383

396-
begin
397-
File.unlink(@path)
398-
rescue Errno::ENOENT
384+
if @open_files.empty? && Process.pid == @pid
385+
$stderr.puts "removing #{@path}..." if $DEBUG
386+
begin
387+
File.unlink(@path)
388+
rescue Errno::ENOENT
389+
end
390+
$stderr.puts "done" if $DEBUG
399391
end
400-
401-
$stderr.puts "done" if $DEBUG
402392
end
403393
end
404394

test/test_tempfile.rb

+34-1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
require 'tempfile'
44

55
class TestTempfile < Test::Unit::TestCase
6+
LIB_TEMPFILE_RB_PATH = File.expand_path(__dir__ + "/../lib/tempfile.rb")
7+
68
def initialize(*)
79
super
810
@tempfile = nil
@@ -172,6 +174,38 @@ def test_close_bang_does_not_unlink_if_already_unlinked
172174
end
173175
end unless /mswin|mingw/ =~ RUBY_PLATFORM
174176

177+
def test_finalizer_removes_file
178+
assert_in_out_err(["-r#{LIB_TEMPFILE_RB_PATH}"], <<~RUBY) do |(filename,*), (error,*)|
179+
file = Tempfile.new("foo")
180+
puts file.path
181+
RUBY
182+
assert_file.not_exist?(filename)
183+
assert_nil error
184+
end
185+
end
186+
187+
def test_finalizer_removes_file_when_dup
188+
assert_in_out_err(["-r#{LIB_TEMPFILE_RB_PATH}"], <<~RUBY) do |(filename,*), (error,*)|
189+
file = Tempfile.new("foo")
190+
file.dup
191+
puts file.path
192+
RUBY
193+
assert_file.not_exist?(filename)
194+
assert_nil error
195+
end
196+
end
197+
198+
def test_finalizer_removes_file_when_clone
199+
assert_in_out_err(["-r#{LIB_TEMPFILE_RB_PATH}"], <<~RUBY) do |(filename,*), (error,*)|
200+
file = Tempfile.new("foo")
201+
file.clone
202+
puts file.path
203+
RUBY
204+
assert_file.not_exist?(filename)
205+
assert_nil error
206+
end
207+
end
208+
175209
def test_finalizer_does_not_unlink_if_already_unlinked
176210
assert_in_out_err('-rtempfile', <<-'EOS') do |(filename,*), (error,*)|
177211
file = Tempfile.new('foo')
@@ -474,5 +508,4 @@ def test_create_anonymous_autoclose
474508
assert_equal(true, t.autoclose?)
475509
}
476510
end
477-
478511
end

0 commit comments

Comments
 (0)