Tempfile not deleted when using "format"

[mrkamel]

I'm using 3.7.0.
Minimagick leaves an additional tempfile in /tmp when using the "format" command.

# test.rb

require "mini_magick"

image = MiniMagick::Image.open("test.tif")
image.format "jpg"

such that my disk fills up.

Before i run ruby test.rb: ls /tmp/mini_magick* gives 0 results
Afterwards, i have a /tmp/mini_magick20131127-18504-ula55j.jpg that doesn't get deleted when the script terminates, etc.

[mrkamel]

I wrote a fix. Maybe somebody from core could review it, escpecially because it uses convert instead of mogrify (is this ok? - it's imho better suited for Image#format) - and moreover, someone could explain the use case for the existing yield to me:

def format(...)
   ...
   command = CommandBuilder.new(...)
   yield(command) if block_given?
   ...
   run command
   ...
end

as it yields the command builder instance, which probably should not be made accessible to minimagick users directly imho?!

However, here's the pull request

[mrkamel]

Hi, what do you think about this PR?

[kaspergrubbe]

I thought it was up to the garbage collector and/or the operating system to clean up temporary files?

Maybe we could make use of the open-block to help the GC clean up, eg.:

# Short-term use
 Tempfile.open('my_program') do |f|
   puts "Tempfile opened at #{f.path}"
   f.puts "Putting some text into the file"
 end

from http://ruby.about.com/od/beginningruby/a/dir4.htm

Why do you think that convert is better than mogrify in a format situation?

[mrkamel]

I thought it was up to the garbage collector and/or the operating system to clean up temporary files?

Yes, but take a look to:

require "mini_magick"

image = MiniMagick::Image.open("test.tif") # a)
image.format "jpg" # b)

Here, we want minimagick to behave similar to:
a) copy test.tif to a tempfile
b) convert the tif-tempfile to a jpg-tempfile

However, in master, minimagick currently does the following:
a) copy test.tif to a tempfile
b) convert/mogrify the tif-tempfile to a jpg-file (no tempfile!)

Take a look into the source, no tempfile is touched in Image#format.
Therefore, the jpg file will live forever, ... and fill up the disk

Why do you think that convert is better than mogrify in a format situation?

Generally speaking, Image#format receives an input file e.g. input.tif and must create e.g. output.jpg.
However, mogrify does not accept a parameter [outfile], such that convert is better suited
for this sitation. Otherwise, we introduce a dependency and lean on the fact the mogrify will only change the suffix from .tif to .jpg to choose a outfile-path. This dependency is currently present in Image#format.

Moreover, if we use tempfiles (like we do for Image#open), Image#format must create
a new tempfile for the new format (.jpg). However, ruby's tempfile library is afaik not able
to accept an arbitrary input path (only prefix and suffix can be chosen). Therefore, we must
use Tempfile.new to let the tempfile library choose a distinct temporary filename. As we
can't hand this new destination tempfile-path to mogrify, we actually have to use convert
instead of mogrify.

[sockmonk]

+1 for accepting this pull request. I'm also seeing my /tmp directory fill up with mini_magick .jpg files in the same circumstances. Looks like this solution is well researched and works.

[mjgiarlo]

This is problematic for me as well. Have the owners of this repo commented on this PR yet?

[scande3]

+1 on having this merged. When dealing with tens of thousands of large objects, the leftover temporary files filled 60GB of space causing my image processing server to run out of storage.

[jcoyne]

@mrkamel can you rebase this? It doesn't merge cleanly. Then, 👍 for merging into master.

[mrkamel]

Done.

However, Travis currently does not seem to work in general for rbx and jruby-head.

[mrkamel]

oops, don't know what happened in commit 7af2cd4. i'll create a new, fresh PR.