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

A finalizer was used in the Tempfile case to close and unlink the
Tempfile created, but it was never needed, because Tempfile defines
its own finalizer that closes and unlinks.

Update the spec so that it actually tests that tempfiles are getting
removed and unlinked. This uses 500 tempfiles in the JRuby test
because some lower values I tried failed CI.  CRuby could likely get
away with only a handle of tempfiles, but the spec uses 50 to be
sure it doesn't fail.

Author: jeremyevans

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'