Fix re-adding autodiscoveries on subsequent calls

The state instance is shared between all subscriber instances. When invoking `#subscribe_to` on it, we changed its own state (the same bucket where handlers defined at the class level are collected). A second invocation was reusing previous discoveries plus re-discovering the same again.
nebulab · Apr 19, 2022 · 43df11d · 43df11d
1 parent 91ee210
commit 43df11d
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 10 deletions.
diff --git a/lib/omnes/subscriber/state.rb b/lib/omnes/subscriber/state.rb
@@ -24,9 +24,9 @@ def initialize(autodiscover_strategy:, subscription_definitions: [], calling_cac
       def call(bus, instance)
         raise MultipleSubscriberSubscriptionAttemptError if already_called?(bus, instance)
 
-        autodiscover_subscription_definitions(bus, instance) unless autodiscover_strategy.nil?
+        all_subscription_definitions = subscription_definitions + autodiscovered_subscription_definitions(bus, instance)
 
-        definitions = subscription_definitions.map { |defn| defn.(bus, instance) }
+        definitions = all_subscription_definitions.map { |defn| defn.(bus, instance) }
 
         subscribe_definitions(definitions, bus, instance).tap do
           mark_as_called(bus, instance)
@@ -47,21 +47,32 @@ def mark_as_called(bus, instance)
         @calling_cache << [bus, instance]
       end
 
-      def autodiscover_subscription_definitions(bus, instance)
-        bus.registry.event_names.each do |event_name|
-          method_name = autodiscover_strategy.(event_name)
-          next unless instance.respond_to?(method_name, true)
+      def autodiscovered_subscription_definitions(bus, instance)
+        return [] unless autodiscover_strategy
 
-          add_subscription_definition do |_bus|
+        bus.registry.event_names.reduce([]) do |defs, event_name|
+          method_name = autodiscover_strategy.(event_name)
+          if instance.respond_to?(method_name, true)
             [
-              Subscription::SINGLE_EVENT_MATCHER.curry[event_name],
-              Adapter.Type(Adapter::Method.new(method_name)),
-              Subscription.random_id
+              *defs,
+              autodiscovered_subscription_definition(event_name, method_name)
             ]
+          else
+            defs
           end
         end
       end
 
+      def autodiscovered_subscription_definition(event_name, method_name)
+        lambda do |_bus, _instance|
+          [
+            Subscription::SINGLE_EVENT_MATCHER.curry[event_name],
+            Adapter.Type(Adapter::Method.new(method_name)),
+            Subscription.random_id
+          ]
+        end
+      end
+
       def subscribe_definitions(definitions, bus, instance)
         matcher_with_callbacks = definitions.map do |(matcher, adapter, id)|
           [matcher, adapter.curry[instance], id]

diff --git a/spec/unit/omnes/subscriber_spec.rb b/spec/unit/omnes/subscriber_spec.rb
@@ -499,6 +499,22 @@ def on_foo(_event)
       expect(subscriber2.called).to be(true)
     end
 
+    it "doesn't readd previous autodiscoveries when subscribing a second time" do
+      bus.register(:foo)
+      subscriber_class = Class.new do
+        include Omnes::Subscriber[autodiscover: true]
+
+        def on_foo(_event); end
+      end
+      subscriber1 = subscriber_class.new
+      subscriber2 = subscriber_class.new
+
+      subscriber1.subscribe_to(bus)
+      subscriber2.subscribe_to(bus)
+
+      expect(bus.subscriptions.count).to be(2)
+    end
+
     it "can subscribe the same instance to different buses" do
       bus_one = Omnes::Bus.new
       bus_two = Omnes::Bus.new