Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Ensure Rack::Test::UploadedFile closes its tempfile file descriptor on GC #180

Merged
merged 11 commits into from
Jun 5, 2017
Merged
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ VERSION
*.rbc
*.swp
/.bundle
/.idea
11 changes: 11 additions & 0 deletions lib/rack/test/uploaded_file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ def initialize(path, content_type = "text/plain", binary = false)
@tempfile.set_encoding(Encoding::BINARY) if @tempfile.respond_to?(:set_encoding)
@tempfile.binmode if binary

ObjectSpace.define_finalizer(self, self.class.finalize(@tempfile))

Copy link
Contributor

@junaruga junaruga Jun 2, 2017

Choose a reason for hiding this comment

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

How about this?
It looks more simple.

ObjectSpace.define_finalizer(self, self.close)
def close()
  @tempfile.close if @tempfile
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I check it. and I learned that we have to use the static method.

FileUtils.copy_file(path, @tempfile.path)
end

Expand All @@ -47,6 +49,15 @@ def respond_to?(method_name, include_private = false) #:nodoc:
@tempfile.respond_to?(method_name, include_private) || super
end

def self.finalize(file)
proc { actually_finalize file }
end

def self.actually_finalize(file)
file.close
file.unlink
end

end

end
Expand Down
31 changes: 31 additions & 0 deletions spec/rack/test/uploaded_file_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ def test_file_path
File.dirname(__FILE__) + "/../../fixtures/foo.txt"
end

it "returns an instance of `Rack::Test::UploadedFile`" do
uploaded_file = Rack::Test::UploadedFile.new(test_file_path)

expect(uploaded_file).to be_a(Rack::Test::UploadedFile)
end

it "responds to things that Tempfile responds to" do
uploaded_file = Rack::Test::UploadedFile.new(test_file_path)

Expand All @@ -26,4 +32,29 @@ def test_file_path

expect(File.extname(uploaded_file.path)).to eq(".txt")
end

context "it should call its destructor" do
it "calls the destructor" do
expect(Rack::Test::UploadedFile).to receive(:actually_finalize).at_least(:once)

if RUBY_PLATFORM == 'java' && RUBY_VERSION == '2.3.1'
Copy link
Contributor

@junaruga junaruga Jun 5, 2017

Choose a reason for hiding this comment

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

Current jruby-head 's Ruby version is 2.3.3. jruby-head is failed because of the reason.
I do not want to write actual version number in the test code.
Can you remove && RUBY_VERSION == '2.3.1' from the code?

require 'java'
java_import 'java.lang.System'

20.times do |i|
uploaded_file = Rack::Test::UploadedFile.new(test_file_path)

uploaded_file = nil

System.gc()
end
else
uploaded_file = Rack::Test::UploadedFile.new(test_file_path)

uploaded_file = nil

GC.start
end
end
end
end