----- Original Message -----
> >
> > diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
> > index cfeb73a..00c3b18 100644
> > --- a/lib/puppet/configurer.rb
> > +++ b/lib/puppet/configurer.rb
> > @@ -156,9 +156,10 @@ class Puppet::Configurer
> > return
> > end
> > ensure
> > - # Make sure we forget the retained module_directories of any
> > autoload
> > - # we might have used.
> > + # Make sure we forget the retained module_directories and gem
> > search
> > + # path of any autoload we might have used.
> > Thread.current[:env_module_directories] = nil
> > + Thread.current[:gemsearch_directories] = nil
>
>
> This doesn't make it at all clear *why* this is being done. What
> goes wrong if that doesn't happen? Where (as in, specifically, which bits
> of code) is it important that this is flushed? Is it just important
> to reduce memory consumption, or does it have semantic effects too?
My first attempt at this was just to return the gem list. This had the effect
of pushing the test run into many many minutes before killing my machine -
ran out of swap (it never finished). So performance and memory consumption.
So I went spelunking and found the caching pattern, added it, problem gone.
>From my - extremely limited - understanding of this specific code here this
just has the effect of clearing the cache after each client run, which sounds
sane.
>
> > # Now close all of our existing http connections, since there's
> > no
> > # reason to leave them lying open.
> > diff --git a/lib/puppet/parser/compiler.rb
> > b/lib/puppet/parser/compiler.rb
> > index c1daade..68498ad 100644
> > --- a/lib/puppet/parser/compiler.rb
> > +++ b/lib/puppet/parser/compiler.rb
> > @@ -22,6 +22,7 @@ class Puppet::Parser::Compiler
> > # stick until the next entry to this function.
> > Thread.current[:known_resource_types] = nil
> > Thread.current[:env_module_directories] = nil
> > + Thread.current[:gemsearch_directories] = nil
>
> Why do these have the same semantics as the other values there?
needs to be cleared after each compile. If you install a parser function using
a gem without restarting your master you want it found.
>
> > # ...and we actually do the compile now we have caching ready.
> > new(node).compile.to_resource
> > diff --git a/lib/puppet/util/autoload.rb
> > b/lib/puppet/util/autoload.rb
> > index 6537a4a..e61f4cd 100644
> > --- a/lib/puppet/util/autoload.rb
> > +++ b/lib/puppet/util/autoload.rb
> > @@ -124,9 +124,24 @@ class Puppet::Util::Autoload
> > searchpath.map { |dir| Dir.glob("#{dir}/*.rb") }.flatten
> > end
> >
> > - # The list of directories to search through for loadable
> > plugins.
> > + # features uses the autoloader which causes a loop so avoid
> > trying to
> > + # ask Gem for its path during feature loading.
> > + #
> > + # We're using a per-thread cache based on the code in
> > #module_directories.
>
> Why are we using a per-thread cache?
cos thats the existing pattern I found in the file that had sufficient comments
so their use were clear :)
Do you want comments added to these 3 bits of code? Seems pretty clear to me
already
>
>
> Otherwise, it looks reasonably good, code-wise.
>
> On the testing front, I don't see any testing in there that tries to
> exercise the thread semantics, or verifies the life-cycle
> assumptions, or that verifies correct cache management. Given the issues we
> had
> from distant changes breaking those recently, in the area of thread
> caching, it would be important to see those.
yes, I wanted to make tests too. No I dont know how, I went looking through
the code
and recent commits (like yours in 68c106e3ef192d64eb5a1e8daa1e070774909728) but
found
that the code got changed without tests being written.
Without examples I'd have to say this is beyond my understanding of puppet
internals
to test.
>
> (If you can't work static tests to verify correctness, it would be
> ideal to see internal assertions in the code that verify the cache is
> managed correctly – so that when we mess it up in the field we get
> good diagnostics, not just randomly delayed catalog compilation or
> whatever.)
OK, this actually put me on the right track (I hope?) these tests below are
hopefully
testing the side effects to confirm the vars are managed correctly. Feedback
welcome,
if these are ok I'll send a new patch
+++ b/spec/unit/util/autoload_spec.rb
@@ -57,6 +57,16 @@ describe Puppet::Util::Autoload do
@autoload.gemsearchpaths
end
+ it "should save the gem load path to the thread local cache" do
+ Thread.current[:gemsearch_directories] = nil
+ Gem.expects(:all_load_paths).returns(["foo/loaddir"])
+
+ @autoload = Puppet::Util::Autoload.new("foo", "loaddir")
+ @autoload.gemsearchpaths
+
+ Thread.current[:gemsearch_directories].should == ["foo/loaddir"]
+ end
+
+++ b/spec/unit/configurer_spec.rb
@@ -85,6 +85,16 @@ describe Puppet::Configurer, "when executing a catalog run"
do
Puppet::Transaction::Report.indirection.stubs(:save)
end
+ it "should clear the thread local caches" do
+ Thread.current[:env_module_directories] = false
+ Thread.current[:gemsearch_directories] = false
+
+ @agent.run
+
+ Thread.current[:env_module_directories].should == nil
+ Thread.current[:gemsearch_directories].should == nil
+ end
+++ b/spec/unit/parser/compiler_spec.rb
@@ -75,6 +75,24 @@ describe Puppet::Parser::Compiler do
Puppet::Parser::Compiler.compile(@node).should equal(converted_catalog)
end
+ it "should clear the thread local caches before compile" do
+ compiler = stub 'compiler'
+ Puppet::Parser::Compiler.expects(:new).with(@node).returns compiler
+ catalog = stub 'catalog'
+ compiler.expects(:compile).returns catalog
+ catalog.expects(:to_resource)
+
+ [:known_resource_types, :env_module_directories,
:gemsearch_directories].each do |var|
+ Thread.current[var] = "rspec"
+ end
+
+ Puppet::Parser::Compiler.compile(@node)
+
+ [:known_resource_types, :env_module_directories,
:gemsearch_directories].each do |var|
+ Thread.current[var].should == nil
+ end
+ end
--
R.I.Pienaar
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
