Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixed file descriptors leak in paperclip_loader #16

Merged
merged 1 commit into from

2 participants

@arr-ee

Fixes autotelik/datashift_spree#10

Looks like paperclip expects that file would be closed somewhere else. Actually I got lost somewhere in it so this one may be a little hackish.

Also sorry for the mess with whitespaces, please tell if that's a problem — I'll fix that.

@autotelik autotelik merged commit d6716b9 into from
@autotelik
Owner

Hi Max .. great stuff

I'll merge this in for now, but really you've highlighted that the get_file method is not too great, or at least badly named ! .. not clear that handle needs closing .. even to me and I wrote the damn thing !

Will review after me Easter hols

many thanks for contributing
tom

@arr-ee

Hey Tom, thanks for the fast response!

Actually naming is okay, but maybe it'd be better to mimic File.open w/ block approach (you know, we work with file in the block and then it gets closed automatically).

Or maybe it'd be even better to pass File object into the loader — less object allocations, less everything + it already ensures it's being closed afterwards.

Or even mix this approaches. I'll try to play with that during weekend.

@autotelik
Owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 28, 2013
  1. @arr-ee

    Fixed file descriptor leak

    arr-ee authored
This page is out of date. Refresh to see the latest.
Showing with 49 additions and 43 deletions.
  1. +49 −43 lib/loaders/paperclip/datashift_paperclip.rb
View
92 lib/loaders/paperclip/datashift_paperclip.rb
@@ -4,115 +4,121 @@
# License:: MIT. Free, Open Source.
#
# Details:: Module containing common functionality for working with Paperclip attachments
-#
+#
require 'logging'
require 'paperclip'
module DataShift
module Paperclip
-
+
include DataShift::Logging
-
+
require 'paperclip/attachment_loader'
-
+
attr_accessor :attachment
-
+
# Get all image files (based on file extensions) from supplied path.
- # Options :
+ # Options :
# :glob : The glob to use to find files
# => :recursive : Descend tree looking for files rather than just supplied path
-
+
def self.get_files(path, options = {})
return [path] if(File.file?(path))
glob = options[:glob] ? options[:glob] : '*.*'
glob = (options['recursive'] || options[:recursive]) ? "**/#{glob}" : glob
-
+
Dir.glob("#{path}/#{glob}", File::FNM_CASEFOLD)
end
-
+
def get_file( attachment_path )
-
+
unless File.exists?(attachment_path) && File.readable?(attachment_path)
logger.error("Cannot process Image from #{Dir.pwd}: Invalid Path #{attachment_path}")
raise PathError.new("Cannot process Image : Invalid Path #{attachment_path}")
end
-
+
file = begin
File.new(attachment_path, "rb")
rescue => e
puts e.inspect
raise PathError.new("ERROR : Failed to read image from #{attachment_path}")
end
-
+
file
end
-
+
# Note the paperclip attachment model defines the storage path via something like :
# => :path => ":rails_root/public/blah/blahs/:id/:style/:basename.:extension"
- #
- # Options
- #
+ #
+ # Options
+ #
# :attributes
- #
+ #
# Pass through a hash of attributes to the Paperclip klass's initializer
- #
+ #
# :has_attached_file_name
- #
- # Paperclip attachment name defined with macro 'has_attached_file :name'
- #
+ #
+ # Paperclip attachment name defined with macro 'has_attached_file :name'
+ #
# This is usually called/defaults :attachment
- #
- # e.g
- # When : has_attached_file :avatar
- #
+ #
+ # e.g
+ # When : has_attached_file :avatar
+ #
# Give : {:has_attached_file_attribute => :avatar}
- #
- # When : has_attached_file :icon
+ #
+ # When : has_attached_file :icon
#
# Give : { :has_attached_file_attribute => :icon }
- #
+ #
def create_attachment(klass, attachment_path, record = nil, attach_to_record_field = nil, options = {})
-
+
has_attached_file_attribute = options[:has_attached_file_name] ? options[:has_attached_file_name].to_sym : :attachment
-
+
# e.g (:attachment => File.read)
- paperclip_attributes = { has_attached_file_attribute => get_file(attachment_path) }
-
+
+ attachment_file = get_file(attachment_path)
+ paperclip_attributes = { has_attached_file_attribute => attachment_file }
+
paperclip_attributes.merge!(options[:attributes]) if(options[:attributes])
-
- begin
- @attachment = klass.new(paperclip_attributes, :without_protection => true)
+
+ begin
+ @attachment = klass.new(paperclip_attributes, :without_protection => true)
rescue => e
puts e.inspect
logger.error("Failed to create PaperClip Attachment : #{e.inspect}")
raise CreateAttachmentFailed.new("Failed to create PaperClip Attachment from : #{attachment_path}")
+ ensure
+ attachment_file.close unless attachment_file.closed?
end
-
+
begin
-
+
if(@attachment.save)
puts "Success: Created Attachment #{@attachment.id} : #{@attachment.attachment_file_name}"
-
+
if(attach_to_record_field.is_a? MethodDetail)
DataShift::Populator.new().assign(attach_to_record_field, record, @attachment)
else
- # assume its not a has_many and try basic send
+ # assume its not a has_many and try basic send
record.send("#{attach_to_record_field}=", @attachment)
end if(record && attach_to_record_field)
-
+
else
puts "ERROR : Problem saving to DB : #{@attachment.inspect}"
puts @attachment.errors.messages.inspect
end
-
+
@attachment
rescue => e
logger.error("Problem saving Paperclip Attachment: #{e.inspect}")
puts e.inspect
raise CreateAttachmentFailed.new("PaperClip error - Problem saving Attachment")
+ ensure
+ attachment_file.close unless attachment_file.closed?
end
- end
+ end
end
-
+
end
Something went wrong with that request. Please try again.