From db6e0c631dec34b0d422b3dc7a1ce90aae6dc037 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Wed, 8 Nov 2017 11:04:37 +0200 Subject: [PATCH 1/4] Fix tests --- aiohttp/signals.py | 89 +++++------------------------------ aiohttp/test_utils.py | 1 + aiohttp/web.py | 15 +----- aiohttp/worker.py | 1 + tests/test_signals.py | 32 +++---------- tests/test_web_application.py | 18 +++---- tests/test_web_exceptions.py | 1 + tests/test_web_functional.py | 2 +- tests/test_web_response.py | 15 ++++-- tests/test_web_websocket.py | 1 + 10 files changed, 44 insertions(+), 131 deletions(-) diff --git a/aiohttp/signals.py b/aiohttp/signals.py index 77a3df2419f..725912af9af 100644 --- a/aiohttp/signals.py +++ b/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 ''.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) diff --git a/aiohttp/test_utils.py b/aiohttp/test_utils.py index 1daa3f55146..47bf226f8c2 100644 --- a/aiohttp/test_utils.py +++ b/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 diff --git a/aiohttp/web.py b/aiohttp/web.py index 8e63590b1b8..8d31568cfef 100644 --- a/aiohttp/web.py +++ b/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: diff --git a/aiohttp/worker.py b/aiohttp/worker.py index c416d089203..11db69f1d3a 100644 --- a/aiohttp/worker.py +++ b/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) diff --git a/tests/test_signals.py b/tests/test_signals.py index 2e1e2a508d5..67e2d141b29 100644 --- a/tests/test_signals.py +++ b/tests/test_signals.py @@ -13,11 +13,6 @@ 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) @@ -25,6 +20,7 @@ def make_request(app, method, path, headers=CIMultiDict()): 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 +35,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 +52,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 +65,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 +81,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): diff --git a/tests/test_web_application.py b/tests/test_web_application.py index d50816403e2..35e69847d9e 100644 --- a/tests/test_web_application.py +++ b/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 diff --git a/tests/test_web_exceptions.py b/tests/test_web_exceptions.py index b35988e9003..0c280bd5eef 100644 --- a/tests/test_web_exceptions.py +++ b/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 diff --git a/tests/test_web_functional.py b/tests/test_web_functional.py index bfedc38e7e9..dffae48f509 100644 --- a/tests/test_web_functional.py +++ b/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() diff --git a/tests/test_web_response.py b/tests/test_web_response.py index c353bc09e64..0db4ea96c1a 100644 --- a/tests/test_web_response.py +++ b/tests/test_web_response.py @@ -13,10 +13,13 @@ def make_request(method, path, headers=CIMultiDict(), - version=HttpVersion11, **kwargs): + version=HttpVersion11, on_response_prepare=None, **kwargs): app = kwargs.pop('app', None) or mock.Mock() app._debug = False - app.on_response_prepare = signals.Signal(app) + if on_response_prepare is None: + on_response_prepare = signals.Signal(app) + app.on_response_prepare = on_response_prepare + app.on_response_prepare.freeze() protocol = kwargs.pop('protocol', None) or mock.Mock() return make_mocked_request(method, path, headers, version=version, protocol=protocol, @@ -744,11 +747,13 @@ async def test_prepare_twice(): async def test_prepare_calls_signal(): app = mock.Mock() - req = make_request('GET', '/', app=app) + sig = make_mocked_coro() + on_response_prepare = signals.Signal(app) + on_response_prepare.append(sig) + req = make_request('GET', '/', app=app, + on_response_prepare=on_response_prepare) resp = StreamResponse() - sig = mock.Mock() - app.on_response_prepare.append(sig) await resp.prepare(req) sig.assert_called_with(req, resp) diff --git a/tests/test_web_websocket.py b/tests/test_web_websocket.py index 2875102ec3b..4d66798da8e 100644 --- a/tests/test_web_websocket.py +++ b/tests/test_web_websocket.py @@ -17,6 +17,7 @@ def app(loop): ret.loop = loop ret._debug = False ret.on_response_prepare = signals.Signal(ret) + ret.on_response_prepare.freeze() return ret From 2768d72fa345e4e582dfe1f4579322448493ef86 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Wed, 8 Nov 2017 11:27:58 +0200 Subject: [PATCH 2/4] Improve coverage --- tests/test_signals.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/test_signals.py b/tests/test_signals.py index 67e2d141b29..12478a87238 100644 --- a/tests/test_signals.py +++ b/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 @@ -139,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", frozen=False, " + r"\[\]>", + repr(signal)) From 44fbe7e90c8f209a692a793d4790129fb0570607 Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Wed, 8 Nov 2017 11:34:17 +0200 Subject: [PATCH 3/4] Add changenotes --- CHANGES/2480.feature | 1 + CHANGES/2480.removal | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 CHANGES/2480.feature create mode 100644 CHANGES/2480.removal diff --git a/CHANGES/2480.feature b/CHANGES/2480.feature new file mode 100644 index 00000000000..97482cc1d5f --- /dev/null +++ b/CHANGES/2480.feature @@ -0,0 +1 @@ +Signal handlers (registered callbacks) should be coroutines. diff --git a/CHANGES/2480.removal b/CHANGES/2480.removal new file mode 100644 index 00000000000..3def1f82f51 --- /dev/null +++ b/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. From a9b36944bde4652684c65f55b11ea32fe35a0d3a Mon Sep 17 00:00:00 2001 From: Andrew Svetlov Date: Wed, 8 Nov 2017 11:42:13 +0200 Subject: [PATCH 4/4] Update doc --- docs/api.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/api.rst b/docs/api.rst index 3168631fbbc..59e8cc201a3 100644 --- a/docs/api.rst +++ b/docs/api.rst @@ -192,6 +192,13 @@ callbacks by ``await sig.send(data)``. For concrete usage examples see :ref:`signals in aiohttp.web ` 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