(#5437) Invalidate cached TypeCollection when there was an error parsing

The caching of the TypeCollection in Puppet::Node::Environment would cause
parse errors to occur (and be reported) only once and never again, until
the file had changed on disk.  This would also cause empty catalogs to be
sent down to the agents further hiding the problem.

Now, when a file fails to parse, it will be re-parsed every time on every
following compilation, causing the parse error to be reported every time,
and preventing sending down empty catalogs to agents.

Paired-with: Nick Lewis <nick@puppetlabs.com>

4 files changed, 30 insertions, 16 deletions

diff --git a/lib/puppet/node/environment.rb b/lib/puppet/node/environment.rb
index b64fb8a1f..807479bc8 100644
--- a/lib/puppet/node/environment.rb
+++ b/lib/puppet/node/environment.rb
@@ -79,7 +79,7 @@ class Puppet::Node::Environment
     # environment has changed do we delve deeper. 
     Thread.current[:known_resource_types] = nil if (krt = Thread.current[:known_resource_types]) && krt.environment != self
     Thread.current[:known_resource_types] ||= synchronize {
-      if @known_resource_types.nil? or @known_resource_types.stale?
+      if @known_resource_types.nil? or @known_resource_types.require_reparse?
         @known_resource_types = Puppet::Resource::TypeCollection.new(self)
         @known_resource_types.perform_initial_import
       end
diff --git a/lib/puppet/resource/type_collection.rb b/lib/puppet/resource/type_collection.rb
index 347e1c0e0..4210475ad 100644
--- a/lib/puppet/resource/type_collection.rb
+++ b/lib/puppet/resource/type_collection.rb
@@ -166,12 +166,18 @@ class Puppet::Resource::TypeCollection
     end
     parser.parse
   rescue => detail
+    @parse_failed = true
+
     msg = "Could not parse for environment #{environment}: #{detail}"
     error = Puppet::Error.new(msg)
     error.set_backtrace(detail.backtrace)
     raise error
   end
 
+  def require_reparse?
+    @parse_failed || stale?
+  end
+
   def stale?
     @watched_files.values.detect { |file| file.changed? }
   end
diff --git a/spec/unit/node/environment_spec.rb b/spec/unit/node/environment_spec.rb
index 6edcce56c..6109007b9 100755
--- a/spec/unit/node/environment_spec.rb
+++ b/spec/unit/node/environment_spec.rb
@@ -85,28 +85,29 @@ describe Puppet::Node::Environment do
       @env.known_resource_types.should equal(@collection)
     end
 
-    it "should give to all threads the same collection if it didn't change" do
-      Puppet::Resource::TypeCollection.expects(:new).with(@env).returns @collection
-      @env.known_resource_types
+    it "should give to all threads using the same environment the same collection if the collection isn't stale" do
+      original_thread_type_collection = Puppet::Resource::TypeCollection.new(@env)
+      Puppet::Resource::TypeCollection.expects(:new).with(@env).returns original_thread_type_collection
+      @env.known_resource_types.should equal(original_thread_type_collection)
+
+      original_thread_type_collection.expects(:require_reparse?).returns(false)
+      Puppet::Resource::TypeCollection.stubs(:new).with(@env).returns @collection
 
       t = Thread.new {
-        @env.known_resource_types.should equal(@collection)
+        @env.known_resource_types.should equal(original_thread_type_collection)
       }
       t.join
     end
 
-    it "should give to new threads a new collection if it isn't stale" do
-      Puppet::Resource::TypeCollection.expects(:new).with(@env).returns @collection
-      @env.known_resource_types.expects(:stale?).returns(true)
-
-      Puppet::Resource::TypeCollection.expects(:new).returns @collection
+    it "should generate a new TypeCollection if the current one requires reparsing" do
+      old_type_collection = @env.known_resource_types
+      old_type_collection.stubs(:require_reparse?).returns true
+      Thread.current[:known_resource_types] = nil
+      new_type_collection = @env.known_resource_types
 
-      t = Thread.new {
-        @env.known_resource_types.should equal(@collection)
-      }
-      t.join
+      new_type_collection.should be_a Puppet::Resource::TypeCollection
+      new_type_collection.should_not equal(old_type_collection)
     end
-
   end
 
   [:modulepath, :manifestdir].each do |setting|
diff --git a/spec/unit/resource/type_collection_spec.rb b/spec/unit/resource/type_collection_spec.rb
index cf7039a51..1576c0615 100644
--- a/spec/unit/resource/type_collection_spec.rb
+++ b/spec/unit/resource/type_collection_spec.rb
@@ -387,7 +387,7 @@ describe Puppet::Resource::TypeCollection do
 
   describe "when performing initial import" do
     before do
-      @parser = stub 'parser'
+      @parser = Puppet::Parser::Parser.new("test")
       Puppet::Parser::Parser.stubs(:new).returns @parser
       @code = Puppet::Resource::TypeCollection.new("env")
     end
@@ -423,6 +423,13 @@ describe Puppet::Resource::TypeCollection do
       @parser.stubs(:file=)
       lambda { @code.perform_initial_import }.should raise_error(Puppet::Error)
     end
+
+    it "should mark the type collection as needing a reparse when there is an error parsing" do
+      @parser.expects(:parse).raises Puppet::ParseError.new("Syntax error at ...")
+
+      lambda { @code.perform_initial_import }.should raise_error(Puppet::Error, /Syntax error at .../)
+      @code.require_reparse?.should be_true
+    end
   end
 
   describe "when determining the configuration version" do