Merge pull request #1334 from berkshelf/sethvargo/tmpcleanup

Do not leak tempdirs

lib/berkshelf.rb

@@ -8,7 +8,6 @@
 require 'semverse'
 require 'solve'
 require 'thor'
-require 'tmpdir'
 require 'uri'
 require 'celluloid'
 
@@ -105,20 +104,6 @@ def initialize_filesystem
       end
     end
 
-    # @return [String]
-    def tmp_dir
-      File.join(berkshelf_path, 'tmp')
-    end
-
-    # Creates a temporary directory within the Berkshelf path
-    #
-    # @return [String]
-    #   path to the created temporary directory
-    def mktmpdir
-      FileUtils.mkdir_p(tmp_dir)
-      Dir.mktmpdir(nil, tmp_dir)
-    end
-
     # @return [Berkshelf::CookbookStore]
     def cookbook_store
       CookbookStore.instance

lib/berkshelf/berksfile.rb

@@ -574,48 +574,49 @@ def package(path)
     # @return [String, nil]
     #   the expanded path cookbooks were vendored to or nil if nothing was vendored
     def vendor(destination)
-      scratch          = Berkshelf.mktmpdir
-      chefignore       = nil
-      cached_cookbooks = install
+      Dir.mktmpdir do |scratch|
+        chefignore       = nil
+        cached_cookbooks = install
 
-      return nil if cached_cookbooks.empty?
+        return nil if cached_cookbooks.empty?
 
-      cached_cookbooks.each do |cookbook|
-        Berkshelf.formatter.vendor(cookbook, destination)
-        cookbook_destination = File.join(scratch, cookbook.cookbook_name)
-        FileUtils.mkdir_p(cookbook_destination)
+        cached_cookbooks.each do |cookbook|
+          Berkshelf.formatter.vendor(cookbook, destination)
+          cookbook_destination = File.join(scratch, cookbook.cookbook_name)
+          FileUtils.mkdir_p(cookbook_destination)
 
-        # Dir.glob does not support backslash as a File separator
-        src   = cookbook.path.to_s.gsub('\\', '/')
-        files = FileSyncer.glob(File.join(src, '*'))
+          # Dir.glob does not support backslash as a File separator
+          src   = cookbook.path.to_s.gsub('\\', '/')
+          files = FileSyncer.glob(File.join(src, '*'))
 
-        chefignore = Ridley::Chef::Chefignore.new(cookbook.path.to_s) rescue nil
-        chefignore.apply!(files) if chefignore
+          chefignore = Ridley::Chef::Chefignore.new(cookbook.path.to_s) rescue nil
+          chefignore.apply!(files) if chefignore
 
-        unless cookbook.compiled_metadata?
-          cookbook.compile_metadata(cookbook_destination)
+          unless cookbook.compiled_metadata?
+            cookbook.compile_metadata(cookbook_destination)
+          end
+
+          FileUtils.cp_r(files, cookbook_destination)
         end
 
-        FileUtils.cp_r(files, cookbook_destination)
+        # Don't vendor the raw metadata (metadata.rb). The raw metadata is
+        # unecessary for the client, and this is required until compiled metadata
+        # (metadata.json) takes precedence over raw metadata in the Chef-Client.
+        #
+        # We can change back to including the raw metadata in the future after
+        # this has been fixed or just remove these comments. There is no
+        # circumstance that I can currently think of where raw metadata should
+        # ever be read by the client.
+        #
+        # - Jamie
+        #
+        # See the following tickets for more information:
+        #
+        #   * https://tickets.opscode.com/browse/CHEF-4811
+        #   * https://tickets.opscode.com/browse/CHEF-4810
+        FileSyncer.sync(scratch, destination, exclude: ["**/*/metadata.rb"])
       end
 
-      # Don't vendor the raw metadata (metadata.rb). The raw metadata is
-      # unecessary for the client, and this is required until compiled metadata
-      # (metadata.json) takes precedence over raw metadata in the Chef-Client.
-      #
-      # We can change back to including the raw metadata in the future after
-      # this has been fixed or just remove these comments. There is no
-      # circumstance that I can currently think of where raw metadata should
-      # ever be read by the client.
-      #
-      # - Jamie
-      #
-      # See the following tickets for more information:
-      #
-      #   * https://tickets.opscode.com/browse/CHEF-4811
-      #   * https://tickets.opscode.com/browse/CHEF-4810
-      FileSyncer.sync(scratch, destination, exclude: ["**/*/metadata.rb"])
-
       destination
     end
 

lib/berkshelf/community_rest.rb

@@ -10,14 +10,15 @@ class << self
       #   file path to extract the contents of the target to
       #
       # @return [String]
-      def unpack(target, destination = Dir.mktmpdir)
+      def unpack(target, destination)
         if is_gzip_file(target)
           Archive::Tar::Minitar.unpack(Zlib::GzipReader.new(File.open(target, 'rb')), destination)
         elsif is_tar_file(target)
           Archive::Tar::Minitar.unpack(target, destination)
         else
           raise Berkshelf::UnknownCompressionType.new(target)
         end
+
         FileUtils.rm_rf Dir.glob("#{destination}/**/PaxHeader")
         destination
       end
@@ -100,7 +101,8 @@ def initialize(uri = V1_API, options = {})
     #   cookbook filepath, or nil if archive does not contain a cookbook
     def download(name, version)
       archive   = stream(find(name, version)[:file])
-      extracted = self.class.unpack(archive.path)
+      scratch = Dir.mktmpdir
+      extracted = self.class.unpack(archive.path, scratch)
 
       if File.cookbook?(extracted)
         extracted

lib/berkshelf/downloader.rb

@@ -15,7 +15,10 @@ def initialize(berksfile)
       @berksfile = berksfile
     end
 
-    # Download the given Berkshelf::Dependency.
+    # Download the given Berkshelf::Dependency. If the optional block is given,
+    # the temporary path to the cookbook is yielded and automatically deleted
+    # when the block returns. If no block is given, it is the responsibility of
+    # the caller to remove the tmpdir.
     #
     # @param [String] name
     # @param [String] version
@@ -25,12 +28,18 @@ def initialize(berksfile)
     # @raise [CookbookNotFound]
     #
     # @return [String]
-    def download(*args)
+    def download(*args, &block)
       options = args.last.is_a?(Hash) ? args.pop : Hash.new
       dependency, version = args
 
       sources.each do |source|
         if result = try_download(source, dependency, version)
+          if block_given?
+            value = yield result
+            FileUtils.rm_rf(result)
+            return value
+          end
+
           return result
         end
       end

lib/berkshelf/installer.rb

@@ -102,8 +102,9 @@ def install(dependency)
 
             Berkshelf.formatter.install(source, cookbook)
 
-            stash = downloader.download(name, version)
-            CookbookStore.import(name, version, stash)
+            downloader.download(name, version) do |stash|
+              CookbookStore.import(name, version, stash)
+            end
           end
         end
       end

spec/unit/berkshelf/community_rest_spec.rb

@@ -78,6 +78,7 @@
     before do
       allow(subject).to receive(:stream).with(any_args()).and_return(archive)
       allow(Berkshelf::CommunityREST).to receive(:unpack)
+        .and_return('/some/path')
     end
 
     it 'unpacks the archive' do
@@ -87,7 +88,10 @@
         headers: { 'Content-Type' => 'application/json' },
       )
 
-      expect(Berkshelf::CommunityREST).to receive(:unpack).with('/foo/bar').once.and_return('/foo/nginx')
+      expect(Berkshelf::CommunityREST).to receive(:unpack)
+        .with('/foo/bar', String)
+        .and_return('/foo/nginx')
+        .once
       expect(archive).to receive(:unlink).once
 
       subject.download('bacon', '1.0.0')