Do not hold open File/Tempfile instances around

465fba83 · Mark Van Holstyn · c75cb763 · 465fba83 · 465fba83 · 465fba83
Commit 465fba83 authored Apr 19, 2008 by Mark Van Holstyn
Showing with 86 additions and 89 deletions

lib/paperclip.rb
+2 -2

lib/paperclip/attachment.rb
+13 -29

lib/paperclip/iostream.rb
+1 -1

lib/paperclip/storage.rb
+34 -28

test/test_attachment.rb
+14 -19

test/test_integration.rb
+22 -6

test/test_storage.rb
+0 -4

No files found.
--- a/lib/paperclip.rb
+++ b/lib/paperclip.rb
@@ -131,7 +131,7 @@ module Paperclip
      end
      define_method "#{name}?" do
-        ! attachment_for(name).file.nil?
+        ! attachment_for(name).original_filename.blank?
      end
      validates_each(name) do |record, attr, value|
@@ -161,7 +161,7 @@ module Paperclip
    # Places ActiveRecord-style validations on the presence of a file.
    def validates_attachment_presence name
      attachment_definitions[name][:validations] << lambda do |attachment, instance|
-        if attachment.file.nil? || !File.exist?(attachment.file.path)
+        if attachment.original_filename.blank?
          "must be set."
        end
      end

--- a/lib/paperclip/attachment.rb
+++ b/lib/paperclip/attachment.rb
@@ -15,7 +15,7 @@ module Paperclip
      }
    end
-    attr_reader :name, :instance, :file, :styles, :default_style
+    attr_reader :name, :instance, :styles, :default_style
    # Creates an Attachment object. +name+ is the name of the attachment, +instance+ is the
    # ActiveRecord object instance it's attached to, and +options+ is the same as the hash
@@ -35,19 +35,13 @@ module Paperclip
      @storage           = options[:storage]
      @options           = options
      @queued_for_delete = []
-      @processed_files   = {}
+      @queued_for_write  = {}
      @errors            = []
-      @file              = nil
      @validation_errors = nil
      @dirty             = false
      normalize_style_definition
      initialize_storage
-      if original_filename
-        @processed_files = locate_files
-        @file            = @processed_files[@default_style]
-      end
    end
    # What gets called when you call instance.attachment = File. It clears errors,
@@ -58,11 +52,11 @@ module Paperclip
      queue_existing_for_delete
      @errors            = []
-      @validation_errors = nil 
+      @validation_errors = nil
      return nil if uploaded_file.nil?
-      @file                               = uploaded_file.to_tempfile
+      @queued_for_write[:original]        = uploaded_file.to_tempfile
      @instance[:"#{@name}_file_name"]    = uploaded_file.original_filename
      @instance[:"#{@name}_content_type"] = uploaded_file.content_type
      @instance[:"#{@name}_file_size"]    = uploaded_file.size
@@ -79,8 +73,8 @@ module Paperclip
    # and can point to an action in your app, if you need fine grained security.
    # This is not recommended if you don't need the security, however, for
    # performance reasons.
-    def url style = nil
+    def url style = default_style
-      @file ? interpolate(@url, style) : interpolate(@default_url, style)
+      exists?(style) ? interpolate(@url, style) : interpolate(@default_url, style)
    end
    # Returns the path of the attachment as defined by the :path optionn. If the
@@ -118,7 +112,6 @@ module Paperclip
        flush_deletes
        flush_writes
        @dirty = false
-        @file = @processed_files[default_style]
        true
      else
        flush_errors
@@ -126,14 +119,6 @@ module Paperclip
      end
    end
-    # Returns representation of the data of the file assigned to the given
-    # style, in the format most representative of the current storage.
-    def to_file style = nil
-      @processed_files[style || default_style]
-    end
-    alias_method :to_io, :to_file
    # Returns the name of the file as originally assigned, and as lives in the
    # <attachment>_file_name attribute of the model.
    def original_filename
@@ -196,11 +181,11 @@ module Paperclip
    end
    def post_process #:nodoc:
-      return nil if @file.nil?
+      return if @queued_for_write[:original].nil?
      @styles.each do |name, args|
        begin
          dimensions, format = args
-          @processed_files[name] = Thumbnail.make(self.file, 
+          @queued_for_write[name] = Thumbnail.make(@queued_for_write[:original], 
                                                  dimensions,
                                                  format, 
                                                  @whiny_thumnails)
@@ -210,11 +195,9 @@ module Paperclip
          @errors << e.message
        end
      end
-      @processed_files[:original] = @file
    end
-    def interpolate pattern, style = nil #:nodoc:
+    def interpolate pattern, style = default_style #:nodoc:
-      style ||= default_style
      pattern = pattern.dup
      self.class.interpolations.each do |tag, l|
        pattern.gsub!(/:\b#{tag}\b/) do |match|
@@ -225,9 +208,10 @@ module Paperclip
    end
    def queue_existing_for_delete #:nodoc:
-      @queued_for_delete += @processed_files.values
+      return if original_filename.blank?
-      @file               = nil
+      @queued_for_delete += [:original, *@styles.keys].uniq.map do |style|
-      @processed_files    = {}
+        path(style) if exists?(style)
+      end.compact
      @instance[:"#{@name}_file_name"]    = nil
      @instance[:"#{@name}_content_type"] = nil
      @instance[:"#{@name}_file_size"]    = nil

--- a/lib/paperclip/iostream.rb
+++ b/lib/paperclip/iostream.rb
@@ -25,7 +25,7 @@ module IOStream
    while self.read(in_blocks_of, buffer) do
      dstio.write(buffer)
    end
-    dstio.rewind
+    dstio.rewind    
    dstio
  end
 end

--- a/lib/paperclip/storage.rb
+++ b/lib/paperclip/storage.rb
 module Paperclip
  module Storage
-    # With the Filesystem module installed, @file and @processed_files are just File instances.
    module Filesystem
      def self.extended base
      end
+      def exists?(style = default_style)
+        File.exist?(path(style))
+      end
-      def locate_files
+      # Returns representation of the data of the file assigned to the given
-        [:original, *@styles.keys].uniq.inject({}) do |files, style|
+      # style, in the format most representative of the current storage.
-          files[style] = File.new(path(style), "rb") if File.exist?(path(style))
+      def to_file style = default_style
-          files
+        File.new(path(style)) if exists?(style)
-        end
      end
+      alias_method :to_io, :to_file
      def flush_writes #:nodoc:
-        @processed_files.each do |style, file|
+        @queued_for_write.each do |style, file|
-          FileUtils.mkdir_p( File.dirname(path(style)) )
+          FileUtils.mkdir_p(File.dirname(path(style)))
-          @processed_files[style] = file.stream_to(path(style)) unless file.path == path(style)
+          result = file.stream_to(path(style))
+          file.close
+          result.close
        end
+        @queued_for_write = {}
      end
      def flush_deletes #:nodoc:
-        @queued_for_delete.compact.each do |file|
+        @queued_for_delete.each do |path|
          begin
-            FileUtils.rm(file.path)
+            FileUtils.rm(path) if File.exist?(path)
          rescue Errno::ENOENT => e
            # ignore them
          end
@@ -32,8 +38,6 @@ module Paperclip
      end
    end
-    # With the S3 module included, @file and the @processed_files will be
-    # RightAws::S3::Key instances.
    module S3
      def self.extended base
        require 'right_aws'
@@ -58,35 +62,37 @@ module Paperclip
        creds = find_credentials(creds).stringify_keys
        (creds[ENV['RAILS_ENV']] || creds).symbolize_keys
      end
+      def exists?(style = default_style)
+        @s3_bucket.key(path(style)) ? true : false
+      end
-      def locate_files
+      # Returns representation of the data of the file assigned to the given
-        [:original, *@styles.keys].uniq.inject({}) do |files, style|
+      # style, in the format most representative of the current storage.
-          files[style] = @s3_bucket.key(path(style))
+      def to_file style = default_style
-          files
+        @s3_bucket.key(path(style))
-        end
      end
+      alias_method :to_io, :to_file
      def flush_writes #:nodoc:
-        return if not dirty?
+        @queued_for_write.each do |style, file|
-        @processed_files.each do |style, key|
          begin
-            unless key.is_a? RightAws::S3::Key
+            key = @s3_bucket.key(path(style))
-              saved_data = key
+            key.data = file
-              key = @processed_files[style] = @s3_bucket.key(path(style))
-              key.data = saved_data
-            end
            key.put(nil, @s3_permissions)
          rescue RightAws::AwsError => e
-            @processed_files[style] = nil
            raise
          end
        end
+        @queued_for_write = {}
      end
      def flush_deletes #:nodoc:
-        @queued_for_delete.compact.each do |file|
+        @queued_for_delete.each do |path|
          begin
-            file.delete
+            if file = @s3_bucket.key(path)
+              file.delete
+            end
          rescue RightAws::AwsError
            # Ignore this.
          end

--- a/test/test_attachment.rb
+++ b/test/test_attachment.rb
@@ -96,7 +96,7 @@ class AttachmentTest < Test::Unit::TestCase
    end
    should "return its default_url when no file assigned" do
-      assert @attachment.file.nil?
+      assert @attachment.to_file.nil?
      assert_equal "/tests/original/missing.png", @attachment.url
      assert_equal "/tests/blah/missing.png", @attachment.url(:blah)
    end
@@ -123,29 +123,28 @@ class AttachmentTest < Test::Unit::TestCase
          @attachment.assign(@file)
        end
-        should "return the real url" do
-          assert @attachment.file
-          assert_equal "/tests/41/original/5k.png", @attachment.url
-          assert_equal "/tests/41/blah/5k.png", @attachment.url(:blah)
-        end
        should "be dirty" do
          assert @attachment.dirty?
        end
-        should "have its image and attachments as tempfiles" do
-          [nil, :large, :medium, :small].each do |style|
-            assert File.exists?(@attachment.to_io(style))
-          end
-        end
        context "and saved" do
          setup do
            @attachment.save
          end
+          should "return the real url" do
+            assert @attachment.to_file
+            assert_equal "/tests/41/original/5k.png", @attachment.url
+            assert_equal "/tests/41/small/5k.jpg", @attachment.url(:small)
+          end
+          should "return its default_url when no file assigned" do
+            assert @attachment.to_file
+            assert_equal "/tests/blah/missing.png", @attachment.url(:blah)
+          end
          should "commit the files to disk" do
-            [nil, :large, :medium, :small].each do |style|
+            [:large, :medium, :small].each do |style|
              io = @attachment.to_io(style)
              assert File.exists?(io)
              assert ! io.is_a?(::Tempfile)
@@ -166,12 +165,8 @@ class AttachmentTest < Test::Unit::TestCase
            end
          end
-          should "have #file be equal #to_io(:original)" do
-            assert_equal @attachment.file, @attachment.to_io(:original)
-          end
          should "still have its #file attribute not be nil" do
-            assert ! @attachment.file.nil?
+            assert ! @attachment.to_file.nil?
          end
          context "and deleted" do

--- a/test/test_integration.rb
+++ b/test/test_integration.rb
 require 'test/helper.rb'
 class IntegrationTest < Test::Unit::TestCase
+  context "Many models at once" do
+    setup do
+      rebuild_model
+      @file      = File.new(File.join(FIXTURES_DIR, "5k.png"))
+      300.times do |i|
+        Dummy.create! :avatar => @file
+      end
+    end
+    should "not exceed the open file limit" do
+       assert_nothing_raised do
+         dummies = Dummy.find(:all)
+         dummies.each { |dummy| dummy.avatar }
+       end
+    end
+  end
  context "A model with a filesystem attachment" do
    setup do
      rebuild_model :styles => { :large => "300x300>",
@@ -19,8 +36,7 @@ class IntegrationTest < Test::Unit::TestCase
    end
    should "write and delete its files" do
-      [["100x15", nil],
+      [["434x66", :original],
-       ["434x66", :original],
       ["300x46", :large],
       ["100x15", :medium],
       ["32x32", :thumb]].each do |geo, style|
@@ -78,10 +94,10 @@ class IntegrationTest < Test::Unit::TestCase
    end
    should "know the difference between good files, bad files, not files, and nil" do
-      expected = @dummy.avatar.file
+      expected = @dummy.avatar.to_file
      @dummy.avatar = "not a file"
      assert @dummy.valid?
-      assert_equal expected.path, @dummy.avatar.file.path
+      assert_equal expected.path, @dummy.avatar.to_file.path
      @dummy.avatar = @bad_file
      assert ! @dummy.valid?
@@ -204,10 +220,10 @@ class IntegrationTest < Test::Unit::TestCase
      end
      should "know the difference between good files, bad files, not files, and nil" do
-        expected = @dummy.avatar.file
+        expected = @dummy.avatar.to_file
        @dummy.avatar = "not a file"
        assert @dummy.valid?
-        assert_equal expected, @dummy.avatar.file
+        assert_equal expected, @dummy.avatar.to_file
        @dummy.avatar = @bad_file
        assert ! @dummy.valid?

--- a/test/test_storage.rb
+++ b/test/test_storage.rb
@@ -78,10 +78,6 @@ class S3Test < Test::Unit::TestCase
        @dummy.avatar = @file
      end
-      should "still return a Tempfile when sent #to_io" do
-        assert_equal Tempfile, @dummy.avatar.to_io.class
-      end
      context "and saved" do
        setup do
          @key_mock = stub