16 files changed, 89 insertions, 267 deletions

diff --git a/lib/puppet/node/environment.rb b/lib/puppet/node/environment.rb
index 96fdc3c1e..f25bb65a9 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 3dddec0d4..136c9973e 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 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
diff --git a/test/lib/puppettest.rb b/test/lib/puppettest.rb
index a60092cf7..6bae80a01 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