Remove use of finalizer in Rack::Test::UploadedFile (Fixes #338)

History.md

@@ -1,3 +1,11 @@
+## main
+
+* Minor enhancements:
+  * `Rack::Test::UploadedFile` no longer uses a finalizer for named
+    paths to close and unlink the created Tempfile.  Tempfile itself
+    uses a finalizer to close and unlink itself, so there is no
+    reason for `Rack::Test::UploadedFile` to do so (Jeremy Evans #338)
+
 ## 2.1.0 / 2023-03-14
 
 * Breaking changes:

lib/rack/test/uploaded_file.rb

@@ -72,18 +72,6 @@ def respond_to_missing?(method_name, include_private = false) #:nodoc:
         tempfile.respond_to?(method_name, include_private) || super
       end
 
-      # A proc that can be used as a finalizer to close and unlink the tempfile.
-      def self.finalize(file)
-        proc { actually_finalize file }
-      end
-
-      # Close and unlink the given file, used as a finalizer for the tempfile,
-      # if the tempfile is backed by a file in the filesystem.
-      def self.actually_finalize(file)
-        file.close
-        file.unlink
-      end
-
       private
 
       # Use the StringIO as the tempfile.
@@ -104,8 +92,6 @@ def initialize_from_file_path(path)
         @tempfile = Tempfile.new([::File.basename(@original_filename, extension), extension])
         @tempfile.set_encoding(Encoding::BINARY)
 
-        ObjectSpace.define_finalizer(self, self.class.finalize(@tempfile))
-
         FileUtils.copy_file(path, @tempfile.path)
       end
     end

spec/rack/test/uploaded_file_spec.rb

@@ -51,35 +51,31 @@ def file_path
     proc{Rack::Test::UploadedFile.new('does_not_exist')}.must_raise RuntimeError
   end
 
-  it 'finalizes on garbage collection' do
-    finalized = false
-    c = Class.new(Rack::Test::UploadedFile) do
-      define_singleton_method(:actually_finalize) do |file|
-        finalized = true
-        super(file)
-      end
+  def local_paths(n)
+    local_paths = n.times.map do
+      Rack::Test::UploadedFile.new(file_path)
     end
+    local_paths.map(&:local_path).all?{|f| File.exist?(f)}.must_equal true
+    local_paths.map!(&:local_path)
+    local_paths.uniq.size.must_equal n
+    local_paths
+  end
+
+  it 'removes local paths on garbage collection' do
 
     if RUBY_PLATFORM == 'java'
       require 'java'
       java_import 'java.lang.System'
 
-      50.times do |_i|
-        c.new(file_path)
-        System.gc
-      end
+      paths = local_paths(500)
+      System.gc
     else
-      50.times do |_i|
-        c.new(file_path)
-        GC.start
-      end
+      paths = local_paths(50)
+      GC.start
     end
 
-    # Due to CRuby's conservative garbage collection, you can never guarantee
-    # an object will be garbage collected, so this is a source of potential
-    # nondeterministic failure
-    finalized.must_equal true
-  end if RUBY_VERSION >= '2.7' || RUBY_ENGINE != 'ruby'
+    paths.all?{|f| File.exist?(f)}.must_equal false
+  end
 
   it '#initialize with an IO object sets the specified filename' do
     original_filename = 'content.txt'