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

2 files changed, 17 insertions, 75 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