Signals refacoting (#2480)

* Fix tests

* Improve coverage

* Add changenotes

* Update doc

CHANGES/2480.feature

@@ -0,0 +1 @@
+Signal handlers (registered callbacks) should be coroutines.

CHANGES/2480.removal

@@ -0,0 +1,3 @@
+Drop undocumented `app.on_pre_signal` and `app.on_post_signal`. Signal
+handlers should be coroutines, support for regular functions is
+dropped.

aiohttp/signals.py

@@ -1,95 +1,32 @@
-import asyncio
-from itertools import count
-
 from aiohttp.frozenlist import FrozenList
 
 
-class BaseSignal(FrozenList):
-
-    __slots__ = ()
-
-    async def _send(self, *args, **kwargs):
-        for receiver in self:
-            res = receiver(*args, **kwargs)
-            if asyncio.iscoroutine(res) or asyncio.isfuture(res):
-                await res
-
-
-class Signal(BaseSignal):
+class Signal(FrozenList):
     """Coroutine-based signal implementation.
 
     To connect a callback to a signal, use any list method.
 
-    Signals are fired using the :meth:`send` coroutine, which takes named
+    Signals are fired using the send() coroutine, which takes named
     arguments.
     """
 
-    __slots__ = ('_app', '_name', '_pre', '_post')
+    __slots__ = ('_owner',)
 
-    def __init__(self, app):
+    def __init__(self, owner):
         super().__init__()
-        self._app = app
-        klass = self.__class__
-        self._name = klass.__module__ + ':' + klass.__qualname__
-        self._pre = app.on_pre_signal
-        self._post = app.on_post_signal
+        self._owner = owner
+
+    def __repr__(self):
+        return '<Signal owner={}, frozen={}, {!r}>'.format(self._owner,
+                                                           self.frozen,
+                                                           list(self))
 
     async def send(self, *args, **kwargs):
         """
         Sends data to all registered receivers.
         """
-        if self:
-            ordinal = None
-            debug = self._app._debug
-            if debug:
-                ordinal = self._pre.ordinal()
-                await self._pre.send(
-                    ordinal, self._name, *args, **kwargs)
-            await self._send(*args, **kwargs)
-            if debug:
-                await self._post.send(
-                    ordinal, self._name, *args, **kwargs)
-
+        if not self.frozen:
+            raise RuntimeError("Cannot send non-frozen signal.")
 
-class FuncSignal(BaseSignal):
-    """Callback-based signal implementation.
-
-    To connect a callback to a signal, use any list method.
-
-    Signals are fired using the :meth:`send` method, which takes named
-    arguments.
-    """
-
-    __slots__ = ()
-
-    def send(self, *args, **kwargs):
-        """
-        Sends data to all registered receivers.
-        """
         for receiver in self:
-            receiver(*args, **kwargs)
-
-
-class DebugSignal(BaseSignal):
-
-    __slots__ = ()
-
-    async def send(self, ordinal, name, *args, **kwargs):
-        await self._send(ordinal, name, *args, **kwargs)
-
-
-class PreSignal(DebugSignal):
-
-    __slots__ = ('_counter',)
-
-    def __init__(self):
-        super().__init__()
-        self._counter = count(1)
-
-    def ordinal(self):
-        return next(self._counter)
-
-
-class PostSignal(DebugSignal):
-
-    __slots__ = ()
+            await receiver(*args, **kwargs)

aiohttp/test_utils.py

@@ -141,6 +141,7 @@ def __init__(self, app, *,
 
     async def _make_factory(self, **kwargs):
         self.app._set_loop(self._loop)
+        self.app.freeze()
         await self.app.startup()
         self.handler = self.app.make_handler(loop=self._loop, **kwargs)
         return self.handler

aiohttp/web.py

@@ -20,7 +20,7 @@
 from .helpers import AccessLogger
 from .http import HttpVersion  # noqa
 from .log import access_logger, web_logger
-from .signals import PostSignal, PreSignal, Signal
+from .signals import Signal
 from .web_exceptions import *  # noqa
 from .web_fileresponse import *  # noqa
 from .web_middlewares import *  # noqa
@@ -72,8 +72,6 @@ def __init__(self, *,
         self._frozen = False
         self._subapps = []
 
-        self._on_pre_signal = PreSignal()
-        self._on_post_signal = PostSignal()
         self._on_response_prepare = Signal(self)
         self._on_startup = Signal(self)
         self._on_shutdown = Signal(self)
@@ -142,8 +140,6 @@ def freeze(self):
         self._frozen = True
         self._middlewares = tuple(self._prepare_middleware())
         self._router.freeze()
-        self._on_pre_signal.freeze()
-        self._on_post_signal.freeze()
         self._on_response_prepare.freeze()
         self._on_startup.freeze()
         self._on_shutdown.freeze()
@@ -193,14 +189,6 @@ def add_subapp(self, prefix, subapp):
     def on_response_prepare(self):
         return self._on_response_prepare
 
-    @property
-    def on_pre_signal(self):
-        return self._on_pre_signal
-
-    @property
-    def on_post_signal(self):
-        return self._on_post_signal
-
     @property
     def on_startup(self):
         return self._on_startup
@@ -422,6 +410,7 @@ def run_app(app, *, host=None, port=None, path=None, sock=None,
         loop = asyncio.get_event_loop()
 
     app._set_loop(loop)
+    app.freeze()
     loop.run_until_complete(app.startup())
 
     try:

aiohttp/worker.py

@@ -47,6 +47,7 @@ def init_process(self):
 
     def run(self):
         if hasattr(self.wsgi, 'startup'):
+            self.wsgi.freeze()
             self.loop.run_until_complete(self.wsgi.startup())
         self._runner = asyncio.ensure_future(self._run(), loop=self.loop)
 

docs/api.rst

@@ -192,6 +192,13 @@ callbacks by ``await sig.send(data)``.
 For concrete usage examples see :ref:`signals in aiohttp.web
 <aiohttp-web-signals>` chapter.
 
+.. versionchanged:: 3.0
+
+   ``sig.send()`` call is forbidden for non-frozen signal.
+
+   Support for regular (non-async) callbacks is dropped. All callbacks
+   should be async functions.
+
 
 .. class:: Signal
 

tests/test_signals.py

@@ -1,10 +1,11 @@
+import re
 from unittest import mock
 
 import pytest
 from multidict import CIMultiDict
 
 from aiohttp.signals import Signal
-from aiohttp.test_utils import make_mocked_request
+from aiohttp.test_utils import make_mocked_coro, make_mocked_request
 from aiohttp.web import Application, Response
 
 
@@ -13,18 +14,14 @@ def app():
     return Application()
 
 
-@pytest.fixture
-def debug_app():
-    return Application(debug=True)
-
-
 def make_request(app, method, path, headers=CIMultiDict()):
     return make_mocked_request(method, path, headers, app=app)
 
 
 async def test_add_signal_handler_not_a_callable(app):
     callback = True
     app.on_response_prepare.append(callback)
+    app.on_response_prepare.freeze()
     with pytest.raises(TypeError):
         await app.on_response_prepare(None, None)
 
@@ -39,6 +36,7 @@ async def callback(**kwargs):
         callback_mock(**kwargs)
 
     signal.append(callback)
+    signal.freeze()
 
     await signal.send(**kwargs)
     callback_mock.assert_called_once_with(**kwargs)
@@ -55,6 +53,7 @@ async def callback(*args, **kwargs):
         callback_mock(*args, **kwargs)
 
     signal.append(callback)
+    signal.freeze()
 
     await signal.send(*args, **kwargs)
     callback_mock.assert_called_once_with(*args, **kwargs)
@@ -67,6 +66,7 @@ async def cb(*args, **kwargs):
         callback(*args, **kwargs)
 
     app.on_response_prepare.append(cb)
+    app.on_response_prepare.freeze()
 
     request = make_request(app, 'GET', '/')
     response = Response(body=b'')
@@ -82,27 +82,10 @@ async def test_non_coroutine(app):
     callback = mock.Mock()
 
     signal.append(callback)
+    signal.freeze()
 
-    await signal.send(**kwargs)
-    callback.assert_called_once_with(**kwargs)
-
-
-async def test_debug_signal(debug_app):
-    assert debug_app.debug, "Should be True"
-    signal = Signal(debug_app)
-
-    callback = mock.Mock()
-    pre = mock.Mock()
-    post = mock.Mock()
-
-    signal.append(callback)
-    debug_app.on_pre_signal.append(pre)
-    debug_app.on_post_signal.append(post)
-
-    await signal.send(1, a=2)
-    callback.assert_called_once_with(1, a=2)
-    pre.assert_called_once_with(1, 'aiohttp.signals:Signal', 1, a=2)
-    post.assert_called_once_with(1, 'aiohttp.signals:Signal', 1, a=2)
+    with pytest.raises(TypeError):
+        await signal.send(**kwargs)
 
 
 def test_setitem(app):
@@ -157,3 +140,28 @@ def test_cannot_delitem_in_frozen_signal(app):
         del signal[0]
 
     assert list(signal) == [m1]
+
+
+async def test_cannot_send_non_frozen_signal(app):
+    signal = Signal(app)
+
+    callback = make_mocked_coro()
+
+    signal.append(callback)
+
+    with pytest.raises(RuntimeError):
+        await signal.send()
+
+    assert not callback.called
+
+
+async def test_repr(app):
+    signal = Signal(app)
+
+    callback = make_mocked_coro()
+
+    signal.append(callback)
+
+    assert re.match(r"<Signal owner=<Application .+>, frozen=False, "
+                    r"\[<Mock id='\d+'>\]>",
+                    repr(signal))

tests/test_web_application.py

@@ -5,6 +5,7 @@
 
 from aiohttp import log, web
 from aiohttp.abc import AbstractAccessLogger, AbstractRouter
+from aiohttp.test_utils import make_mocked_coro
 
 
 def test_app_ctor(loop):
@@ -95,10 +96,11 @@ def log(self, request, response, time):
 
 async def test_app_register_on_finish():
     app = web.Application()
-    cb1 = mock.Mock()
-    cb2 = mock.Mock()
+    cb1 = make_mocked_coro(None)
+    cb2 = make_mocked_coro(None)
     app.on_cleanup.append(cb1)
     app.on_cleanup.append(cb2)
+    app.freeze()
     await app.cleanup()
     cb1.assert_called_once_with(app)
     cb2.assert_called_once_with(app)
@@ -113,6 +115,7 @@ async def cb(app):
         fut.set_result(123)
 
     app.on_cleanup.append(cb)
+    app.freeze()
     await app.cleanup()
     assert fut.done()
     assert 123 == fut.result()
@@ -141,7 +144,7 @@ async def on_shutdown(app_param):
         called = True
 
     app.on_shutdown.append(on_shutdown)
-
+    app.freeze()
     await app.shutdown()
     assert called
 
@@ -150,16 +153,10 @@ async def test_on_startup(loop):
     app = web.Application()
     app._set_loop(loop)
 
-    blocking_called = False
     long_running1_called = False
     long_running2_called = False
     all_long_running_called = False
 
-    def on_startup_blocking(app_param):
-        nonlocal blocking_called
-        assert app is app_param
-        blocking_called = True
-
     async def long_running1(app_param):
         nonlocal long_running1_called
         assert app is app_param
@@ -178,11 +175,10 @@ async def on_startup_all_long_running(app_param):
                                     long_running2(app_param),
                                     loop=app_param.loop)
 
-    app.on_startup.append(on_startup_blocking)
     app.on_startup.append(on_startup_all_long_running)
+    app.freeze()
 
     await app.startup()
-    assert blocking_called
     assert long_running1_called
     assert long_running2_called
     assert all_long_running_called

tests/test_web_exceptions.py

@@ -38,6 +38,7 @@ def write_headers(status_line, headers):
     app = mock.Mock()
     app._debug = False
     app.on_response_prepare = signals.Signal(app)
+    app.on_response_prepare.freeze()
     req = make_mocked_request(method, path, app=app, payload_writer=writer)
     return req
 

tests/test_web_functional.py

@@ -1247,7 +1247,7 @@ async def on_signal(app):
 async def test_subapp_on_shutdown(loop, test_server):
     order = []
 
-    def on_signal(app):
+    async def on_signal(app):
         order.append(app)
 
     app = web.Application()