Merge pull request #32861 from zvkemp/asn-unsubscribe-proxy

use ProxyPattern to match for ActiveSupport::Notifications fanout/unsubscribe

activesupport/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Revise `ActiveSupport::Notifications.unsubscribe` to correctly handle Regex or other multiple-pattern subscribers.
+
+    *Zach Kemp*
+
 *   Add `before_reset` callback to `CurrentAttributes` and define `after_reset` as an alias of `resets` for symmetry.
 
     *Rosa Gutierrez*

activesupport/lib/active_support/notifications.rb

@@ -153,6 +153,15 @@ module ActiveSupport
   #
   #   ActiveSupport::Notifications.unsubscribe("render")
   #
+  # Subscribers using a regexp or other pattern-matching object will remain subscribed
+  # to all events that match their original pattern, unless those events match a string
+  # passed to `unsubscribe`:
+  #
+  #   subscriber = ActiveSupport::Notifications.subscribe(/render/) { }
+  #   ActiveSupport::Notifications.unsubscribe('render_template.action_view')
+  #   subscriber.matches?('render_template.action_view') # => false
+  #   subscriber.matches?('render_partial.action_view') # => true
+  #
   # == Default Queue
   #
   # Notifications ships with a queue implementation that consumes and publishes events

activesupport/lib/active_support/notifications/fanout.rb

@@ -2,6 +2,7 @@
 
 require "mutex_m"
 require "concurrent/map"
+require "set"
 
 module ActiveSupport
   module Notifications
@@ -39,6 +40,7 @@ module ActiveSupport
           when String
             @string_subscribers[subscriber_or_name].clear
             @listeners_for.delete(subscriber_or_name)
+            @other_subscribers.each { |sub| sub.unsubscribe!(subscriber_or_name) }
           else
             pattern = subscriber_or_name.try(:pattern)
             if String === pattern
@@ -113,11 +115,33 @@ module ActiveSupport
           end
         end
 
+        class Matcher #:nodoc:
+          attr_reader :pattern, :exclusions
+
+          def self.wrap(pattern)
+            return pattern if String === pattern
+            new(pattern)
+          end
+
+          def initialize(pattern)
+            @pattern = pattern
+            @exclusions = Set.new
+          end
+
+          def unsubscribe!(name)
+            exclusions << -name if pattern === name
+          end
+
+          def ===(name)
+            pattern === name && !exclusions.include?(name)
+          end
+        end
+
         class Evented #:nodoc:
           attr_reader :pattern
 
           def initialize(pattern, delegate)
-            @pattern = pattern
+            @pattern = Matcher.wrap(pattern)
             @delegate = delegate
             @can_publish = delegate.respond_to?(:publish)
           end
@@ -137,11 +161,15 @@ module ActiveSupport
           end
 
           def subscribed_to?(name)
-            @pattern === name
+            pattern === name
           end
 
           def matches?(name)
-            @pattern && @pattern === name
+            pattern && pattern === name
+          end
+
+          def unsubscribe!(name)
+            pattern.unsubscribe!(name)
           end
         end
 
@@ -204,6 +232,10 @@ module ActiveSupport
             true
           end
 
+          def unsubscribe!(*)
+            false
+          end
+
           alias :matches? :===
         end
       end

activesupport/test/notifications/evented_notification_test.rb

@@ -84,6 +84,39 @@ module ActiveSupport
           [:finish, "hi", 1, {}]
         ], listener.events
       end
+
+      def test_listen_to_regexp
+        notifier = Fanout.new
+        listener = Listener.new
+        notifier.subscribe(/[a-z]*.world/, listener)
+        notifier.start("hi.world", 1, {})
+        notifier.finish("hi.world", 2, {})
+        notifier.start("hello.world", 1, {})
+        notifier.finish("hello.world", 2, {})
+
+        assert_equal [
+          [:start, "hi.world", 1, {}],
+          [:finish, "hi.world", 2, {}],
+          [:start, "hello.world", 1, {}],
+          [:finish, "hello.world", 2, {}]
+        ], listener.events
+      end
+
+      def test_listen_to_regexp_with_exclusions
+        notifier = Fanout.new
+        listener = Listener.new
+        notifier.subscribe(/[a-z]*.world/, listener)
+        notifier.unsubscribe("hi.world")
+        notifier.start("hi.world", 1, {})
+        notifier.finish("hi.world", 2, {})
+        notifier.start("hello.world", 1, {})
+        notifier.finish("hello.world", 2, {})
+
+        assert_equal [
+          [:start, "hello.world", 1, {}],
+          [:finish, "hello.world", 2, {}]
+        ], listener.events
+      end
     end
   end
 end

activesupport/test/notifications_test.rb

@@ -128,6 +128,25 @@ module Notifications
       assert_equal [["named.subscription", :foo], ["named.subscription", :foo]], @events
     end
 
+    def test_unsubscribing_by_name_leaves_regexp_matched_subscriptions
+      @matched_events = []
+      @notifier.subscribe(/subscription/) { |*args| @matched_events << event(*args) }
+      @notifier.publish("named.subscription", :before)
+      @notifier.wait
+      [@events, @named_events, @matched_events].each do |collector|
+        assert_includes(collector, ["named.subscription", :before])
+      end
+      @notifier.unsubscribe("named.subscription")
+      @notifier.publish("named.subscription", :after)
+      @notifier.publish("other.subscription", :after)
+      @notifier.wait
+      assert_includes(@events, ["named.subscription", :after])
+      assert_includes(@events, ["other.subscription", :after])
+      assert_includes(@matched_events, ["other.subscription", :after])
+      assert_not_includes(@matched_events, ["named.subscription", :after])
+      assert_not_includes(@named_events, ["named.subscription", :after])
+    end
+
   private
     def event(*args)
       args