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

bsodmike · 2017-05-20T00:51:01Z

Hi all - I noticed the file descriptor of the tempfile is left 'hanging' and have ensured it is closed on GC.

Cheers!

…tor on GC

perlun · 2017-05-22T19:49:23Z

Thanks @bsodmike for your contribution! This does unfortunately not build cleanly on JRuby, as the Travis logs indicate.

Any idea on how we can make it work there also?

(For reference: I did some minor cleanups to prepare the code for more uniform style compared to the existing codebase.)

bsodmike · 2017-05-23T01:04:05Z

Thanks so much @perlun - will take a look at the CI logs and fix this up asap.

bsodmike · 2017-06-02T06:10:44Z

Hey @perlun it's good to go now - thanks! I had a bad conflict resolution, so put back some corrections you had made. Oops!

Interestingly, I ran the jRuby GC in a loop (without my patch) ~ 100_000.times (ha!) and obviously got

     # Java::JavaIo::IOException:
     #   unhandled errno: Too many open files in system
     #   org.jruby.util.RegularFileResource.openChannel(RegularFileResource.java:177)

For this spec, I reduced it to 20 and that seems to work fine.

bsodmike · 2017-06-02T13:00:39Z

This would also close #118

dennissivia

@perlun @junaruga I am not sure that finalizers are the best solution for this, due to some existing edge cases. However it is way better than having the current issue. I think we should accept this improvement and we can still improve/change the details later.
This PR directly changes an existing issue without changing the public API, thus I think it is good to merge.

junaruga · 2017-06-02T19:02:18Z

.gitignore

@@ -6,3 +6,4 @@ VERSION
 *.rbc
 *.swp
 /.bundle
+.idea/


.idea? A directory for "Intellij IDE"?
I am not know detail about it.
Kind of this case **/.idea is possible?
/.idea looks better, isn't it referring /.bundle?

.idea/ auto globs .idea/**. You don't need to be that specific :)
Yes, it's for Intellij IDEs.

I mean if aa/.idea exists, .idea makes sense. But if ./.idea/**, /.idea is correct. /.idea in .gitignore means ./.idea/**.

Sure, I'll change this to /.idea

junaruga · 2017-06-02T19:25:46Z

lib/rack/test/uploaded_file.rb

@@ -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))
+


How about this?
It looks more simple.

ObjectSpace.define_finalizer(self, self.close)

def close() @tempfile.close if @tempfile end

Check this out mate, http://www.mikeperham.com/2010/02/24/the-trouble-with-ruby-finalizers/

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

junaruga · 2017-06-02T19:43:36Z

Sorry for my late replying.
Though I do not understand the use case that UploadedFile is used in.

We can not close @tempfile file descriptor in def initialize, right?

In my opinion, if we can not close it, such as Ruby File class (http://ruby-doc.org/core-2.4.1/File.html) and Java InputStream class(https://docs.oracle.com/javase/7/docs/api/java/io/InputStream.html).

The responsibility to close the file descriptor is likely to be for user side.

Or provide static method?

UploadedFile::open(foo) do |file|
  something for file.
end

Sorry, just I got idea. It may be wrong.

bsodmike · 2017-06-02T21:09:56Z

We can not close @tempfile file descriptor in def initialize, right?

Yes, that's the way this API has been setup and does make sense. The user is supposed to 'work' with the provided descriptor, and ideally close it when done. But this allows for user error.

The responsibility to close the file descriptor is likely to be for user side.

Right, exactly. That's the reason I've set this up to auto-close the descriptor on GC if the user forgets to close the descriptor. It's also the reason one should typically use the block-form when working with IO in Ruby (as closing the descriptor is automatically handled).

Think of this as a safe-guard. Of course, it wouldn't break if the user decides to close the descriptor either.

junaruga · 2017-06-02T21:17:09Z

Think of this as a safe-guard. Of course, it wouldn't break if the user decides to close the descriptor either.

Yes, I agree with you.
So, we can provide the close method that is called by finalizer (GC) also can be called by user, right?

bsodmike · 2017-06-02T21:27:08Z

Yes, they can call tempfile.close and also tempfile.unlink - these are methods on Tempfile
https://ruby-doc.org/stdlib-2.4.0/libdoc/tempfile/rdoc/Tempfile.html#method-i-unlink

We do not need to provide this explicitly, as the user already has access to the tempfile descriptor.

…dation

bsodmike · 2017-06-02T21:32:45Z

Added an improvement as recommended here https://ruby-doc.org/stdlib-2.4.0/libdoc/tempfile/rdoc/Tempfile.html

When a Tempfile object is garbage collected, or when the Ruby interpreter exits, its associated temporary file is automatically deleted. This means that’s it’s unnecessary to explicitly delete a Tempfile after use, though it’s good practice to do so: not explicitly deleting unused Tempfiles can potentially leave behind large amounts of tempfiles on the filesystem until they’re garbage collected. The existence of these temp files can make it harder to determine a new Tempfile filename.

Therefore, one should always call unlink or close in an ensure block

junaruga · 2017-06-02T21:36:27Z

Thank you for the replying. Now at 23:30 in my area. So, let me check it later.
I will definitely spend a time until this PR is merged.

bsodmike · 2017-06-02T21:40:14Z

Sure mate, here's a quick snippet for you - this is normally how a user should handle a tempfile, if they do not use the block format.

file = File.join(Rails.root, 'tmp', 'foo.jpg')
FileUtils.touch file
tempfile = Rack::Test::UploadedFile.new file

# with my patch we are safe-guarding the user by doing this on GC
# even if they forget to do so.
tempfile.close
tempfile.unlink

junaruga · 2017-06-05T13:15:15Z

spec/rack/test/uploaded_file_spec.rb

+    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'


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?

junaruga · 2017-06-05T13:16:25Z

@bsodmike ok thanks. I commented one thing about removing && RUBY_VERSION == '2.3.1'.
Other things, it looks good for me.

bsodmike · 2017-06-05T13:16:49Z

Sure, I'll push this in about 4 hours time. Thanks mate.

On Mon, 5 Jun 2017 at 18:45, Jun Aruga ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In spec/rack/test/uploaded_file_spec.rb <#180 (comment)>: > @@ -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' Current JRuby '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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#180 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWehAM0O7n5VLuJX5oQLIepcTJ7H5yRks5sA_9kgaJpZM4NhJJQ> .

-- -- Mike's on the move!

bsodmike · 2017-06-05T20:03:31Z

@junaruga I've updated this mate, thanks.

junaruga

I approved the review! thanks.

bsodmike · 2017-06-05T21:41:09Z

Thanks mate, so how does this get merged?

junaruga · 2017-06-05T21:43:15Z

I did merged with squash option for your several commits. Check latest master branch.

$ git log -p
commit b3b20dd45a7074a4324f270719f96bdad89a6212
Author: Michael de Silva <michael@mwdesilva.com>
Date:   Tue Jun 6 03:11:15 2017 +0530

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

agibralter · 2018-08-20T22:36:56Z

@bsodmike Hi, I am just curious about the use case for this change—are the file descriptors left hanging when the ruby process exits? Or just during GC? I presume most people are using this class for tests, which shouldn't really be long-running processes. Just curious!

…tor on GC (rack#180)

bsodmike and others added 4 commits May 20, 2017 06:13

FIX: Ensure Rack::Test::UploadedFile closes its tempfile file descrip…

7030e86

…tor on GC

FIX: Ensure an instance of UploadedFile is correctly returned

b0665b6

Nitpick: Changed single quotes to double for consistency

50a72f1

Nitpick: Whitespace fix

86c464c

FIX: Patch spec to support jRuby

476df61

bsodmike added 2 commits June 2, 2017 11:42

FIX: Change single quotes to double for consistency, again

f7f291d

Merge branch 'master' into fix/uploadedfile_close_file_descriptor

eed1a17

dennissivia reviewed Jun 2, 2017

View reviewed changes

junaruga reviewed Jun 2, 2017

View reviewed changes

bsodmike added 2 commits June 3, 2017 02:41

Merge branch 'master' into fix/uploadedfile_close_file_descriptor

82e7078

FIX: Correct entry for IntelliJ IDEs as per review

a69651d

FIX: Ensure descriptor is unlinked post close as per API doc recommen…

b28b644

…dation

junaruga reviewed Jun 5, 2017

View reviewed changes

FIX: Update spec, do not specify Ruby version, only platform

274e8e1

junaruga approved these changes Jun 5, 2017

View reviewed changes

junaruga merged commit b3b20dd into rack:master Jun 5, 2017

perlun mentioned this pull request Jul 18, 2017

Close tempfile after upload #118

Closed

alex-damian-negru pushed a commit to alex-damian-negru/rack-test that referenced this pull request Apr 5, 2021

FIX: Ensure Rack::Test::UploadedFile closes its tempfile file descrip…

79e4f51

…tor on GC (rack#180)

kyori19 mentioned this pull request Jul 8, 2023

Accessing Tempfile after GC of UploadedFile will cause IOError #338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bsodmike commented May 20, 2017

perlun commented May 22, 2017

bsodmike commented May 23, 2017

bsodmike commented Jun 2, 2017 •

edited

Loading

bsodmike commented Jun 2, 2017

dennissivia left a comment

junaruga Jun 2, 2017

bsodmike Jun 2, 2017

junaruga Jun 2, 2017

bsodmike Jun 2, 2017

junaruga Jun 2, 2017 •

edited

Loading

bsodmike Jun 2, 2017

junaruga Jun 5, 2017

junaruga commented Jun 2, 2017 •

edited

Loading

bsodmike commented Jun 2, 2017 •

edited

Loading

junaruga commented Jun 2, 2017

bsodmike commented Jun 2, 2017 •

edited

Loading

bsodmike commented Jun 2, 2017

junaruga commented Jun 2, 2017

bsodmike commented Jun 2, 2017 •

edited

Loading

junaruga Jun 5, 2017 •

edited

Loading

junaruga commented Jun 5, 2017

bsodmike commented Jun 5, 2017 via email

bsodmike commented Jun 5, 2017

junaruga left a comment

bsodmike commented Jun 5, 2017

junaruga commented Jun 5, 2017 •

edited

Loading

agibralter commented Aug 20, 2018

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

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

Conversation

bsodmike commented May 20, 2017

perlun commented May 22, 2017

bsodmike commented May 23, 2017

bsodmike commented Jun 2, 2017 • edited Loading

bsodmike commented Jun 2, 2017

dennissivia left a comment

Choose a reason for hiding this comment

junaruga Jun 2, 2017

Choose a reason for hiding this comment

bsodmike Jun 2, 2017

Choose a reason for hiding this comment

junaruga Jun 2, 2017

Choose a reason for hiding this comment

bsodmike Jun 2, 2017

Choose a reason for hiding this comment

junaruga Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

bsodmike Jun 2, 2017

Choose a reason for hiding this comment

junaruga Jun 5, 2017

Choose a reason for hiding this comment

junaruga commented Jun 2, 2017 • edited Loading

bsodmike commented Jun 2, 2017 • edited Loading

junaruga commented Jun 2, 2017

bsodmike commented Jun 2, 2017 • edited Loading

bsodmike commented Jun 2, 2017

junaruga commented Jun 2, 2017

bsodmike commented Jun 2, 2017 • edited Loading

junaruga Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

junaruga commented Jun 5, 2017

bsodmike commented Jun 5, 2017 via email

bsodmike commented Jun 5, 2017

junaruga left a comment

Choose a reason for hiding this comment

bsodmike commented Jun 5, 2017

junaruga commented Jun 5, 2017 • edited Loading

agibralter commented Aug 20, 2018

bsodmike commented Jun 2, 2017 •

edited

Loading

junaruga Jun 2, 2017 •

edited

Loading

junaruga commented Jun 2, 2017 •

edited

Loading

bsodmike commented Jun 2, 2017 •

edited

Loading

bsodmike commented Jun 2, 2017 •

edited

Loading

bsodmike commented Jun 2, 2017 •

edited

Loading

junaruga Jun 5, 2017 •

edited

Loading

junaruga commented Jun 5, 2017 •

edited

Loading