From 4fa741f1e4eee0e0f3cf5dd4c6e67a40d304d4e3 Mon Sep 17 00:00:00 2001
From: Jeremy Evans <code@jeremyevans.net>
Date: Sat, 8 Jul 2023 19:55:16 -0700
Subject: [PATCH] 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.
---
 History.md                           |  8 +++++++
 lib/rack/test/uploaded_file.rb       | 14 -----------
 spec/rack/test/uploaded_file_spec.rb | 36 +++++++++++++---------------
 3 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/History.md b/History.md
index 738c592..80d5cb0 100644
--- a/History.md
+++ b/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:
diff --git a/lib/rack/test/uploaded_file.rb b/lib/rack/test/uploaded_file.rb
index 39346e4..5be6dc7 100644
--- a/lib/rack/test/uploaded_file.rb
+++ b/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
diff --git a/spec/rack/test/uploaded_file_spec.rb b/spec/rack/test/uploaded_file_spec.rb
index 67bfbc5..5c42dba 100644
--- a/spec/rack/test/uploaded_file_spec.rb
+++ b/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'