Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

make Tempfile unduplicable #2068

Closed
wants to merge 1 commit into from

6 participants

@amatsuda
Collaborator

GCing a Tempfile object deletes the actual file on the filesystem. So, two (or more) Tempfiles pointing to one same actual file should better not exist.
However, we found this actually happens in our real world applications because Tempfiles are duped in parameter_filter here https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/http/parameter_filter.rb#L39

Because of this, a request with an attachment very rarely dies with IOError: closed stream.

Attached a patch and test that changes Tempfile unduplicable.

@josevalim josevalim commented on the diff
...port/lib/active_support/core_ext/object/duplicable.rb
@@ -1,3 +1,5 @@
+require 'tempfile' unless defined?(Tempfile)
@josevalim Owner

Why do we need the defined?() check? :)

@amatsuda Collaborator

Well, actually I just copied this line from here without understanding what it really aims to do https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/file/atomic.rb#L16
I guess it might not needed. Should I better remove it?

@josevalim Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmathieu
Collaborator

What if dup created a new file with the same content ?
This might be a feature for ruby, not rails though

@josevalim
Owner

We would then need to discuss if duplicable? means .dup is available or if the semantics of calling .dup suits Rails. In any case, we could always change filter_parameters to ignore tempfiles.

@jeremy
Owner

Agreed, @josevalim. We should be careful about when to dup instead of changing duplicable?. In this case, the parameter filter should check for Tempfile and, say, show it as a path or some placeholder string.

@steveklabnik
Collaborator

This pull request needs to be rebased, as it can't be auto-merged any more. Mind doing that, @amatsuda?

@amatsuda amatsuda make Tempfile unduplicable
GCing a Tempfile object deletes the actual file on the filesystem. Thus, two Tempfiles pointing to one same actual file should better not exist
6cae7e4
@amatsuda
Collaborator

@steveklabnik Rebased.

@jeremy
Owner

The same concerns stand. The parameter filter should handle Tempfile itself rather than changing duplicable? to achieve the same result.

@jeremy jeremy closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 1, 2012
  1. @amatsuda

    make Tempfile unduplicable

    amatsuda authored
    GCing a Tempfile object deletes the actual file on the filesystem. Thus, two Tempfiles pointing to one same actual file should better not exist
This page is out of date. Refresh to see the latest.
View
12 activesupport/lib/active_support/core_ext/object/duplicable.rb
@@ -93,3 +93,15 @@ def duplicable?
# can't dup, so use superclass implementation
end
end
+
+require 'tempfile' unless defined?(Tempfile)
@josevalim Owner

Why do we need the defined?() check? :)

@amatsuda Collaborator

Well, actually I just copied this line from here without understanding what it really aims to do https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/file/atomic.rb#L16
I guess it might not needed. Should I better remove it?

@josevalim Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+class Tempfile
+ # Tempfiles are not duplicable:
+ #
+ # t = Tempfile.new('foo.txt') # => #<File:0x101bf2438>
+ # t.dup # => #<File:0x101bf2438>
+ #
+ def duplicable?
+ false
+ end
+end
View
3  activesupport/test/core_ext/duplicable_test.rb
@@ -6,7 +6,7 @@
class DuplicableTest < ActiveSupport::TestCase
RAISE_DUP = [nil, false, true, :symbol, 1, 2.3, 5.seconds]
YES = ['1', Object.new, /foo/, [], {}, Time.now, Class.new, Module.new]
- NO = []
+ NO = [Tempfile.new('temporary.txt')]
begin
bd = BigDecimal.new('4.56')
@@ -15,7 +15,6 @@ class DuplicableTest < ActiveSupport::TestCase
RAISE_DUP << bd
end
-
def test_duplicable
(RAISE_DUP + NO).each do |v|
assert !v.duplicable?
Something went wrong with that request. Please try again.