We have removed every usage of cached_attr in which the attribute needs to be manually expired. Thus, the only meaningful behavior provided by Puppet::Util::Cacher is expiration based on TTLs. This commit reworks the cacher to only support that behavior.
Rather than accepting an options hash, of which :ttl is the only available option, cached_attr now requires a second argument, which is the TTL. TTLs are now used to compute expirations, which are stored and used for expiring values. Previously, we stored a timestamp and used it and the TTL to determine whether the attribute was expired. This had the potentially undesirable side effect that the lifetime of a cached attribute could be extended after its insertion by modifying the TTL setting for the cache. Now, the lifetime of an attribute is determined when it is set, and is thereafter immutable, aside from deliberately re-setting the expiration for that particular attribute. Reviewed-By: Jacob Helwig <[email protected]> --- lib/puppet/node/environment.rb | 10 +- lib/puppet/util/cacher.rb | 82 ++-------- spec/integration/file_serving/content_spec.rb | 2 - spec/integration/file_serving/metadata_spec.rb | 2 - spec/integration/network/server/webrick_spec.rb | 1 - spec/integration/node/facts_spec.rb | 2 - spec/integration/resource/catalog_spec.rb | 1 - spec/integration/ssl/host_spec.rb | 1 - spec/integration/transaction/report_spec.rb | 1 - spec/unit/indirector/indirection_spec.rb | 4 - spec/unit/node/environment_spec.rb | 34 +--- spec/unit/node/facts_spec.rb | 4 - spec/unit/node_spec.rb | 4 - spec/unit/transaction/report_spec.rb | 4 - spec/unit/util/cacher_spec.rb | 205 +++++++---------------- test/lib/puppettest.rb | 1 - 16 files changed, 89 insertions(+), 269 deletions(-) diff --git a/lib/puppet/node/environment.rb b/lib/puppet/node/environment.rb index 96fdc3c..f25bb65 100644 --- a/lib/puppet/node/environment.rb +++ b/lib/puppet/node/environment.rb @@ -95,7 +95,7 @@ class Puppet::Node::Environment # Cache the modulepath, so that we aren't searching through # all known directories all the time. - cached_attr(:modulepath, :ttl => Puppet[:filetimeout]) do + cached_attr(:modulepath, Puppet[:filetimeout]) do dirs = self[:modulepath].split(File::PATH_SEPARATOR) dirs = ENV["PUPPETLIB"].split(File::PATH_SEPARATOR) + dirs if ENV["PUPPETLIB"] validate_dirs(dirs) @@ -103,7 +103,7 @@ class Puppet::Node::Environment # Return all modules from this environment. # Cache the list, because it can be expensive to create. - cached_attr(:modules, :ttl => Puppet[:filetimeout]) do + cached_attr(:modules, Puppet[:filetimeout]) do module_names = modulepath.collect { |path| Dir.entries(path) }.flatten.uniq module_names.collect do |path| begin @@ -114,12 +114,6 @@ class Puppet::Node::Environment end.compact end - # Cache the manifestdir, so that we aren't searching through - # all known directories all the time. - cached_attr(:manifestdir, :ttl => Puppet[:filetimeout]) do - validate_dirs(self[:manifestdir].split(File::PATH_SEPARATOR)) - end - def to_s name.to_s end diff --git a/lib/puppet/util/cacher.rb b/lib/puppet/util/cacher.rb index 3dddec0..136c997 100644 --- a/lib/puppet/util/cacher.rb +++ b/lib/puppet/util/cacher.rb @@ -1,25 +1,6 @@ require 'monitor' module Puppet::Util::Cacher - module Expirer - attr_reader :timestamp - - # Cause all cached values to be considered expired. - def expire - @timestamp = Time.now - end - - # Is the provided timestamp earlier than our expiration timestamp? - # If it is, then the associated value is expired. - def dependent_data_expired?(ts) - return false unless timestamp - - timestamp > ts - end - end - - extend Expirer - # Our module has been extended in a class; we can only add the Instance methods, # which become *class* methods in the class. def self.extended(other) @@ -40,27 +21,26 @@ module Puppet::Util::Cacher module ClassMethods # Provide a means of defining an attribute whose value will be cached. # Must provide a block capable of defining the value if it's flushed.. - def cached_attr(name, options = {}, &block) + def cached_attr(name, ttl, &block) init_method = "init_#{name}" define_method(init_method, &block) + set_attr_ttl(name, ttl) + define_method(name) do cached_value(name) end define_method(name.to_s + "=") do |value| # Make sure the cache timestamp is set - cache_timestamp - value_cache.synchronize { value_cache[name] = value } - end - - if ttl = options[:ttl] - set_attr_ttl(name, ttl) + value_cache.synchronize do + value_cache[name] = value + set_expiration(name) + end end end def attr_ttl(name) - return nil unless @attr_ttls @attr_ttls[name] end @@ -72,57 +52,25 @@ module Puppet::Util::Cacher # Methods that get added to instances. module InstanceMethods - - def expire - # Only expire if we have an expirer. This is - # mostly so that we can comfortably handle cases - # like Puppet::Type instances, which use their - # catalog as their expirer, and they often don't - # have a catalog. - if e = expirer - e.expire - end - end - - def expirer - Puppet::Util::Cacher - end - private - def cache_timestamp - @cache_timestamp ||= Time.now - end - def cached_value(name) value_cache.synchronize do - # Allow a nil expirer, in which case we regenerate the value every time. - if expired_by_expirer?(name) - value_cache.clear - @cache_timestamp = Time.now - elsif expired_by_ttl?(name) - value_cache.delete(name) + if value_cache[name].nil? or expired_by_ttl?(name) + value_cache[name] = send("init_#{name}") + set_expiration(name) end - value_cache[name] = send("init_#{name}") unless value_cache.include?(name) value_cache[name] end end - def expired_by_expirer?(name) - if expirer.nil? - return true unless self.class.attr_ttl(name) - end - expirer.dependent_data_expired?(cache_timestamp) - end - def expired_by_ttl?(name) - return false unless self.class.respond_to?(:attr_ttl) - return false unless ttl = self.class.attr_ttl(name) - - @ttl_timestamps ||= {} - @ttl_timestamps[name] ||= Time.now + @attr_expirations[name] < Time.now + end - (Time.now - @ttl_timestamps[name]) > ttl + def set_expiration(name) + @attr_expirations ||= {} + @attr_expirations[name] = Time.now + self.class.attr_ttl(name) end def value_cache diff --git a/spec/integration/file_serving/content_spec.rb b/spec/integration/file_serving/content_spec.rb index a95ddc5..e82b726 100755 --- a/spec/integration/file_serving/content_spec.rb +++ b/spec/integration/file_serving/content_spec.rb @@ -15,6 +15,4 @@ describe Puppet::FileServing::Content, " when finding files" do @test_class = Puppet::FileServing::Content @indirection = Puppet::FileServing::Content.indirection end - - after { Puppet::Util::Cacher.expire } end diff --git a/spec/integration/file_serving/metadata_spec.rb b/spec/integration/file_serving/metadata_spec.rb index ba7d331..8b4d53d 100755 --- a/spec/integration/file_serving/metadata_spec.rb +++ b/spec/integration/file_serving/metadata_spec.rb @@ -16,6 +16,4 @@ describe Puppet::FileServing::Metadata, " when finding files" do @test_class = Puppet::FileServing::Metadata @indirection = Puppet::FileServing::Metadata.indirection end - - after { Puppet::Util::Cacher.expire } end diff --git a/spec/integration/network/server/webrick_spec.rb b/spec/integration/network/server/webrick_spec.rb index 81c35af..2390fca 100755 --- a/spec/integration/network/server/webrick_spec.rb +++ b/spec/integration/network/server/webrick_spec.rb @@ -32,7 +32,6 @@ describe Puppet::Network::Server do system("rm -rf #{@dir}") Puppet::SSL::Host.ca_location = :none - Puppet::Util::Cacher.expire end describe "before listening" do diff --git a/spec/integration/node/facts_spec.rb b/spec/integration/node/facts_spec.rb index f54d7f9..4ec4bc8 100755 --- a/spec/integration/node/facts_spec.rb +++ b/spec/integration/node/facts_spec.rb @@ -7,8 +7,6 @@ require 'spec_helper' describe Puppet::Node::Facts do describe "when using the indirector" do - after { Puppet::Util::Cacher.expire } - it "should expire any cached node instances when it is saved" do Puppet::Node::Facts.indirection.stubs(:terminus_class).returns :yaml diff --git a/spec/integration/resource/catalog_spec.rb b/spec/integration/resource/catalog_spec.rb index 41a475d..ae15ee4 100755 --- a/spec/integration/resource/catalog_spec.rb +++ b/spec/integration/resource/catalog_spec.rb @@ -13,7 +13,6 @@ describe Puppet::Resource::Catalog do end describe "when using the indirector" do - after { Puppet::Util::Cacher.expire } before do # This is so the tests work w/out networking. Facter.stubs(:to_hash).returns({"hostname" => "foo.domain.com"}) diff --git a/spec/integration/ssl/host_spec.rb b/spec/integration/ssl/host_spec.rb index 2da5bcf..53ff88e 100755 --- a/spec/integration/ssl/host_spec.rb +++ b/spec/integration/ssl/host_spec.rb @@ -30,7 +30,6 @@ describe Puppet::SSL::Host, :fails_on_windows => true do system("rm -rf #{@dir}") Puppet.settings.clear - Puppet::Util::Cacher.expire } it "should be considered a CA host if its name is equal to 'ca'" do diff --git a/spec/integration/transaction/report_spec.rb b/spec/integration/transaction/report_spec.rb index 183d93f..315c0c0 100755 --- a/spec/integration/transaction/report_spec.rb +++ b/spec/integration/transaction/report_spec.rb @@ -8,7 +8,6 @@ require 'spec_helper' describe Puppet::Transaction::Report do describe "when using the indirector" do after do - Puppet::Util::Cacher.expire Puppet.settings.stubs(:use) end diff --git a/spec/unit/indirector/indirection_spec.rb b/spec/unit/indirector/indirection_spec.rb index 4bbc855..c33fdf1 100755 --- a/spec/unit/indirector/indirection_spec.rb +++ b/spec/unit/indirector/indirection_spec.rb @@ -102,9 +102,6 @@ shared_examples_for "Delegation Authorizer" do end describe Puppet::Indirector::Indirection do - after do - Puppet::Util::Cacher.expire - end describe "when initializing" do # (LAK) I've no idea how to test this, really. it "should store a reference to itself before it consumes its options" do @@ -643,7 +640,6 @@ describe Puppet::Indirector::Indirection do after :each do @indirection.delete - Puppet::Util::Cacher.expire end end diff --git a/spec/unit/node/environment_spec.rb b/spec/unit/node/environment_spec.rb index 144e82e..f377274 100755 --- a/spec/unit/node/environment_spec.rb +++ b/spec/unit/node/environment_spec.rb @@ -1,6 +1,8 @@ #!/usr/bin/env rspec require 'spec_helper' +require 'tmpdir' + require 'puppet/node/environment' require 'puppet/util/execution' @@ -10,10 +12,6 @@ describe Puppet::Node::Environment do Puppet::Node::Environment.clear end - it "should include the Cacher module" do - Puppet::Node::Environment.ancestors.should be_include(Puppet::Util::Cacher) - end - it "should use the filetimeout for the ttl for the modulepath" do Puppet::Node::Environment.attr_ttl(:modulepath).should == Integer(Puppet[:filetimeout]) end @@ -22,10 +20,6 @@ describe Puppet::Node::Environment do Puppet::Node::Environment.attr_ttl(:modules).should == Integer(Puppet[:filetimeout]) end - it "should use the filetimeout for the ttl for the manifestdir" do - Puppet::Node::Environment.attr_ttl(:manifestdir).should == Integer(Puppet[:filetimeout]) - end - it "should use the default environment if no name is provided while initializing an environment" do Puppet.settings.expects(:value).with(:environment).returns("one") Puppet::Node::Environment.new.name.should == :one @@ -109,27 +103,15 @@ describe Puppet::Node::Environment do end end - [:modulepath, :manifestdir].each do |setting| - it "should validate the #{setting} directories" do - path = %w{/one /two}.join(File::PATH_SEPARATOR) - - env = Puppet::Node::Environment.new("testing") - env.stubs(:[]).with(setting).returns path - - env.expects(:validate_dirs).with(%w{/one /two}) - - env.send(setting) - end + it "should validate the modulepath directories" do + real_file = Dir.mktmpdir + path = %W[/one /two #{real_file}].join(File::PATH_SEPARATOR) - it "should return the validated dirs for #{setting}" do - path = %w{/one /two}.join(File::PATH_SEPARATOR) + Puppet[:modulepath] = path - env = Puppet::Node::Environment.new("testing") - env.stubs(:[]).with(setting).returns path - env.stubs(:validate_dirs).returns %w{/one /two} + env = Puppet::Node::Environment.new("testing") - env.send(setting).should == %w{/one /two} - end + env.modulepath.should == [real_file] end it "should prefix the value of the 'PUPPETLIB' environment variable to the module path if present" do diff --git a/spec/unit/node/facts_spec.rb b/spec/unit/node/facts_spec.rb index 6d2b0a7..b3a0f92 100755 --- a/spec/unit/node/facts_spec.rb +++ b/spec/unit/node/facts_spec.rb @@ -68,10 +68,6 @@ describe Puppet::Node::Facts, "when indirecting" do before do @indirection = stub 'indirection', :request => mock('request'), :name => :facts - # We have to clear the cache so that the facts ask for our indirection stub, - # instead of anything that might be cached. - Puppet::Util::Cacher.expire - @facts = Puppet::Node::Facts.new("me", "one" => "two") end diff --git a/spec/unit/node_spec.rb b/spec/unit/node_spec.rb index 476b0d6..5f3e3b4 100755 --- a/spec/unit/node_spec.rb +++ b/spec/unit/node_spec.rb @@ -207,10 +207,6 @@ describe Puppet::Node, "when indirecting" do it "should not have a cache class defined" do Puppet::Node.indirection.cache_class.should be_nil end - - after do - Puppet::Util::Cacher.expire - end end describe Puppet::Node, "when generating the list of names to search through" do diff --git a/spec/unit/transaction/report_spec.rb b/spec/unit/transaction/report_spec.rb index 033c4c7..0a6ab8b 100755 --- a/spec/unit/transaction/report_spec.rb +++ b/spec/unit/transaction/report_spec.rb @@ -103,10 +103,6 @@ describe Puppet::Transaction::Report do report.expects(:host).returns "me" report.name.should == "me" end - - after do - Puppet::Util::Cacher.expire - end end describe "when computing exit status" do diff --git a/spec/unit/util/cacher_spec.rb b/spec/unit/util/cacher_spec.rb index fe93afd..16414c8 100755 --- a/spec/unit/util/cacher_spec.rb +++ b/spec/unit/util/cacher_spec.rb @@ -3,182 +3,105 @@ require 'spec_helper' require 'puppet/util/cacher' -class ExpirerTest - include Puppet::Util::Cacher::Expirer -end - class CacheTest - @@init_count = 0 - - include Puppet::Util::Cacher - cached_attr(:instance_cache) { Time.now } -end + @@count = 0 -describe Puppet::Util::Cacher::Expirer do - before do - @expirer = ExpirerTest.new + def self.count + @@count end - it "should be able to test whether a timestamp is expired" do - @expirer.should respond_to(:dependent_data_expired?) - end - - it "should be able to expire all values" do - @expirer.should respond_to(:expire) - end - - it "should consider any value to be valid if it has never been expired" do - @expirer.should_not be_dependent_data_expired(Time.now) - end + include Puppet::Util::Cacher - it "should consider any value created after expiration to be expired" do - @expirer.expire - @expirer.should be_dependent_data_expired(Time.now - 1) + cached_attr(:instance_cache, 10) do + @@count += 1 + {:number => @@count} end end describe Puppet::Util::Cacher do - it "should be extended with the Expirer module" do - Puppet::Util::Cacher.singleton_class.ancestors.should be_include(Puppet::Util::Cacher::Expirer) + before :each do + CacheTest.set_attr_ttl(:instance_cache, 10) + @object = CacheTest.new end - it "should support defining cached attributes", :'fails_on_ruby_1.9.2' => true do - CacheTest.methods.should be_include("cached_attr") + it "should return a value calculated from the provided block" do + @object.instance_cache.should == {:number => CacheTest.count} end - it "should default to the Cacher module as its expirer" do - CacheTest.new.expirer.should equal(Puppet::Util::Cacher) + it "should return the cached value from the getter every time if the value is not expired" do + @object.instance_cache.should equal(@object.instance_cache) end - describe "when using cached attributes" do - before do - @expirer = ExpirerTest.new - @object = CacheTest.new + it "should regenerate and return a new value using the provided block if the value has expired" do + initial = @object.instance_cache - @object.stubs(:expirer).returns @expirer - end - - it "should create a getter for the cached attribute" do - @object.should respond_to(:instance_cache) - end - - it "should return a value calculated from the provided block" do - time = Time.now - Time.stubs(:now).returns time - @object.instance_cache.should equal(time) - end + # Ensure the value is expired immediately + CacheTest.set_attr_ttl(:instance_cache, -10) + @object.send(:set_expiration, :instance_cache) - it "should return the cached value from the getter every time if the value is not expired" do - @object.instance_cache.should equal(@object.instance_cache) - end - - it "should regenerate and return a new value using the provided block if the value has been expired" do - value = @object.instance_cache - @expirer.expire - @object.instance_cache.should_not equal(value) - end + @object.instance_cache.should_not equal(initial) + end - it "should be able to trigger expiration on its expirer" do - @expirer.expects(:expire) - @object.expire - end + it "should be able to cache false values" do + @object.expects(:init_instance_cache).once.returns false + @object.instance_cache.should be_false + @object.instance_cache.should be_false + end - it "should do nothing when asked to expire when no expirer is available" do - cacher = CacheTest.new - class << cacher - def expirer - nil - end - end - lambda { cacher.expire }.should_not raise_error - end + it "should cache values again after expiration" do + initial = @object.instance_cache - it "should be able to cache false values" do - @object.expects(:init_instance_cache).returns false - @object.instance_cache.should be_false - @object.instance_cache.should be_false - end + # Ensure the value is expired immediately + CacheTest.set_attr_ttl(:instance_cache, -10) + @object.send(:set_expiration, :instance_cache) - it "should cache values again after expiration" do - @object.instance_cache - @expirer.expire - @object.instance_cache.should equal(@object.instance_cache) - end + # Reset ttl so this new value doesn't get expired + CacheTest.set_attr_ttl(:instance_cache, 10) + after_expiration = @object.instance_cache - it "should always consider a value expired if it has no expirer" do - @object.stubs(:expirer).returns nil - @object.instance_cache.should_not equal(@object.instance_cache) - end + after_expiration.should_not == initial + @object.instance_cache.should == after_expiration + end - it "should allow writing of the attribute" do - @object.should respond_to(:instance_cache=) - end + it "should allow writing of the attribute" do + initial = @object.instance_cache - it "should correctly configure timestamps for expiration when the cached attribute is written to" do - @object.instance_cache = "foo" - @expirer.expire - @object.instance_cache.should_not == "foo" - end + @object.instance_cache = "another value" + @object.instance_cache.should == "another value" + end - it "should allow specification of a ttl for cached attributes" do - klass = Class.new do - include Puppet::Util::Cacher - end + it "should update the expiration when the cached attribute is set manually" do + # Freeze time + now = Time.now + Time.stubs(:now).returns now - klass.cached_attr(:myattr, :ttl => 5) { Time.now } + @object.instance_cache - klass.attr_ttl(:myattr).should == 5 - end + # Set expiration to something far in the future + CacheTest.set_attr_ttl(:instance_cache, 60) + @object.send(:set_expiration, :instance_cache) - it "should allow specification of a ttl as a string" do - klass = Class.new do - include Puppet::Util::Cacher - end + CacheTest.set_attr_ttl(:instance_cache, 10) - klass.cached_attr(:myattr, :ttl => "5") { Time.now } + @object.instance_cache = "foo" + @object.instance_variable_get(:@attr_expirations)[:instance_cache].should == now + 10 + end - klass.attr_ttl(:myattr).should == 5 + it "should allow specification of a ttl as a string" do + klass = Class.new do + include Puppet::Util::Cacher end - it "should fail helpfully if the ttl cannot be converted to an integer" do - klass = Class.new do - include Puppet::Util::Cacher - end - - lambda { klass.cached_attr(:myattr, :ttl => "yep") { Time.now } }.should raise_error(ArgumentError) - end + klass.cached_attr(:myattr, "5") { 10 } - it "should not check for a ttl expiration if the class does not support that method" do - klass = Class.new do - extend Puppet::Util::Cacher - end + klass.attr_ttl(:myattr).should == 5 + end - klass.singleton_class.cached_attr(:myattr) { "eh" } - klass.myattr + it "should fail helpfully if the ttl cannot be converted to an integer" do + klass = Class.new do + include Puppet::Util::Cacher end - it "should automatically expire cached attributes whose ttl has expired, even if no expirer is present" do - klass = Class.new do - def self.to_s - "CacheTestClass" - end - include Puppet::Util::Cacher - attr_accessor :value - end - - klass.cached_attr(:myattr, :ttl => 5) { self.value += 1; self.value } - - now = Time.now - later = Time.now + 15 - - instance = klass.new - instance.value = 0 - instance.myattr.should == 1 - - Time.expects(:now).returns later - - # This call should get the new Time value, which should expire the old value - instance.myattr.should == 2 - end + lambda { klass.cached_attr(:myattr, "yep") { 10 } }.should raise_error(ArgumentError) end end diff --git a/test/lib/puppettest.rb b/test/lib/puppettest.rb index a60092c..6bae80a 100755 --- a/test/lib/puppettest.rb +++ b/test/lib/puppettest.rb @@ -280,7 +280,6 @@ module PuppetTest Puppet::Util::Storage.clear Puppet.clear Puppet.settings.clear - Puppet::Util::Cacher.expire @memoryatend = Puppet::Util.memory diff = @memoryatend - @memoryatstart -- 1.7.5.4 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
