bot: fix race-condition in dispatch

The scenario is:

* dispatch is called for a pretrigger
* it looks into the list of callable by priority
* it triggers one of the first callables, which wants to remove a
  callable of the same priority
* because Python must not allow a dict to be modified while it is
  iterated over, Python raises an exception

If we put a lock here, Python won't raise the exception... but we'll get
a deadlock. To prevent that, instead, we copy the list of callables, and
return the ones that are triggered. If any of them wants to change the
callables of the same priority, they won't raise an error, because we
are not iterating anymore on the dict.

Hoo-ray!

(with applied review suggestions from dgw)

Co-Authored-By: dgw <[email protected]>

sopel/bot.py

@@ -539,10 +539,36 @@ def call(self, func, sopel, trigger):
             if not trigger.is_privmsg:
                 self._times[trigger.sender][func] = current_time
 
-    def dispatch(self, pretrigger):
-        """Dispatch a parsed message to any registered callables.
+    def get_triggered_callables(self, priority, pretrigger, blocked):
+        """Get triggered callables by priority.
+
+        :param str priority: priority to retrieve callables
+        :param pretrigger: Sopel pretrigger object
+        :type pretrigger: :class:`~sopel.trigger.PreTrigger`
+        :param bool blocked: true if nick or channel is blocked from triggering
+                             callables
+        :return: a tuple with the callable, the trigger, and if it's blocked
+        :rtype: :class:`tuple`
+
+        This methods retrieves all triggered callables for this ``priority``
+        level. It yields each function with its
+        :class:`trigger<sopel.trigger.Trigger>` object and a boolean
+        flag to tell if this callable is blocked or not.
+
+        To be triggered, a callable must match the ``pretrigger`` using a regex
+        pattern. Then it must comply with other criteria (if any) such as
+        event, intents, and echo-message filters.
+
+        A triggered callable won't actually be invoked by Sopel if the nickname
+        or hostname is ``blocked``, *unless* the nickname is an admin or
+        the callable is marked as "unblockable".
 
-        :param PreTrigger pretrigger: a parsed message from the server
+        .. seealso::
+
+            Sopel invokes triggered callables in its :meth:`~.dispatch` method.
+            The priority of a callable can be set with the
+            :func:`sopel.module.priority` decorator. Other decorators from
+            :mod:`sopel.module` provide additional criteria and conditions.
         """
         args = pretrigger.args
         text = args[-1] if args else ''
@@ -556,63 +582,100 @@ def dispatch(self, pretrigger):
         user_obj = self.users.get(nick)
         account = user_obj.account if user_obj else None
 
-        if self.config.core.nick_blocks or self.config.core.host_blocks:
+        # get a copy of the list of (regex, function) to prevent race-condition
+        # e.g. when a callable wants to remove callable (other or itself)
+        # from the bot, Python won't (and must not) allow to modify a dict
+        # while it iterates over this very same dict.
+        items = list(self._callables[priority].items())
+
+        for regexp, funcs in items:
+            match = regexp.match(text)
+            if not match:
+                continue
+
+            for func in funcs:
+                trigger = Trigger(
+                    self.settings, pretrigger, match, account)
+
+                # check event
+                if event not in func.event:
+                    continue
+
+                # check intents
+                if hasattr(func, 'intents'):
+                    if not intent:
+                        continue
+
+                    match = any(
+                        func_intent.match(intent)
+                        for func_intent in func.intents
+                    )
+                    if not match:
+                        continue
+
+                # check echo-message feature
+                if is_echo_message and not func.echo:
+                    continue
+
+                is_unblockable = func.unblockable or trigger.admin
+                is_blocked = blocked and not is_unblockable
+                yield (func, trigger, is_blocked)
+
+    def _is_pretrigger_blocked(self, pretrigger):
+        if self.settings.core.nick_blocks or self.settings.core.host_blocks:
             nick_blocked = self._nick_blocked(pretrigger.nick)
             host_blocked = self._host_blocked(pretrigger.host)
         else:
             nick_blocked = host_blocked = None
-        blocked = bool(nick_blocked or host_blocked)
 
-        list_of_blocked_functions = []
-        for priority in ('high', 'medium', 'low'):
-            for regexp, funcs in self._callables[priority].items():
-                match = regexp.match(text)
-                if not match:
-                    continue
+        return (nick_blocked, host_blocked)
 
-                for func in funcs:
-                    trigger = Trigger(self.config, pretrigger, match, account)
+    def dispatch(self, pretrigger):
+        """Dispatch a parsed message to any registered callables.
 
-                    # check event
-                    if event not in func.event:
-                        continue
+        :param PreTrigger pretrigger: a parsed message from the server
 
-                    # check intents
-                    if hasattr(func, 'intents'):
-                        if not intent:
-                            continue
+        The ``pretrigger`` (a parsed message) is used to find matching callables:
+        it will retrieve them by order of priority, and run them. It runs
+        triggered callables in separate threads, unless they are marked
+        otherwise with the :func:`sopel.module.thread` decorator.
 
-                        match = any(
-                            func_intent.match(intent)
-                            for func_intent in func.intents
-                        )
-                        if not match:
-                            continue
+        However, it won't run triggered callables at all when they can't be run
+        for blocked nickname or hostname (unless marked "unblockable" with
+        the :func:`sopel.module.unblockable` decorator).
 
-                    # check echo-message feature
-                    if is_echo_message and not func.echo:
-                        continue
+        .. seealso::
 
-                    # check blocked nick/host
-                    # done after we know the trigger would have matched so we
-                    # don't spam logs with "prevented from using" entries about
-                    # functions that weren't going to run anyway
-                    if blocked and not func.unblockable and not trigger.admin:
-                        function_name = "%s.%s" % (
-                            func.__module__, func.__name__
-                        )
-                        list_of_blocked_functions.append(function_name)
-                        continue
+            To get a list of triggered callables by priority, it uses
+            :meth:`~get_triggered_callables`. This method is also responsible
+            for telling ``dispatch`` if the function is blocked or not.
+        """
+        # nickname/hostname blocking
+        nick_blocked = host_blocked = self._is_pretrigger_blocked(pretrigger)
+        blocked = bool(nick_blocked or host_blocked)
+        list_of_blocked_functions = []
 
-                    # call triggered function
-                    wrapper = SopelWrapper(
-                        self, trigger, output_prefix=func.output_prefix)
-                    if func.thread:
-                        targs = (func, wrapper, trigger)
-                        t = threading.Thread(target=self.call, args=targs)
-                        t.start()
-                    else:
-                        self.call(func, wrapper, trigger)
+        # trigger by priority
+        for priority in ('high', 'medium', 'low'):
+            items = self.get_triggered_callables(priority, pretrigger, blocked)
+            for func, trigger, is_blocked in items:
+                # skip running blocked functions, but track them for logging
+                if is_blocked:
+                    function_name = "%s.%s" % (
+                        func.__module__, func.__name__
+                    )
+                    list_of_blocked_functions.append(function_name)
+                    continue
+
+                # call triggered function
+                wrapper = SopelWrapper(
+                    self, trigger, output_prefix=func.output_prefix)
+                if func.thread:
+                    targs = (func, wrapper, trigger)
+                    t = threading.Thread(target=self.call, args=targs)
+                    t.start()
+                else:
+                    self.call(func, wrapper, trigger)
 
         if list_of_blocked_functions:
             if nick_blocked and host_blocked:
@@ -624,7 +687,7 @@ def dispatch(self, pretrigger):
             LOGGER.info(
                 "[%s]%s prevented from using %s.",
                 block_type,
-                nick,
+                pretrigger.nick,
                 ', '.join(list_of_blocked_functions)
             )
 

sopel/module.py

@@ -81,9 +81,24 @@
 
 
 def unblockable(function):
-    """Decorator which exempts the function from nickname and hostname blocking.
+    """Decorator to exempt ``function`` from nickname and hostname blocking.
+
+    This can be used to ensure events such as ``JOIN`` are always recorded::
+
+        from sopel import module
+
+        @module.event('JOIN')
+        @module.unblockable
+        def on_join_callable(bot, trigger):
+            # do something when a user JOIN a channel
+            # a blocked nickname or hostname *will* trigger this
+            pass
+
+    .. seealso::
+
+        Sopel's :meth:`~sopel.bot.Sopel.dispatch` and
+        :meth:`~sopel.bot.Sopel.get_triggered_callables` methods.
 
-    This can be used to ensure events such as JOIN are always recorded.
     """
     function.unblockable = True
     return function