Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signals refacoting #2480

Merged
merged 5 commits into from
Nov 9, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 13 additions & 76 deletions aiohttp/signals.py
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about to add more friendly exception if plain func is going to be registered as handler? I guess now such code will fail with cryptic TypeError: object NoneType can't be used in 'await' expression or similar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest catching TypeError and raise a new TypeError with more informative exception message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an option we could check ll callbacks on freezing stage.
It prevents registering a regular function which returns a future but it's fine I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes for TypeError and yes, better check this early. On append call if possible. The stacktrace will help to find that bad signal usage with easy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FrozenList can be modified by too many ways, .append is not the only one.
I think .freeze() is a good compromise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a pity. Ok then if error message would contains function name which is not async.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, but I see you point.
Maybe we need specialized CheckedFrozenList class with registered callback? It could improve UX slightly.
Please let me merge the PR as is and make a new issue.
I don't want to block @pfreixes with his client tracing PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Let's do that.

1 change: 1 addition & 0 deletions aiohttp/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 2 additions & 13 deletions aiohttp/web.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions aiohttp/worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
32 changes: 7 additions & 25 deletions tests/test_signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,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)

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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'')
Expand All @@ -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):
Expand Down
18 changes: 7 additions & 11 deletions tests/test_web_application.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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)
Expand All @@ -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()
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions tests/test_web_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion tests/test_web_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
15 changes: 10 additions & 5 deletions tests/test_web_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions tests/test_web_websocket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down