Browse files

Added uploaded_file instance variable for new assigns

  • Loading branch information...
1 parent 131a8b1 commit 24b0c5adbc3f05b8f3356611329127c7ff6c9311 @yawn yawn committed with mike-burns Apr 1, 2011
Showing with 8 additions and 1 deletion.
  1. +8 −1 lib/paperclip/attachment.rb
View
9 lib/paperclip/attachment.rb
@@ -94,6 +94,8 @@ def assign uploaded_file
uploaded_filename = uploaded_file.original_filename
uploaded_file = uploaded_file.to_file(:original)
close_uploaded_file = uploaded_file.respond_to?(:close)
+ else
+ instance_write(:uploaded_file, uploaded_file)
@exviva
exviva added a line comment May 15, 2012

I know this is a very old commit, but we've just upgraded from Paperclip 2.4.0 to 2.7.0 (this commit landed in 2.4.5) and this line is causing our test suite to fail. I'm reporting it in case someone runs into the same issue.

The uploaded_file is memoized in an ivar and for some reason, it seems that it's never garbage collected - the file descriptor is never released, which causes a flood of Errno::EMFILE: Too many open files errors.

I git-bisected paperclip to that specific commit, and indeed commenting out that line makes everything pass again.

I printed the number of open file descriptors using something like:

RSpec.configure do |config|
  config.after { puts `lsof -p #{Process.pid} | wc -l` }
end

It's constantly growing after each example.

We're assigning the attachments to records using FactoryGirl and File.new.

I saw that on Paperclip 3.x this line has been removed (in 89c8d11), without any real notice.

It seems wrong that the ensure section below only closes the uploaded_file if it was a Paperclip::Attachment - I think it should close it regardless of type.

Are any patches still accepted for Paperclip 2.7.x?

@sikachu
thoughtbot, inc. member
sikachu added a line comment May 15, 2012

Patch is still accepted for 2.7.x

However, the thing is: Paperclip will not close the file you assign to the attachment. Paperclip will copy the file to Tempfile after you assign it, so you're on your own on closing the file whenever you want to. We have to go with this route because in some circumstances people are still interacting with the file after passed to Paperclip, and it resulted in unrecoverable error since you can't reopen the file after Paperclip has closed it.

@exviva
exviva added a line comment May 15, 2012

@sikachu I understand that Paperclip should not close the files passed to it, but as of 2.4.4 they used to be closed automatically when Ruby finalized them. After this commit this no longer happens (since the file object is memoized), so I'd consider this a bug.

Also, Paperclip documentation used to encourage the use of something along the lines of user.avatar = File.new(...) in tests (and shipped with the Upfile module to allow such assignment), never mentioning the need to manually close such a file.

Would you accept a patch removing this line? It's gone in 3.x anyway.

Or maybe I'm missing something? This whole file descriptor mess can be really confusing for me :(.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
return nil unless valid_assignment?(uploaded_file)
@@ -197,12 +199,17 @@ def destroy
end
end
+ # Returns the uploaded file if present.
+ def uploaded_file
+ instance_read(:uploaded_file)
+ end
+
# Returns the name of the file as originally assigned, and lives in the
# <attachment>_file_name attribute of the model.
def original_filename
instance_read(:file_name)
end
-
+
# Returns the size of the file as originally assigned, and lives in the
# <attachment>_file_size attribute of the model.
def size

0 comments on commit 24b0c5a

Please sign in to comment.