Make FileVendor configuration specific to the two implementationssersut/port-policyfile-fix

FileVendor previously was configured by storing a closure/anonymous
function as a class instance variable. This had the following downsides:
* The API was too general, which caused a lot of code repetition
* The block was lazily evaluated, which hid errors and made testing more
  difficult
* The closures captured references to classes with references to large
  data structures, which complicates GC.

Since we've only ever had the same two implementations of FileVendor, we
can encapsulate configuration of the FileVendor factory by wrapping each
configuration option in a method. As a side benefit, arguments to these
methods will be eagerly evaluated, which makes it easier to detect
errors.

7 files changed, 40 insertions, 13 deletions

diff --git a/lib/chef/cookbook/file_system_file_vendor.rb b/lib/chef/cookbook/file_system_file_vendor.rb
index 8896e3ed30..e351ec4702 100644
--- a/lib/chef/cookbook/file_system_file_vendor.rb
+++ b/lib/chef/cookbook/file_system_file_vendor.rb
@@ -31,6 +31,9 @@ class Chef
     # non-sensical.
     class FileSystemFileVendor < FileVendor
 
+      attr_reader :cookbook_name
+      attr_reader :repo_paths
+
       def initialize(manifest, *repo_paths)
         @cookbook_name = manifest[:cookbook_name]
         @repo_paths = repo_paths.flatten
diff --git a/lib/chef/cookbook/file_vendor.rb b/lib/chef/cookbook/file_vendor.rb
index 406f23ca25..b82b52f90c 100644
--- a/lib/chef/cookbook/file_vendor.rb
+++ b/lib/chef/cookbook/file_vendor.rb
@@ -24,15 +24,39 @@ class Chef
     # This class handles fetching of cookbook files based on specificity.
     class FileVendor
 
-      def self.on_create(&block)
-        @instance_creator = block
+      @vendor_class = nil
+      @initialization_options = nil
+
+      # Configures FileVendor to use the RemoteFileVendor implementation. After
+      # calling this, subsequent calls to create_from_manifest will return a
+      # RemoteFileVendor object initialized with the given http_client
+      def self.fetch_from_remote(http_client)
+        @vendor_class = RemoteFileVendor
+        @initialization_options = http_client
+      end
+
+      def self.fetch_from_disk(cookbook_paths)
+        @vendor_class = FileSystemFileVendor
+        @initialization_options = cookbook_paths
+      end
+
+      # Returns the implementation class that is currently configured, or `nil`
+      # if one has not been configured yet.
+      def self.vendor_class
+        @vendor_class
+      end
+
+      def self.initialization_options
+        @initialization_options
       end
 
       # Factory method that creates the appropriate kind of
       # Cookbook::FileVendor to serve the contents of the manifest
       def self.create_from_manifest(manifest)
-        raise "Must call Chef::Cookbook::FileVendor.on_create before calling create_from_manifest factory" unless defined?(@instance_creator)
-        @instance_creator.call(manifest)
+        if @vendor_class.nil?
+          raise "Must configure FileVendor to use a specific implementation before creating an instance"
+        end
+        @vendor_class.new(manifest, @initialization_options)
       end
 
       # Gets the on-disk location for the given cookbook file.
diff --git a/lib/chef/cookbook/remote_file_vendor.rb b/lib/chef/cookbook/remote_file_vendor.rb
index 49de62cf65..2ddce31001 100644
--- a/lib/chef/cookbook/remote_file_vendor.rb
+++ b/lib/chef/cookbook/remote_file_vendor.rb
@@ -25,6 +25,9 @@ class Chef
     # if not available, loading them from the remote server.
     class RemoteFileVendor < FileVendor
 
+      attr_reader :rest
+      attr_reader :cookbook_name
+
       def initialize(manifest, rest)
         @manifest = manifest
         @cookbook_name = @manifest[:cookbook_name]
diff --git a/lib/chef/knife/cookbook_upload.rb b/lib/chef/knife/cookbook_upload.rb
index 9d6e0d438d..4b2b611171 100644
--- a/lib/chef/knife/cookbook_upload.rb
+++ b/lib/chef/knife/cookbook_upload.rb
@@ -184,7 +184,7 @@ class Chef
 
       def cookbook_repo
         @cookbook_loader ||= begin
-          Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, config[:cookbook_path]) }
+          Chef::Cookbook::FileVendor.fetch_from_disk(config[:cookbook_path])
           Chef::CookbookLoader.new(config[:cookbook_path])
         end
       end
diff --git a/lib/chef/policy_builder/expand_node_object.rb b/lib/chef/policy_builder/expand_node_object.rb
index 161e14ca38..6e4fa84569 100644
--- a/lib/chef/policy_builder/expand_node_object.rb
+++ b/lib/chef/policy_builder/expand_node_object.rb
@@ -56,13 +56,13 @@ class Chef
 
       def setup_run_context(specific_recipes=nil)
         if Chef::Config[:solo]
-          Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, Chef::Config[:cookbook_path]) }
+          Chef::Cookbook::FileVendor.fetch_from_disk(Chef::Config[:cookbook_path])
           cl = Chef::CookbookLoader.new(Chef::Config[:cookbook_path])
           cl.load_cookbooks
           cookbook_collection = Chef::CookbookCollection.new(cl)
           run_context = Chef::RunContext.new(node, cookbook_collection, @events)
         else
-          Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::RemoteFileVendor.new(manifest, api_service) }
+          Chef::Cookbook::FileVendor.fetch_from_remote(api_service)
           cookbook_hash = sync_cookbooks
           cookbook_collection = Chef::CookbookCollection.new(cookbook_hash)
           run_context = Chef::RunContext.new(node, cookbook_collection, @events)
diff --git a/lib/chef/policy_builder/policyfile.rb b/lib/chef/policy_builder/policyfile.rb
index 4baa6340d4..0df3dd5dd2 100644
--- a/lib/chef/policy_builder/policyfile.rb
+++ b/lib/chef/policy_builder/policyfile.rb
@@ -154,10 +154,7 @@ class Chef
       end
 
       def setup_run_context(specific_recipes=nil)
-        # TODO: This file vendor stuff is duplicated and initializing it with a
-        # block traps a reference to this object in a global context which will
-        # prevent it from getting GC'd. Simplify it.
-        Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::RemoteFileVendor.new(manifest, api_service) }
+        Chef::Cookbook::FileVendor.fetch_from_remote(http_api)
         sync_cookbooks
         cookbook_collection = Chef::CookbookCollection.new(cookbooks_to_sync)
         run_context = Chef::RunContext.new(node, cookbook_collection, events)
diff --git a/lib/chef/shell/shell_session.rb b/lib/chef/shell/shell_session.rb
index a158020116..73e6c34ebb 100644
--- a/lib/chef/shell/shell_session.rb
+++ b/lib/chef/shell/shell_session.rb
@@ -169,7 +169,7 @@ module Shell
 
     def rebuild_context
       @run_status = Chef::RunStatus.new(@node, @events)
-      Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, Chef::Config[:cookbook_path]) }
+      Chef::Cookbook::FileVendor.fetch_from_disk(Chef::Config[:cookbook_path])
       cl = Chef::CookbookLoader.new(Chef::Config[:cookbook_path])
       cl.load_cookbooks
       cookbook_collection = Chef::CookbookCollection.new(cl)
@@ -201,7 +201,7 @@ module Shell
 
     def rebuild_context
       @run_status = Chef::RunStatus.new(@node, @events)
-      Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::RemoteFileVendor.new(manifest, Chef::REST.new(Chef::Config[:server_url])) }
+      Chef::Cookbook::FileVendor.fetch_from_remote(Chef::REST.new(Chef::Config[:chef_server_url]))
       cookbook_hash = @client.sync_cookbooks
       cookbook_collection = Chef::CookbookCollection.new(cookbook_hash)
       @run_context = Chef::RunContext.new(node, cookbook_collection, @events)