Repository: rubyzip/rubyzip (ruby)

Issue:
Temp files created in get_output_stream not unlinked
I have an issue when I am using rubyzip in a daemon and a lot of tempfiles are creeated (and persist) in the folder where I am creating the zip files.

This seems to be because the temp files which are created are not explicitly unlinked and remain on the disk until the ruby process ends or the tempfile objects are garbage collected.

Temp files created in get_output_stream not unlinked
I have an issue when I am using rubyzip in a daemon and a lot of tempfiles are creeated (and persist) in the folder where I am creating the zip files.

This seems to be because the temp files which are created are not explicitly unlinked and remain on the disk until the ruby process ends or the tempfile objects are garbage collected.



Discussion:
It's how works Tempfile. If you don't like it please send me patch.

I will have a look into writing a patch.
According to the Tempfile docs it is good practice:

## Good practices

### Explicit close

When a Tempfile object is garbage collected, or when the Ruby interpreter exits, its associated temporary file is automatically deleted. This means that’s it’s unnecessary to explicitly delete a Tempfile after use, though it’s good practice to do so: not explicitly deleting unused Tempfiles can potentially leave behind large amounts of tempfiles on the filesystem until they’re garbage collected. The existence of these temp files can make it harder to determine a new Tempfile filename.

Therefore, one should always call unlink or close in an ensure block, like this:

``` ruby
file = Tempfile.new('foo')
begin
   ...do something with file...
ensure
   file.close
   file.unlink   # deletes the temp file
end
```

To work around this I am using GC.start to clean up the temp files

```
def write(name,body)
  Zip::ZipFile.open(filename, Zip::ZipFile::CREATE) do |zipfile|          
    zipfile.get_output_stream(name) { |f| f.puts body }
  end
ensure
  GC.start # cleanup temporary files
end
```

I haven't found any side effects.  If I do I will post them here.

It's how works Tempfile. If you don't like it please send me patch.

I will have a look into writing a patch.
According to the Tempfile docs it is good practice:

## Good practices

### Explicit close

When a Tempfile object is garbage collected, or when the Ruby interpreter exits, its associated temporary file is automatically deleted. This means that’s it’s unnecessary to explicitly delete a Tempfile after use, though it’s good practice to do so: not explicitly deleting unused Tempfiles can potentially leave behind large amounts of tempfiles on the filesystem until they’re garbage collected. The existence of these temp files can make it harder to determine a new Tempfile filename.

Therefore, one should always call unlink or close in an ensure block, like this:

``` ruby
file = Tempfile.new('foo')
begin
   ...do something with file...
ensure
   file.close
   file.unlink   # deletes the temp file
end
```

To work around this I am using GC.start to clean up the temp files

```
def write(name,body)
  Zip::ZipFile.open(filename, Zip::ZipFile::CREATE) do |zipfile|          
    zipfile.get_output_stream(name) { |f| f.puts body }
  end
ensure
  GC.start # cleanup temporary files
end
```

I haven't found any side effects.  If I do I will post them here.


Patch:
diff --git a/lib/zip/entry.rb b/lib/zip/entry.rb
--- a/lib/zip/entry.rb
+++ b/lib/zip/entry.rb
@@ -560,6 +560,10 @@ def get_raw_input_stream(&block)
       end
     end
 
+    def clean_up
+      # By default, do nothing
+    end
+
     private
 
     def set_time(binary_dos_date, binary_dos_time)
diff --git a/lib/zip/file.rb b/lib/zip/file.rb
--- a/lib/zip/file.rb
+++ b/lib/zip/file.rb
@@ -303,6 +303,7 @@ def commit
           @entry_set.each do |e|
             e.write_to_zip_output_stream(zos)
             e.dirty = false
+            e.clean_up
           end
           zos.comment = comment
         end
diff --git a/lib/zip/streamable_stream.rb b/lib/zip/streamable_stream.rb
--- a/lib/zip/streamable_stream.rb
+++ b/lib/zip/streamable_stream.rb
@@ -44,6 +44,10 @@ def write_to_zip_output_stream(aZipOutputStream)
       aZipOutputStream.put_next_entry(self)
       get_input_stream { |is| ::Zip::IOExtras.copy_stream(aZipOutputStream, is) }
     end
+
+    def clean_up
+      @temp_file.unlink
+    end
   end
 end
 


Test patch:
diff --git a/test/file_test.rb b/test/file_test.rb
--- a/test/file_test.rb
+++ b/test/file_test.rb
@@ -89,6 +89,22 @@ def test_get_output_stream
     }
   end
 
+  def test_cleans_up_tempfiles_after_close
+    comment = "a short comment"
+
+    zf = ::Zip::File.new(EMPTY_FILENAME, ::Zip::File::CREATE)
+    zf.get_output_stream("myFile") do |os|
+      @tempfile_path = os.path
+      os.write "myFile contains just this"
+    end
+
+    assert_equal(true, File.exists?(@tempfile_path))
+
+    zf.close
+
+    assert_equal(false, File.exists?(@tempfile_path))
+  end
+
   def test_add
     srcFile = "test/data/file2.txt"
     entryName = "newEntryName.rb"