Fixing #95. I had to redesign how events were triggered; the transaction now individually triggers each subscription, so that it has control in how to respond to failures. Eventually, this will lead the way to error handling within puppet, but for now, it just allows us to trigger every appropriate subscription, whether some have failed or not.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@998 980ebf18-57e1-0310-9a29-db15c13687c0

5 files changed, 95 insertions, 127 deletions

diff --git a/lib/puppet/event.rb b/lib/puppet/event.rb
index 7c0d0c573..f6ff87007 100644
--- a/lib/puppet/event.rb
+++ b/lib/puppet/event.rb
@@ -77,12 +77,13 @@ module Puppet
 
             # Trigger the subscriptions related to an event, and then pass it up
             # as appropriate
-            def self.trigger(source, event, transaction)
+            def self.trigger(source, event)
                 type, name = self.split(source)
 
                 @subscriptions[type][name].each { |sub|
                     if sub.match?(event)
-                        sub.trigger(transaction)
+                        yield sub
+                        #sub.trigger(transaction)
                     end
                 }
             end
@@ -211,26 +212,14 @@ module Puppet
                     target = self.target
                     #Puppet.debug "'%s' matched '%s'; triggering '%s' on '%s'" %
                     #    [@source,@event,@method,target]
-                    begin
-                        if target.respond_to?(@callback)
-                            target.log "triggering %s" % @callback
-                            event = target.send(@callback)
-                        else
-                            Puppet.debug(   
-                                "'%s' of type '%s' does not respond to '%s'" %
-                                [target,target.class,@callback.inspect]
-                            )
-                        end
-                    rescue => detail
-                        # um, what the heck do i do when an object fails to
-                        # refresh?  shouldn't that result in the transaction
-                        # rolling back?  the 'onerror' metaparam will be used
-                        # to determine behaviour in that case
-                        Puppet.err "'%s' failed to %s: '%s'" %
-                            [target,@callback,detail]
-                        raise
-                        #raise "We need to roll '%s' transaction back" %
-                            #transaction
+                    if target.respond_to?(@callback)
+                        target.log "triggering %s" % @callback
+                        event = target.send(@callback)
+                    else
+                        Puppet.debug(   
+                            "'%s' of type '%s' does not respond to '%s'" %
+                            [target,target.class,@callback.inspect]
+                        )
                     end
                     transaction.triggered(target, @callback)
                 end
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 0b1e88553..182401cb5 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -1,38 +1,27 @@
 # the class that actually walks our object/state tree, collects the changes,
 # and performs them
 
-# there are two directions of walking:
-#   - first we recurse down the tree and collect changes
-#   - then we walk back up the tree through 'refresh' after the changes
-
 require 'puppet'
 require 'puppet/statechange'
 
-#---------------------------------------------------------------
 module Puppet
 class Transaction
     attr_accessor :toplevel, :component, :objects
 
-    #---------------------------------------------------------------
     # a bit of a gross hack; a global list of objects that have failed to sync,
     # so that we can verify during later syncs that our dependencies haven't
     # failed
     def Transaction.init
         @@failures = Hash.new(0)
         Puppet::Metric.init
-        @@changed = []
     end
-    #---------------------------------------------------------------
 
-    #---------------------------------------------------------------
     # for now, just store the changes for executing linearly
     # later, we might execute them as we receive them
     def change(change)
         @changes.push change
     end
-    #---------------------------------------------------------------
 
-    #---------------------------------------------------------------
     # okay, here's the deal:
     # a given transaction maps directly to a component, and each transaction
     # will only ever receive changes from its respective component
@@ -67,7 +56,6 @@ class Transaction
                     # use an array, so that changes can return more than one
                     # event if they want
                     events = [change.forward].flatten.reject { |e| e.nil? }
-                    #@@changed.push change.state.parent
                 rescue => detail
                     change.state.err "change from %s to %s failed: %s" %
                         [change.state.is_to_s, change.state.should_to_s, detail]
@@ -94,60 +82,13 @@ class Transaction
         }.flatten.reject { |child|
             child.nil? # remove empties
         }
-#        events = @changes.collect { |change|
-#            if change.is_a?(Puppet::StateChange)
-#                change.transaction = self
-#                events = nil
-#                begin
-#                    # use an array, so that changes can return more than one
-#                    # event if they want
-#                    events = [change.forward].flatten.reject { |e| e.nil? }
-#                    #@@changed.push change.state.parent
-#                rescue => detail
-#                    change.state.err "change from %s to %s failed: %s" %
-#                        [change.state.is_to_s, change.state.should_to_s, detail]
-#                    #Puppet.err("%s failed: %s" % [change.to_s,detail])
-#                    if Puppet[:debug]
-#                        puts detail.backtrace
-#                    end
-#                    next
-#                    # FIXME this should support using onerror to determine
-#                    # behaviour; or more likely, the client calling us
-#                    # should do so
-#                end
-#
-#                # This is kinda lame, because it can result in the same
-#                # object being modified multiple times, but that's difficult
-#                # to avoid as long as we're syncing each state individually.
-#                change.state.parent.cache(:synced, now)
-#
-#                unless events.nil? or (events.is_a?(Array) and events.empty?)
-#                    change.changed = true
-#                end
-#                events
-#            else
-#                puts caller
-#                raise Puppet::DevError,
-#                    "Transactions cannot handle objects of type %s" % change.class
-#            end
-#        }.flatten.reject { |event|
-#            event.nil?
-#        }
 
         Puppet.debug "Finishing transaction %s with %s changes" %
             [self.object_id, count]
-        #@triggerevents = []
-        events.each { |event|
-            object = event.source
-            #Puppet::Event::Subscriptions.propagate(object, event, self)
-            object.propagate(event, self)
-        }
 
-        #events += @triggerevents
+        self.propagate(events)
     end
-    #---------------------------------------------------------------
 
-    #---------------------------------------------------------------
     # this should only be called by a Puppet::Container object now
     # and it should only receive an array
     def initialize(objects)
@@ -165,26 +106,36 @@ class Transaction
         end
 
         @changes = []
+    end
+
+    # Respond to each of the events.  This method walks up the parent tree,
+    # triggering each parent in turn.  It's important that the transaction
+    # itself know whether a given subscription fails, so that it can respond
+    # appropriately (when we get to the point where we're responding to events).
+    def propagate(events)
+        events.each do |event|
+            source = event.source
+
+            while source
+                Puppet::Event::Subscription.trigger(source, event) do |sub|
+                    begin
+                        sub.trigger(self)
+                    rescue => detail
+                        sub.target.err "Failed to respond to %s: %s" % [event, detail]
+                        if Puppet[:debug]
+                            puts detail.backtrace
+                        end
+                    end
+                end
 
-        # change collection is in-band, and message generation is out-of-band
-        # of course, exception raising is also out-of-band
-        now = Time.now.to_i
-#        @changes = @objects.find_all { |child|
-#            child.scheduled?
-#        }.collect { |child|
-#            # these children are all Puppet::Type instances
-#            # not all of the children will return a change, and Containers
-#            # return transactions
-#            #ary = child.evaluate
-#            #ary
-#            child.evaluate
-#        }.flatten.reject { |child|
-#            child.nil? # remove empties
-#        }
+                # Reset the source if there's a parent obj
+                source = source.parent
+            end
+            #Puppet::Event::Subscriptions.propagate(object, event, self)
+        end
     end
-    #---------------------------------------------------------------
 
-    #---------------------------------------------------------------
+    # Roll all completed changes back.
     def rollback
         events = @changes.reverse.collect { |change|
             if change.is_a?(Puppet::StateChange)
@@ -196,7 +147,6 @@ class Transaction
                 #change.transaction = self
                 begin
                     change.backward
-                    #@@changed.push change.state.parent
                 rescue => detail
                     Puppet.err("%s rollback failed: %s" % [change,detail])
                     if Puppet[:debug]
@@ -220,30 +170,18 @@ class Transaction
             end
         }.flatten.reject { |e| e.nil? }
 
-        #@triggerevents = []
-        events.each { |event|
-            object = event.source
-            object.propagate(event, self)
-        }
-
-        #events += @triggerevents
+        self.propagate(events)
     end
-    #---------------------------------------------------------------
 
-    #---------------------------------------------------------------
     def triggered(object, method)
         @triggered[object][method] += 1
         #@triggerevents << ("%s_%sed" % [object.class.name.to_s, method.to_s]).intern
     end
-    #---------------------------------------------------------------
 
-    #---------------------------------------------------------------
     def triggered?(object, method)
         @triggered[object][method]
     end
-    #---------------------------------------------------------------
 end
 end
-#---------------------------------------------------------------
 
 # $Id$
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index e719c7ae7..9adcf54b4 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1845,16 +1845,6 @@ class Type < Puppet::Element
         }
     end
 
-    # Trigger any associated subscriptions, and then pass the event up to our
-    # parent.
-    def propagate(event, transaction)
-        Puppet::Event::Subscription.trigger(self, event, transaction)
-
-        if defined? @parent
-            @parent.propagate(event, transaction)
-        end
-    end
-
     def requires?(object)
         req = false
         self.eachdependency { |dep|
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index 490da4b1a..d23be0fca 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -140,16 +140,16 @@ module Puppet
 
         # Determine the user to write files as.
         def asuser
-            if @parent.should(:owner) and ! @parent.should(:owner).is_a?(Symbol)
-                writeable = Puppet::Util.asuser(@parent.should(:owner)) {
-                    FileTest.writable?(File.dirname(@parent[:path]))
+            if self.should(:owner) and ! self.should(:owner).is_a?(Symbol)
+                writeable = Puppet::Util.asuser(self.should(:owner)) {
+                    FileTest.writable?(File.dirname(self[:path]))
                 }
 
                 # If the parent directory is writeable, then we execute
                 # as the user in question.  Otherwise we'll rely on
                 # the 'owner' state to do things.
                 if writeable
-                    asuser = @parent.should(:owner)
+                    asuser = self.should(:owner)
                 end
             end
 
diff --git a/test/other/transactions.rb b/test/other/transactions.rb
index a5f881e12..c742b682c 100644
--- a/test/other/transactions.rb
+++ b/test/other/transactions.rb
@@ -232,4 +232,55 @@ class TestTransactions < Test::Unit::TestCase
 
     end
 
+    # Make sure that multiple subscriptions get triggered.
+    def test_multisubs
+        path = tempfile()
+        file1 = tempfile()
+        file2 = tempfile()
+        file = Puppet.type(:file).create(
+            :path => path,
+            :ensure => "file"
+        )
+        exec1 = Puppet.type(:exec).create(
+            :path => ENV["PATH"],
+            :command => "touch %s" % file1,
+            :refreshonly => true,
+            :subscribe => [:file, path]
+        )
+        exec2 = Puppet.type(:exec).create(
+            :path => ENV["PATH"],
+            :command => "touch %s" % file2,
+            :refreshonly => true,
+            :subscribe => [:file, path]
+        )
+
+        assert_apply(file, exec1, exec2)
+        assert(FileTest.exists?(file1), "File 1 did not get created")
+        assert(FileTest.exists?(file2), "File 2 did not get created")
+    end
+
+    # Make sure that a failed trigger doesn't result in other events not
+    # getting triggered.
+    def test_failedrefreshes
+        path = tempfile()
+        newfile = tempfile()
+        file = Puppet.type(:file).create(
+            :path => path,
+            :ensure => "file"
+        )
+        svc = Puppet.type(:service).create(
+            :name => "thisservicedoesnotexist",
+            :subscribe => [:file, path]
+        )
+        exec = Puppet.type(:exec).create(
+            :path => ENV["PATH"],
+            :command => "touch %s" % newfile,
+            :logoutput => true,
+            :refreshonly => true,
+            :subscribe => [:file, path]
+        )
+
+        assert_apply(file, svc, exec)
+        assert(FileTest.exists?(newfile), "File did not get created")
+    end
 end