RFC/1630 Signals

[ahopkins]

Resolves #1630

Adds the following API:

• Sanic.signal
• Sanic.event
• Sanic.dispatch
• Blueprint.signal
• Blueprint.event
• Blueprint.dispatch
• Blueprint.app (I have been wanting this one for a long time. After we register, it should be available)

See: https://community.sanicframework.org/t/a-fast-new-router/649/41?u=ahopkins and #1630 (comment)

[ahopkins]

Replaces #1902
Alternative to #2041

[codecov]

Codecov Report

Merging #2042 (5c1e146) into master (1165663) will increase coverage by 0.162%.
The diff coverage is 94.872%.

@@              Coverage Diff              @@
##            master     #2042       +/-   ##
=============================================
+ Coverage   92.007%   92.169%   +0.162%     
=============================================
  Files           36        38        +2     
  Lines         3303      3448      +145     
  Branches       564       580       +16     
=============================================
+ Hits          3039      3178      +139     
- Misses         179       184        +5     
- Partials        85        86        +1     

Impacted Files	Coverage Δ
sanic/asgi.py	94.231% <0.000%> (-1.848%)	⬇️
sanic/config.py	100.000% <ø> (ø)
sanic/mixins/listeners.py	100.000% <ø> (ø)
sanic/mixins/middleware.py	100.000% <ø> (ø)
sanic/signals.py	92.424% <92.424%> (ø)
sanic/mixins/signals.py	95.238% <95.238%> (ø)
sanic/app.py	92.229% <100.000%> (+0.704%)	⬆️
sanic/base.py	100.000% <100.000%> (ø)
sanic/blueprints.py	100.000% <100.000%> (ø)
sanic/exceptions.py	100.000% <100.000%> (ø)
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1165663...5c1e146. Read the comment docs.

[ahopkins]

A current limitation is having a signal on both an app and a blueprint

@app.signal("foo.bar.baz")
def app_signal():
    ...

@bp.signal("foo.bar.baz")
def bp_signal():
    ...

This is related to this issue. Since signals is being released as a BETA API, I am not too concerned about this limitation for 21.3. I will try and work on a solution, but even if there is not one, I think we can still include this. This is an absolute must for 21.6.

@sanic-org/sanic-core-devs thoughts?

[ashleysommer]

What actually happens in the blueprint case you described above?

When triggering the signal "foo.bar.baz", does it trigger the bp_signal() handler, but not the app_signal() handler, because the bp_signal() has additional requirements, or something different? What is the expected outcome?

[ahopkins]

I think the desired outcome should be that both are executed. Instead it comes up with a NotFound, and neither are executed. I have a patch I will push over the weekend. But, not sure off the top of my head what sort of work would be involved to get both the execute.

[ahopkins]

The solution was simple. Will push this soon.

@pytest.mark.asyncio
async def test_dispatch_signal_triggers_on_bp(app):
    bp = Blueprint("bp")

    app_counter = 0
    bp_counter = 0

    @app.signal("foo.bar.baz")
    def app_signal():
        nonlocal app_counter
        app_counter += 1

    @bp.signal("foo.bar.baz")
    def bp_signal():
        nonlocal bp_counter
        bp_counter += 1

    app.blueprint(bp)
    app.signal_router.finalize()

    await app.dispatch("foo.bar.baz")
    assert app_counter == 1
    assert bp_counter == 1

    await bp.dispatch("foo.bar.baz")
    assert app_counter == 1
    assert bp_counter == 2

[ashleysommer]

I'm not so sure about including Blueprint.app feature in this PR.
Is it required for signals to work?

The issue being that a blueprint by its nature is one step removed from the app. It should be possible (and is possible) to register the blueprint on many apps, and multiple apps at the same time. The blueprint by concept shouldn't have any way of knowing what app it is registered on. Having functionality to ask the blueprint instance "what app are you registered on?" doesn't make sense given the design pattern that Blueprints implement.

The best we can offer is perhaps answering the question "what was the most recent app this blueprint was registered on" where the blueprint instance can keep a reference to the last app that called register() on it.

[ahopkins]

You do raise a good point. The necessity for this stemmed from needing access to the signal_router in the event method. From the instance (app or bp) we should be able to wait on an event.

The problem with allowing a single BP on multiple apps is how should it know what signal to emit? Or, for that matter, to which app should it bind its signals? If more than one, should it be all of them?

[ashleysommer]

Yep, easy solution that I think still fits with the design pattern, is for the blueprint instance to maintain a List of apps, when the blueprint is registered it puts the app in the list. When it emits a signal, it emits it to all the apps in the List.

[ahopkins]

It is not quite so simple.

await bp.event("foo.bar.baz")

What is this waiting for? If we wait for all of the apps, it will block. I suppose we could wait for any of them and then run them in a loop with a timeout for each.

[ashleysommer]

Ok. I'm not familiar enough with how the signals are implemented to see why that would block if sending the event to all of the apps. I'll look into it.

[harshanarayana]

This looks great. I don't have any more change requests on this. I did a few tests locally using the branch and it works great.

/lgmt.

[ashleysommer]

I'm nearly finished my review. Nothing extra from me at this stage.

[ahopkins]

👍 I will aim to merge it tomorrow. Then finish some more documentation, and we should be on track for 2021-03-21 release.

[ashleysommer]

I'd like to see these names changed too. requirements and where are not intuitive to use.
I do like when and with, it makes more sense in context, but would prefer not to use a keyword.

I think @app.signal() should take something like where or when or matches, and app.dispatch() should have something like arguments or with or provides.

[ashleysommer]

When this BP is registered on multiple apps, does every app need to have a signal in its router? In the current code this will throw an error if any of the apps doesn't have the signal. I think this an editing error, I think the correct logic is:

Suggested change

	events = set()
	for app in self.apps:
	signal = app.signal_router.name_index.get(event)
	if not signal:
	raise NotFound("Could not find signal %s" % event)
	events.add(signal.ctx.event)
	events = set()
	for app in self.apps:
	signal = app.signal_router.name_index.get(event)
	if signal:
	events.add(signal.ctx.event)

[ahopkins]

When you register a BP, all of its signals should be on the app. In the case where there are multiple apps, it should be on all of them. If it is missing from one, well, likely something went wrong somewhere.

I am fine making this change, but it still feels to me like something bad just happened that should be handled here.

[ashleysommer]

Ok, if it was your intention to do it that way, thats fine. It just looked like a copy&paste error when I was looking through the code, like that exception wasn't supposed to be there. (Eg, if the exception is thrown in the for loop, there is no way for the NotFound exception at the end of the function to be hit).

[ahopkins]

I went back and forth on this myself a couple times. I think maybe you are right though that the final exception doesn't quite fit.

That exception would only be hit if there are no registered apps, in which caste the exception NotFound doesn't make sense.

[ahopkins]

And, because we are accessing self.apps as a property, you would get an exception earlier than that before it ever reaches there.

I suppose I was thinking that we needed to catch when events was empty, but I am not seeing how it could be.

Anyone see something else?

[ashleysommer]

One thing thing that confused me when I was doing some testing, I was trying to get this to work:

@app.listener('after_server_start')
async def after_start1(app, loop):
    log.info("waiting on event")
    await app.event("test.me.event")

@app.listener('after_server_start')
async def after_start2(app, loop):
    log.info("dispatching event")
    await app.dispatch("test.me.event")

When you run that, after_start1 blocks, and after_start2 never runs. Even though the server is started and loop.running is True.
It is because listeners are executed like this:

def trigger_events(events, loop):
    if events:
        for event in events:
            result = event(loop)
            if isawaitable(result):
                loop.run_until_complete(result)

So the listeners are run_until_complete individually, one at a time.

I think we can change trigger_events in server.py to:

    if events:
        awaitables = []
        for event in events:
            result = event(loop)
            if isawaitable(result):
                awaitables.append(result)
        if awaitables:
            loop.run_until_complete(asyncio.gather(*awaitables))

I've tested this and it allows the above example to work as expected.

[ashleysommer]

Need to add signal_router finalization to asgi.py startup():

@                self.asgi_app.sanic_app.router.finalize()
+                if app.signal_router.routes:
+                    app.signal_router.finalize()

[ahopkins]

I've tested this and it allows the above example to work as expected.

I think running like this sort of misses the point. I would expect and want that to block. Making your change will make both of them run, but I think that is incorrect. In your use case, you should be using create_task or add_task to put that waiter into its own Task.

Your change would prioritize all sync functions over async for a given listener with no way to control that, and also provide no way to guarantee execution order and/or dependencies.

[ashleysommer]

ok, happy to merge now.

[ashleysommer]

Example of an Event-Triggered-Task helper on App in app.py that I mentioned in #2059. Good idea?

    @staticmethod
    async def _event_task_runner(_event, f, *args, **kwargs):
        await _event
        result = f(*args, **kwargs)
        if isawaitable(result):
            result = await result

    def event_task(self, event: str, f, *args, **kwargs):
        _event = self.event(event)
        self.add_task(self._event_task_runner(_event, f, *args, **kwargs))

EDIT: Also a decorator version:

    @staticmethod
    async def _event_task_runner(_event, f, *args, **kwargs):
        await _event
        result = f(*args, **kwargs)
        if isawaitable(result):
            result = await result

    def event_task(self, event: str, *args, **kwargs):
        """decorator"""
        async def set_up_runner(f):
            nonlocal self, event, args, kwargs
            _event_waiter = self.event(event)
            await self._event_task_runner(_event_waiter, f, *args, **kwargs)

        def decorator(f):
            nonlocal self
            self.add_task(set_up_runner(f))
            return f
        return decorator

[ahopkins]

What would be a use case for this? Why not just create a signal handler? That would run in a task.

Well, I suppose more specifically ALL signal handlers execute in a task.

[ashleysommer]

Regarding my helper suggestion:

First I was thinking of how to make app.event() more useful. In its most basic usage, app.event() will block, and only some future thing on the running event loop (like a scheduled task, or an action on a route handler) can unblock it. So it will be a common design pattern that a user will start a new task and run app.event() in there and have that task blocked, if they don't want to deal with the block in their code.

So if that's going to be a pretty common usage pattern, then a helper to do that would be useful. The helper takes a function, and puts it on the new task, to execute after the event unblocks.

Then I was thinking generally a helper that takes a callback function as an argument is better implemented as a function decorator. Hence my second suggestion.

But I agree that might be going a bit too far, as then its actually just replicating the functionality of the Signal Handlers.

[ahopkins]

Let's maybe keep this idea in our back pocket as something to implement in round 2 for 21.6. Maybe once it is in the wild and we get to play with it we will see if this might be helpful or not.

[ashleysommer]

All good from me