Rework Puppet::Util::Cacher to only expire using TTLs

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 <jacob@puppetlabs.com>
(cherry picked from commit d198fedf65e472b384666fc9ae3bef487852068a)

Conflicts:

	spec/integration/node/facts_spec.rb
	spec/unit/node_spec.rb

13 files changed, 72 insertions, 191 deletions

diff --git a/spec/integration/file_serving/content_spec.rb b/spec/integration/file_serving/content_spec.rb
index 5b08a6137..e2efecfa4 100755
--- a/spec/integration/file_serving/content_spec.rb
+++ b/spec/integration/file_serving/content_spec.rb
@@ -11,6 +11,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 821b6baca..d9aaa37f5 100755
--- a/spec/integration/file_serving/metadata_spec.rb
+++ b/spec/integration/file_serving/metadata_spec.rb
@@ -12,6 +12,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 81c35af4f..2390fcab1 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 b2c71e42a..78bdabce1 100755
--- a/spec/integration/node/facts_spec.rb
+++ b/spec/integration/node/facts_spec.rb
@@ -3,8 +3,6 @@ require 'spec_helper'
 
 describe Puppet::Node::Facts do
   describe "when using the indirector" do
-    after(:each) { 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 528e29545..df310b184 100755
--- a/spec/integration/resource/catalog_spec.rb
+++ b/spec/integration/resource/catalog_spec.rb
@@ -9,7 +9,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 4ee32bcc7..f452afa2e 100755
--- a/spec/integration/ssl/host_spec.rb
+++ b/spec/integration/ssl/host_spec.rb
@@ -26,7 +26,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 031562ea1..8c581cc04 100755
--- a/spec/integration/transaction/report_spec.rb
+++ b/spec/integration/transaction/report_spec.rb
@@ -4,7 +4,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 4bbc855b1..c33fdf165 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 144e82e0c..f3772749a 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 efaa76e12..4514607be 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 339054d55..33bb4d1b2 100755
--- a/spec/unit/node_spec.rb
+++ b/spec/unit/node_spec.rb
@@ -132,8 +132,6 @@ describe Puppet::Node, "when indirecting" do
     Puppet::Node.indirection.reset_terminus_class
 
     Puppet::Node.indirection.terminus_class.should == :plain
-
-    Puppet::Util::Cacher.expire
   end
 end
 
diff --git a/spec/unit/transaction/report_spec.rb b/spec/unit/transaction/report_spec.rb
index 033c4c740..0a6ab8b5f 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 fe93afd2b..16414c858 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