Signals

[alexdutton]

To support altering headers before responses are sent, among other things, there should be a signals framework so people can hook into the response process. This would be extensible for other uses too. This issue follows on from #393.

The proposal is:

Create a new signals module, containing a Signal class. Its constructor takes a single argument, arguments, saying which arguments must be passed when dispatching the signal. This class has a method for checking that potential callbacks have a compatible signature. The module also contains instances of this class for individual types of signal (e.g. response_start = Signal({'request', 'response'}) — I can't yet think of what other signals might be useful).

Application gets a new _signals = defaultdict(list) and a @property that exposes a view of it. It gets a add_signal_callback(signal : Signal, callback : Callable) method, which checks that the callback is compatible (see above) and coerces the calback to a coroutine, before appending the callback to self._signals[signal].

Signal dispatching is done by Appllication.dispatch_signal(signal : Signal, kwargs : dict), which iterates through the registered signals, calling each in turn.

Response.start() is modified to dispatch the response_start signal after this line.

I don't think there's any need to create signals for the request creation phase, as the request can be modified safely with middleware.

The use-case of adding response headers from middleware becomes this slightly contrived example:

import asyncio
import time

from aiohttp.signals import response_start

@asyncio.coroutine
def add_header_middleware(app, handler):
    @asyncio.coroutine
    def middleware(request):
        request.started = time.time()
        return (yield from handler(request))
    return middleware

def add_request_started(*, request, response):
    response.headers['X-Started'] = str(request.started)

app = Application(middlewares=(add_header_middleware,))
app.add_signal_handler(response_start, add_request_started)

[asvetlov]

I support the idea but maybe with slightly different API details.
Let me sleep on it first.

For now Request is read-only object, we need .clone(method=..., path=..., ...) method for replacing realonly properties. This is precondition for signals I believe.
Would you work on Pull Request for it?

[alexdutton]

Yep, happy to work on a pull request, just wanted to make sure I wasn't heading in the wrong direction first :-).

[alexdutton]

Oh, forgot to mention: Wasn't actually suggesting it use function annotations, but thought it was useful syntax for expressing argument types.

[asvetlov]

How do you want to use function annotations in signal context?
Make requirement for signal handler to use annotations for type checking?

[alexdutton]

Oh, I didn't have any particular desire to use them, sorry. Should have said "thought it was useful syntax for expressing argument types in writing up the proposal".

Might be handy for documentation purposes, but that'd be a library-wide style thing.

[asvetlov]

I suggest not use annotations for now. After Python 3.5 release we may return to discussion bearing in mind PEP 484.

I suggest using Request.replace instead of Request.clone. Clone is for something independent and may be used along with original. But for request the clone should be used instead of original in subsequent calls.

My English is so far from be fluent but I feel the difference is important. Maybe you may suggest better word.

[alexdutton]

My current implementation makes Response.start() into a coroutine so that it can yield from app.dispatch_signal(). Does this sound reasonable? (There are a lot of tests that need changing, and it'd be backwards incompatible)

[asvetlov]

Backward incompatibility is a big problem. Better to avoid it.
I don't see any satisfactory solution now.
@fafhrd91 do you have any ideas?

[alexdutton]

It also means that all the tests that use mocks for Application now fail as calling yield from app.dispatch_signal(…) from Response.start() says you can't iterate over a mock. I'll pause here and await guidance 😁.

[asvetlov]

I don't care about tests: we can change those if needed.
But breaking API scares me.

[alexdutton]

Yes. I couldn't see an alternative to making start a coroutine, unless all signals are to be blocking too (which makes them less useful if they've got to do IO).

Current WIP at https://github.com/alexsdutton/aiohttp/tree/signals. I've updated the tests to treat response.start as a coroutine, but haven't yet sorted the mock issue.

[fafhrd91]

We won't change 'start' method.

@asvetlov what is the reason for clone()?

[asvetlov]

@fafhrd91 I dislike mutating request object from arbitrary point. I believe it makes a mess quickly.

[fafhrd91]

-1 for clone, you can not mutate request at arbitrary point in any case. In reality you can mutate it safely only in middleware after that it does not matter

I am in general -1 for anything without confirmed improvements.

[asvetlov]

@fafhrd91 are you -1 for signals in general?

[alexdutton]

So, my actual use-case is implementing RFC4559. This says:

A status code 200 status response can also carry a "WWW-Authenticate"
response header containing the final leg of an authentication.

To handle this case, one needs to be able to set response headers on arbitrary responses. The code I have at the moment is https://github.com/ox-it/apiox-core/blob/master/apiox/core/middleware/negotiate.py#L28-L44.

(Negotiate auth is admittedly Not Done Right, on account of the WWW-Authenticate and Authorization headers breaking the grammar (not key-value params), and it being connection-oriented. But still, it's common within our organisation.)

[fafhrd91]

@asvetlov i'm fine with signals. but we can not change start() method, this will brake all production code.

[fafhrd91]

my proposal is to make signals support coroutine by creating different type of signal instances and fire coroutine signal before handler call. also instead of identifying signals by string, let's make signal instances on Application object. something like:

class Signal:

   def register_handler(...):
       ...

   def unregister_handler(...):
      ...

   def fire(*args, **kwargs):
     ...

class SignalWithCoroutines(Signal):

   @asyncio.coroutine
   def fire(*args, **kwargs):
       ...

class web.Application:

   def __init__(...):
        ...
        self.onBeforeHandler = SignalWithCoroutine()
        self.onResponseStart = Signal()

class Response:

    def start(self, request):
        ...
        request.app.onResponseStart.fire(self, request=request)

[alexdutton]

I'd worry that making them attributes on the app would clutter it. Those that want to define their own signals would either have to attach them to the app, or keep track of them some other way.

If it's desirable to eventually have start be a coroutine, it could be phased with some sort of parameter at app creation time, with a deprecation warning if it's not set. Only if it's set is the start-response signal fired. Messy, perhaps.

[asvetlov]

I'm fine with splitting signal types into regular functions and coroutines.
Sure, we shouldn't change .start() signature. Backward compatibility is very important thing.
Let's not guess about future, just use non-coroutine signal for resp.start().

From my taste app.add_signal_handler(response_start, add_request_started) is uglier than app.on_response_start.register(signal_handler).
Anyhow we need a way to iterate over handlers and drop signal handler in our API.

[alexdutton]

Sorry for the radio silence. I'll have a go at implementing something along those lines soon.

[alexdutton]

So, here's a new branch: https://github.com/alexsdutton/aiohttp/tree/signals-v2

I've created an AsyncSignal, but haven't used it anywhere.

Is this looking more like it?

[asvetlov]

Looking on flask I see two mechanisms for the same thing:

1. Signal-based, like http://flask.pocoo.org/docs/0.10/api/#flask.request_started
2. Callback-based, http://flask.pocoo.org/docs/0.10/api/#flask.Flask.before_request

Flask's signals are asynchronous in term that their execution order is undetermenistic and signal handlers don't return values. Callbacks are opposite, they are ordered and may return response object to prevent callback chain execution.

Citation from flask docs:

Also keep in mind that signals are intended to notify subscribers and should not encourage subscribers to modify data.

Do we need both? How to process, say, default-expect-handler via signals?
@fafhrd91 what do you think about?

[fafhrd91]

Let's just do signals for now

[asvetlov]

@alexsdutton I believe parameters arg for Signal.__init__ should be inspect.Signature object, not a sequence.

[alexdutton]

@asvetlov How do you mean? Having the person defining the signal construct a Signature would be a bit involved, and I can't see a way to check whether one signature is e.g. < another (i.e. any call to the latter is a valid call to the former). The inspect.signature(callback).bind(…) does check whether the callback can be called with the given named parameters.

[asvetlov]

I suggest syntax like FunctionSignal(inspect.signature(lambda request, response, *, flag=False: None)) for registering signal type.

[alexdutton]

Isn't that a bit verbose? You'd only be able to check for signature equality, which could cause trouble down the line if you want to pass extra arguments in a backwards-compatible manner — developers would be able to use **kwargs to catch and discard any arguments they weren't expecting.

[alexdutton]

I've just pushed some commits onto the #439 PR that reflect the start/prepare change that happened in #525. This gets rid of the function signals and sends the on_response_start signal from prepare instead of start.

[alexdutton]

I'm also less convinced about the sensibleness of making the dispatch order arbitrary. In the CORS (#516) scenario, one would want the CORS signal handler to come after most other handlers in case one of those other handlers has set a header that the CORS handler needs to expose.

Do others agree that it would make sense to dispatch in reverse order, i.e.::

app.on_response_start.connect(add_cors_headers)
app.on_response_start.connect(add_server_header)

… would add the Server header first, then the CORS headers? I think the reverse ordering makes sense in the response start context, but might not make sense in other contexts.

Should on_response_start be renamed to on_response_prepare, too?

I'd also suggest changing the app finish stuff to use the signals implementation (with backwards compatibility, of course). Good idea?

[asvetlov]

Yes, signal handlers should be ordered -- as middlewares are.
Maybe we need just derive Signal from bare list with adding only .send method and dropping all signature checks at least for now?

[alexdutton]

Sounds plausible :-)

[asvetlov]

I've tried to implement collections.abc.MutableSequence for signal in signals branch (see https://github.com/KeepSafe/aiohttp/tree/signals) but found the keeping abc for signature check only is annoying.
@alexsdutton do you agree with just deriving from list?

[alexdutton]

I'm happy with that. I've only counted five methods that would need overriding if one does want to later implement signature checking (__setitem__, __iadd__, append, insert, extend).

[alexdutton]

@asvetlov I've just pushed to my signals-v2 branch with a list-based implementation.

[asvetlov]

Fixed by #562

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.