Permalink
Browse files

Handle enabling/disabling observers at different levels of the class …

…hierarchy.

Last call wins.
  • Loading branch information...
1 parent 1f8cc44 commit ad62f1928768bd2676958a4a08512bad342fe469 @myronmarston myronmarston committed with dhh Apr 27, 2011
@@ -48,7 +48,7 @@ def enable(*observers, &block)
set_enablement(true, observers, &block)
end
- private
+ protected
def observer_class_for(observer)
return observer if observer.is_a?(Class)
@@ -61,13 +61,37 @@ def observer_class_for(observer)
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 @@ def set_enablement(enabled, observers)
disabled_observers << klass
end
end
+
+ each_subclass_array do |array|
+ array.set_enablement(enabled, observers)
+ end
end
end
end
@@ -70,6 +70,10 @@ def count_observers
observer_instances.size
end
+ def subclasses
@josevalim
josevalim Apr 28, 2011 Member

This is going to leak memory like crazy in development.

@josevalim
josevalim Apr 28, 2011 Member

Instead of keeping the subclasses to propagate changes, make the subclass ask the parent if something is disabled or not. I am going to revert this commit temporarily until a better fix is provided.

@myronmarston
myronmarston Apr 28, 2011 Contributor

I didn't really think through class reloading in development :(.

That said, I'm not sure how your proposed change would work. The reason I added this to keep track of the subclasses is because @dhh wanted last call to win, so in a case like:

Post.observers.disable :all

#... later
ActiveRecord::Base.observers.enable :all

...the desired behavior is that the enable :all on AR::Base will propagate to subclasses since it was called last. I'm not sure how to do that without keeping track of subclasses.

Do either of you have a recommendation for a different implementation to achieve the desired behavior?

@josevalim
josevalim Apr 28, 2011 Member

Right. If you really need to use descendants, use ActiveSupport::DescendantsTracker that will handle class reloading on development for you. When sending a pull request, remember to revert my revert first. :) Thanks for looking into this!

@myronmarston
myronmarston Apr 28, 2011 Contributor

If you really need to use descendants, use ActiveSupport::DescendantsTracker that will handle class reloading on development for you.

Thanks for the tip! I didn't even know about DescendantsTracker. Since AR::Base is already extending that module and also includes ActiveModel::Observing, I was thinking I should remove it from AR::Base and extend it in the included hook of ActiveModel::Observing so that AR::Base doesn't get it twice, and every includer of ActiveModel::Observing will get it. Is that the right approach?

This conversation made me think that maybe some of the code in my prior commit will have the same kind of leaking problem:

https://github.com/rails/rails/blob/1f8cc446d9a7ab751a2def65309ac4bc71e85cd3/activemodel/lib/active_model/observer_array.rb#L7-14

The INSTANCES hash keeps track of the ObserverArray instance for a given model class, and makes it easy to get the instance using the for class method. This could potentially have the same kind of memory leaking issues in development, I think...but I've got an idea for how to keep the same behavior without needing the hash, though.

@josevalim
josevalim Apr 28, 2011 Member

Right. DescendantsTracker is also included in AS::Callbacks so it will be included several times anyway. We just need to be careful to not do a lot of work in the self.included hook of DescendantsTracker, because that will certainly be executed more than once.

@myronmarston
myronmarston Apr 28, 2011 Contributor

@josevalim: I have it fixed in my am_disabling_fix_memory_leaks branch (along with a fix for another bug I just noticed!), but I'm getting a 500 error from github when I try to open a pull request. Can you merge that in?

Thanks for helping me make this code even better :).

@josevalim
josevalim Apr 28, 2011 Member

Done! Thanks for your work!

+ @subclasses ||= []
+ end
+
protected
def instantiate_observer(observer) #:nodoc:
# string/symbol
@@ -89,6 +93,7 @@ def instantiate_observer(observer) #:nodoc:
# Notify observers when the observed class is subclassed.
def inherited(subclass)
super
+ subclasses << subclass
notify_observers :observed_class_inherited, subclass
end
end
@@ -118,5 +118,44 @@ def assert_observer_not_notified(model_class, observer_class)
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

0 comments on commit ad62f19

Please sign in to comment.