From 05d4653cef1c1d8d77228de26d55cf6d6a0ce20b Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Thu, 28 Apr 2011 07:58:58 -0700 Subject: Revert "Revert "Handle enabling/disabling observers at different levels of the class hierarchy."" This reverts commit 2a25c5818b03d7d6cd63aad180bff23479dbd861. I'm going to add another commit that keeps the same behavior of fixes the problems of leaking memory in development. --- activemodel/lib/active_model/observer_array.rb | 34 ++++++++++++++++++++-- activemodel/lib/active_model/observing.rb | 5 ++++ activemodel/test/cases/observer_array_test.rb | 39 ++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 3 deletions(-) (limited to 'activemodel') diff --git a/activemodel/lib/active_model/observer_array.rb b/activemodel/lib/active_model/observer_array.rb index b8aa9cc1e2..f3b5811b81 100644 --- a/activemodel/lib/active_model/observer_array.rb +++ b/activemodel/lib/active_model/observer_array.rb @@ -48,7 +48,7 @@ module ActiveModel set_enablement(true, observers, &block) end - private + protected def observer_class_for(observer) return observer if observer.is_a?(Class) @@ -61,13 +61,37 @@ module ActiveModel end end + def start_transaction + disabled_observer_stack.push(disabled_observers.dup) + each_subclass_array do |array| + array.start_transaction + end + end + + def disabled_observer_stack + @disabled_observer_stack ||= [] + end + + def end_transaction + @disabled_observers = disabled_observer_stack.pop + each_subclass_array do |array| + array.end_transaction + end + end + def transaction - orig_disabled_observers = disabled_observers.dup + start_transaction begin yield ensure - @disabled_observers = orig_disabled_observers + end_transaction + end + end + + def each_subclass_array + model_class.subclasses.each do |subclass| + yield self.class.for(subclass) end end @@ -92,6 +116,10 @@ module ActiveModel disabled_observers << klass end end + + each_subclass_array do |array| + array.set_enablement(enabled, observers) + end end end end diff --git a/activemodel/lib/active_model/observing.rb b/activemodel/lib/active_model/observing.rb index e1a2ce218d..ba6be46670 100644 --- a/activemodel/lib/active_model/observing.rb +++ b/activemodel/lib/active_model/observing.rb @@ -70,6 +70,10 @@ module ActiveModel observer_instances.size end + def subclasses + @subclasses ||= [] + end + protected def instantiate_observer(observer) #:nodoc: # string/symbol @@ -89,6 +93,7 @@ module ActiveModel # Notify observers when the observed class is subclassed. def inherited(subclass) super + subclasses << subclass notify_observers :observed_class_inherited, subclass end end diff --git a/activemodel/test/cases/observer_array_test.rb b/activemodel/test/cases/observer_array_test.rb index 215ca80bb4..38e4fd59fc 100644 --- a/activemodel/test/cases/observer_array_test.rb +++ b/activemodel/test/cases/observer_array_test.rb @@ -118,5 +118,44 @@ class ObserverArrayTest < ActiveModel::TestCase ORM.observers.disable Widget end end + + test "allows #enable at the superclass level to override #disable at the subclass level when called last" do + Widget.observers.disable :all + ORM.observers.enable :all + + assert_observer_notified Widget, WidgetObserver + assert_observer_notified Budget, BudgetObserver + assert_observer_notified Widget, AuditTrail + assert_observer_notified Budget, AuditTrail + end + + test "allows #disable at the superclass level to override #enable at the subclass level when called last" do + Budget.observers.enable :audit_trail + ORM.observers.disable :audit_trail + + assert_observer_notified Widget, WidgetObserver + assert_observer_notified Budget, BudgetObserver + assert_observer_not_notified Widget, AuditTrail + assert_observer_not_notified Budget, AuditTrail + end + + test "can use the block form at different levels of the hierarchy" do + yielded = false + Widget.observers.disable :all + + ORM.observers.enable :all do + yielded = true + assert_observer_notified Widget, WidgetObserver + assert_observer_notified Budget, BudgetObserver + assert_observer_notified Widget, AuditTrail + assert_observer_notified Budget, AuditTrail + end + + assert yielded + assert_observer_not_notified Widget, WidgetObserver + assert_observer_notified Budget, BudgetObserver + assert_observer_not_notified Widget, AuditTrail + assert_observer_notified Budget, AuditTrail + end end -- cgit v1.2.3 From 9a385394acd6599ff588daad491e9e07ee716091 Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Thu, 28 Apr 2011 08:07:08 -0700 Subject: Fix dev env memory leaks by using AS::DescendantsTracker rather than keeping track of subclasses manually. There's also no need to keep track of all ObserverArray instances in a hash, as this is likely to leak memory, too. --- activemodel/lib/active_model/observer_array.rb | 18 +++++----------- activemodel/lib/active_model/observing.rb | 30 ++++++-------------------- 2 files changed, 12 insertions(+), 36 deletions(-) (limited to 'activemodel') diff --git a/activemodel/lib/active_model/observer_array.rb b/activemodel/lib/active_model/observer_array.rb index f3b5811b81..d501215dd6 100644 --- a/activemodel/lib/active_model/observer_array.rb +++ b/activemodel/lib/active_model/observer_array.rb @@ -4,15 +4,6 @@ module ActiveModel # Stores the enabled/disabled state of individual observers for # a particular model classes. class ObserverArray < Array - INSTANCES = Hash.new do |hash, model_class| - hash[model_class] = new(model_class) - end - - def self.for(model_class) - return nil unless model_class < ActiveModel::Observing - INSTANCES[model_class] - end - # returns false if: # - the ObserverArray for the given model's class has the given observer # in its disabled_observers set. @@ -22,7 +13,8 @@ module ActiveModel observer_class = observer.class loop do - break unless array = self.for(klass) + break unless klass.respond_to?(:observers) + array = klass.observers return false if array.disabled_observers.include?(observer_class) klass = klass.superclass end @@ -90,8 +82,8 @@ module ActiveModel end def each_subclass_array - model_class.subclasses.each do |subclass| - yield self.class.for(subclass) + model_class.descendants.each do |subclass| + yield subclass.observers end end @@ -102,7 +94,7 @@ module ActiveModel yield end else - observers = ActiveModel::Observer.all_observers if observers == [:all] + observers = ActiveModel::Observer.descendants if observers == [:all] observers.each do |obs| klass = observer_class_for(obs) diff --git a/activemodel/lib/active_model/observing.rb b/activemodel/lib/active_model/observing.rb index ba6be46670..9ffcda8dd7 100644 --- a/activemodel/lib/active_model/observing.rb +++ b/activemodel/lib/active_model/observing.rb @@ -5,11 +5,16 @@ require 'active_support/core_ext/module/aliasing' require 'active_support/core_ext/module/remove_method' require 'active_support/core_ext/string/inflections' require 'active_support/core_ext/enumerable' +require 'active_support/descendants_tracker' module ActiveModel module Observing extend ActiveSupport::Concern + included do + extend ActiveSupport::DescendantsTracker + end + module ClassMethods # == Active Model Observers Activation # @@ -37,7 +42,7 @@ module ActiveModel # Gets the current observers. def observers - @observers ||= ObserverArray.for(self) + @observers ||= ObserverArray.new(self) end # Gets the current observer instances. @@ -70,10 +75,6 @@ module ActiveModel observer_instances.size end - def subclasses - @subclasses ||= [] - end - protected def instantiate_observer(observer) #:nodoc: # string/symbol @@ -93,7 +94,6 @@ module ActiveModel # Notify observers when the observed class is subclassed. def inherited(subclass) super - subclasses << subclass notify_observers :observed_class_inherited, subclass end end @@ -176,6 +176,7 @@ module ActiveModel # class Observer include Singleton + extend ActiveSupport::DescendantsTracker class << self # Attaches the observer to the supplied model classes. @@ -208,23 +209,6 @@ module ActiveModel nil end end - - def subclasses - @subclasses ||= [] - end - - # List of all observer subclasses, sub-subclasses, etc. - # Necessary so we can disable or enable all observers. - def all_observers - subclasses.each_with_object(subclasses.dup) do |subclass, array| - array.concat(subclass.all_observers) - end - end - end - - def self.inherited(subclass) - subclasses << subclass - super end # Start observing the declared classes and their subclasses. -- cgit v1.2.3 From fef22157b07f101229d29544d578bfe2cb9fedfe Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Thu, 28 Apr 2011 08:27:15 -0700 Subject: Fix bug with AM::Observer disablement. Now that we propagate the enabling/disabling to descendants, we no longer have to check the disabled_observer Set on each superclass of the model class. This was causing a bug when disabling all observers at a superclass level and then enabling an individual observer at a subclass level. Plus the logic is simpler now :). --- activemodel/lib/active_model/observer_array.rb | 30 +++++++------------------- activemodel/lib/active_model/observing.rb | 12 ++++++++--- activemodel/test/cases/observer_array_test.rb | 10 +++++++++ 3 files changed, 27 insertions(+), 25 deletions(-) (limited to 'activemodel') diff --git a/activemodel/lib/active_model/observer_array.rb b/activemodel/lib/active_model/observer_array.rb index d501215dd6..ab7f86007f 100644 --- a/activemodel/lib/active_model/observer_array.rb +++ b/activemodel/lib/active_model/observer_array.rb @@ -4,34 +4,16 @@ module ActiveModel # Stores the enabled/disabled state of individual observers for # a particular model classes. class ObserverArray < Array - # returns false if: - # - the ObserverArray for the given model's class has the given observer - # in its disabled_observers set. - # - or that is the case at any level of the model's superclass chain. - def self.observer_enabled?(observer, model) - klass = model.class - observer_class = observer.class - - loop do - break unless klass.respond_to?(:observers) - array = klass.observers - return false if array.disabled_observers.include?(observer_class) - klass = klass.superclass - end - - true # observers are enabled by default - end - - def disabled_observers - @disabled_observers ||= Set.new - end - attr_reader :model_class def initialize(model_class, *args) @model_class = model_class super(*args) end + def disabled_for?(observer) + disabled_observers.include?(observer.class) + end + def disable(*observers, &block) set_enablement(false, observers, &block) end @@ -42,6 +24,10 @@ module ActiveModel protected + def disabled_observers + @disabled_observers ||= Set.new + end + def observer_class_for(observer) return observer if observer.is_a?(Class) diff --git a/activemodel/lib/active_model/observing.rb b/activemodel/lib/active_model/observing.rb index 9ffcda8dd7..c1ac4eb4af 100644 --- a/activemodel/lib/active_model/observing.rb +++ b/activemodel/lib/active_model/observing.rb @@ -222,9 +222,9 @@ module ActiveModel # Send observed_method(object) if the method exists. def update(observed_method, object) #:nodoc: - if respond_to?(observed_method) && ObserverArray.observer_enabled?(self, object) - send(observed_method, object) - end + return unless respond_to?(observed_method) + return if disabled_for?(object) + send(observed_method, object) end # Special method sent by the observed class when it is inherited. @@ -238,5 +238,11 @@ module ActiveModel def add_observer!(klass) #:nodoc: klass.add_observer(self) end + + def disabled_for?(object) + klass = object.class + return false unless klass.respond_to?(:observers) + klass.observers.disabled_for?(self) + end end end diff --git a/activemodel/test/cases/observer_array_test.rb b/activemodel/test/cases/observer_array_test.rb index 38e4fd59fc..3ede5682b4 100644 --- a/activemodel/test/cases/observer_array_test.rb +++ b/activemodel/test/cases/observer_array_test.rb @@ -65,6 +65,16 @@ class ObserverArrayTest < ActiveModel::TestCase assert_observer_notified Budget, AuditTrail end + test "can enable observers on individual models without affecting those observers on other models" do + ORM.observers.disable :all + Budget.observers.enable AuditTrail + + assert_observer_not_notified Widget, WidgetObserver + assert_observer_not_notified Budget, BudgetObserver + assert_observer_not_notified Widget, AuditTrail + assert_observer_notified Budget, AuditTrail + end + test "can disable observers for the duration of a block" do yielded = false ORM.observers.disable :budget_observer do -- cgit v1.2.3 From 7db7aa505375db75c2205f35df743becc7647c95 Mon Sep 17 00:00:00 2001 From: Myron Marston Date: Thu, 28 Apr 2011 08:32:22 -0700 Subject: Add additional tests for AM::ObserverArray that I had missed yesterday. --- activemodel/test/cases/observer_array_test.rb | 51 ++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) (limited to 'activemodel') diff --git a/activemodel/test/cases/observer_array_test.rb b/activemodel/test/cases/observer_array_test.rb index 3ede5682b4..fc5f18008b 100644 --- a/activemodel/test/cases/observer_array_test.rb +++ b/activemodel/test/cases/observer_array_test.rb @@ -38,6 +38,16 @@ class ObserverArrayTest < ActiveModel::TestCase assert_observer_notified Budget, AuditTrail end + test "can enable individual observers using a class constant" do + ORM.observers.disable :all + ORM.observers.enable AuditTrail + + assert_observer_not_notified Widget, WidgetObserver + assert_observer_not_notified Budget, BudgetObserver + assert_observer_notified Widget, AuditTrail + assert_observer_notified Budget, AuditTrail + end + test "can disable individual observers using a symbol" do ORM.observers.disable :budget_observer @@ -47,6 +57,35 @@ class ObserverArrayTest < ActiveModel::TestCase assert_observer_notified Budget, AuditTrail end + test "can enable individual observers using a symbol" do + ORM.observers.disable :all + ORM.observers.enable :audit_trail + + assert_observer_not_notified Widget, WidgetObserver + assert_observer_not_notified Budget, BudgetObserver + assert_observer_notified Widget, AuditTrail + assert_observer_notified Budget, AuditTrail + end + + test "can disable multiple observers at a time" do + ORM.observers.disable :widget_observer, :budget_observer + + assert_observer_not_notified Widget, WidgetObserver + assert_observer_not_notified Budget, BudgetObserver + assert_observer_notified Widget, AuditTrail + assert_observer_notified Budget, AuditTrail + end + + test "can enable multiple observers at a time" do + ORM.observers.disable :all + ORM.observers.enable :widget_observer, :budget_observer + + assert_observer_notified Widget, WidgetObserver + assert_observer_notified Budget, BudgetObserver + assert_observer_not_notified Widget, AuditTrail + assert_observer_not_notified Budget, AuditTrail + end + test "can disable all observers using :all" do ORM.observers.disable :all @@ -56,7 +95,17 @@ class ObserverArrayTest < ActiveModel::TestCase assert_observer_not_notified Budget, AuditTrail end - test "can disable observers on individual models without affecting observers on other models" do + test "can enable all observers using :all" do + ORM.observers.disable :all + ORM.observers.enable :all + + assert_observer_notified Widget, WidgetObserver + assert_observer_notified Budget, BudgetObserver + assert_observer_notified Widget, AuditTrail + assert_observer_notified Budget, AuditTrail + end + + test "can disable observers on individual models without affecting those observers on other models" do Widget.observers.disable :all assert_observer_not_notified Widget, WidgetObserver -- cgit v1.2.3