Skip to content

Commit

Permalink
Ensure finalizer order in Tempfile
Browse files Browse the repository at this point in the history
The Closer and Remover finalizers are defined on different objects in
Tempfile. The Closer is defined on the Tempfile object while the Remover
is defined on the finalizer_obj. This means that there is no guarantee
of the finalizer order.

On Windows, we must close the file before removing it because we cannot
remove an open file. But since the order is not guaranteed, the GC may
run the Remover finalizer first, which will fail with an Errno::EACCES
(Permission denied @ apply2files).

This commit changes it so that both the Closer and Remover finalizers
are defined on the finalizer_obj, which guarantees the order that it is
ran.
  • Loading branch information
peterzhu2118 committed Aug 15, 2024
1 parent ae896a5 commit eb2d8b1
Showing 1 changed file with 15 additions and 6 deletions.
21 changes: 15 additions & 6 deletions lib/tempfile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -228,22 +228,25 @@ def initialize(basename="", tmpdir=nil, mode: 0, **options)
tmpfile = File.open(tmpname, @mode, **opts)
@opts = opts.freeze
end
ObjectSpace.define_finalizer(@finalizer_obj, Remover.new(tmpfile.path))
ObjectSpace.define_finalizer(self, Closer.new(tmpfile))

super(tmpfile)

define_finalizers
end

private def define_finalizers
ObjectSpace.define_finalizer(@finalizer_obj, Closer.new(__getobj__))
ObjectSpace.define_finalizer(@finalizer_obj, Remover.new(__getobj__.path))
end

def initialize_dup(other) # :nodoc:
initialize_copy_iv(other)
super(other)
ObjectSpace.define_finalizer(self, Closer.new(__getobj__))
end

def initialize_clone(other) # :nodoc:
initialize_copy_iv(other)
super(other)
ObjectSpace.define_finalizer(self, Closer.new(__getobj__))
end

private def initialize_copy_iv(other) # :nodoc:
Expand All @@ -256,10 +259,13 @@ def initialize_clone(other) # :nodoc:
# Opens or reopens the file with mode "r+".
def open
_close
ObjectSpace.undefine_finalizer(self)

mode = @mode & ~(File::CREAT|File::EXCL)
__setobj__(File.open(__getobj__.path, mode, **@opts))
ObjectSpace.define_finalizer(self, Closer.new(__getobj__))

ObjectSpace.undefine_finalizer(@finalizer_obj)
define_finalizers

__getobj__
end

Expand Down Expand Up @@ -327,7 +333,10 @@ def unlink
# may not be able to unlink on Windows; just ignore
return
end

ObjectSpace.undefine_finalizer(@finalizer_obj)
ObjectSpace.define_finalizer(@finalizer_obj, Closer.new(__getobj__))

@unlinked = true
end
alias delete unlink
Expand Down

1 comment on commit eb2d8b1

@jeremyevans
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can see the issue on Windows, I don't think this change is correct. Each Tempfile instance has a separate File instance and file descriptor:

t = Tempfile.new
t.to_i # => 6
t.dup.to_i => 7

I think you still need the finalizers defined in initialize_dup and initialize_clone, even if they use @finalizer_obj instead of self. The ObjectSpace.undefine_finalizer(@finalizer_obj) in open looks wrong as it will undefine the closer finalizers for other related Tempfile instances that were created by dup/clone.

Even if you do this, it means that if you are using dup/clone, the tempfile fds will not be closed until all related Tempfile instances are GCed.

Please sign in to comment.