Permalink
Browse files

Improve performance of applications using file uploads by not busting…

… the method cache on every request containing a file upload.
  • Loading branch information...
1 parent de9b338 commit 1da0dc2f906bbf6c248a33f8d18662fd1494cfba @carllerche carllerche committed Sep 23, 2010
Showing with 20 additions and 29 deletions.
  1. +20 −29 actionpack/lib/action_dispatch/http/upload.rb
@@ -2,49 +2,40 @@
module ActionDispatch
module Http
- module UploadedFile
- def self.extended(object)
- object.class_eval do
- attr_accessor :original_path, :content_type
- alias_method :local_path, :path if method_defined?(:path)
- end
- end
+ class UploadedFile < Tempfile
+ attr_accessor :original_filename, :content_type, :tempfile, :headers
- # Take the basename of the upload's original filename.
- # This handles the full Windows paths given by Internet Explorer
- # (and perhaps other broken user agents) without affecting
- # those which give the lone filename.
- # The Windows regexp is adapted from Perl's File::Basename.
- def original_filename
- unless defined? @original_filename
- @original_filename =
- unless original_path.blank?
- if original_path =~ /^(?:.*[:\\\/])?(.*)/m
- $1
- else
- File.basename original_path
- end
- end
+ def initialize(hash)
+ @original_filename = hash[:filename]
+ @content_type = hash[:type]
+ @headers = hash[:head]
+
+ # To the untrained eye, this may appear as insanity. Given the alternatives,
@nicksieger
nicksieger Sep 23, 2010 Contributor

I'd suggest that it is insanity. This is not likely to be compatible across Ruby implementations. It breaks for JRuby:

http://ci.jruby.org/job/rails-master/component=actionpack/46/console

+ # such as busting the method cache on every request or breaking backwards
+ # compatibility with is_a?(Tempfile), this solution is the best available
+ # option.
+ #
+ # TODO: Deprecate is_a?(Tempfile) and define a real API for this parameter
+ tempfile = hash[:tempfile]
+ tempfile.instance_variables.each do |ivar|
+ instance_variable_set(ivar, tempfile.instance_variable_get(ivar))
@exviva
exviva Sep 23, 2010 Contributor

What is this loop actually supposed to achieve? I'm just curious.

@nicksieger
nicksieger Sep 23, 2010 Contributor

It appears that it's supposed to copy internal state from the tempfile to the uploaded file.

On JRuby, however, Tempfile is implemented in Java for performance, and thus has state that can't be accessed by instance variables alone. It would be better to make UploadedFile has-a Tempfile rather than is-a.

@exviva
exviva Sep 23, 2010 Contributor

Thanks, that's more or less what I suspected.

@pwnall
pwnall Sep 25, 2010 Contributor

Since it's going to be a hack anyway, can the Tempfile compatibility issue be solved like below?
def is_a?(klass)
Tempfile.ancestors.include?(klass) || super
end

end
- @original_filename
end
+
+ alias local_path path
end
module Upload
# Convert nested Hash to HashWithIndifferentAccess and replace
# file upload hash with UploadedFile objects
def normalize_parameters(value)
if Hash === value && value.has_key?(:tempfile)
- upload = value[:tempfile]
- upload.extend(UploadedFile)
- upload.original_path = value[:filename]
- upload.content_type = value[:type]
- upload
+ UploadedFile.new(value)
else
super
end
end
private :normalize_parameters
end
end
-end
+end

3 comments on commit 1da0dc2

@companygardener

This doesn't seem to have identical functionality. Where is the equivalent of this snippet?

if original_path =~ /^(?:.*[:\\\/])?(.*)/m
  $1
else        
  File.basename original_path
end
@wycats
Member
wycats commented on 1da0dc2 Sep 24, 2010

@companygardner that code is now in Rack

@headius
Contributor
headius commented on 1da0dc2 Oct 2, 2010

Instance variables are not part of the public API of a class. There's no rule in Ruby that you can emulate "dup" by constructing a new instance and manually copying instance variables, since this does not work for other classes. If you want a copy, dup is what you should be using; an object knows how to dup/clone itself...you don't, and shouldn't.

If I understand it right, this may be broken anyway. Tempfile cleans up (closes and deletes) its attached file on finalization. Constructing a second Tempfile that points at the same open resource means that resource could potentially be closed/deleted before you're done with it if GC/finalization runs and cleans up the original Tempfile object.

Nick is right: aggregate, don't extend. :)

Please sign in to comment.