Navigation Menu

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

Change signal routing for increased consistency #2277

Merged
merged 7 commits into from Dec 23, 2021
Merged

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Oct 17, 2021

It was recently brought to my attention that signal routing between blueprints and apps could be confusing depending upon where signals were registered.

For example, in the following case, dispatching my.signal.two from the app only works if it is also defined in the app. While this is not exactly a bug, and how it was intended to perform, it is a little confusing and the pattern does not really make sense to have to couple your definitions.

@app.signal('my.signal.one')
def app_signal_one():
    print('App Signal One')

@bp.signal('my.signal.one')
def bp_signal_one():
    print('BP Signal One')

@bp.signal('my.signal.two')
def bp_signal_two():
    print('BP Signal Two')

Similarly, if you have a catch all my.signal.<foo>, it is difficult to knowingly predict if it will be executed if you also have a finite signal defined matching the namespace/context pair.

Therefore, this PR does two things:

  1. Removes the injection of extra, which is not needed since the actual matching happens outside of the router to allow for multiple handlers to be executed. Previously, this was used to force the routing into a dynamic state.
  2. We can achieve this dynamic state by forcefully coercing all final parts of the signal trigger to be dynamic, and also matching on this outside of the router. This further has the benefit of allowing all actual dynamic routing to be executed regardless of the match-ability of the other defined signals.

One thing that needs to be thought through on this still is the breaking change this imposes.

The methog app.signal_router.get returns a tuple of (group, handlers, params). The list of handlers was what would be dispatched.

With this change, handlers in the router get is not determinative. You still need to run through the dispatch since the change is to loop over signals in dispatch and not the getter. I left the signature as is, but it no longer will retrieve the proper handlers.

An alternative is to set more context on the handler function (like handler.__requirements__). I would like to avoid this if we can, but it might be a cleaner solution since it would allow for the get() method to continue without a breaking change.


Still todo:

  • Tests
  • Make sure that the events being set in the _dispatch method are done properly according to the new matching requirements

@Soltares
Copy link

To provide a usage example of the original behavior to help with any decision-making -- I am using a websocket route from within a blueprint to dispatch signals when a websocket message is received.

@bp.websocket("/feed")
async def feed(request, client):
    while True:
        message = json.loads(await client.recv())
        await request.app.dispatch(f'ws.client.{message["event"]}')

I am then attempting to receive the signal from a different blueprint designed to handle a specific event, but Sanic is unable to find the handler for the signal since it is not defined in the app level.

@bp2.signal("ws.client.chat")
async def chat(data):
    print("Chat event")

However it does call the blueprint event if an identically named event is also present in the app-level.

@app.signal("ws.client.chat")
async def chat(data):
    pass

A workaround is to change the websocket route to dispatch to it's own blueprint and reference the websocket blueprint when creating handlers for the signals.

# blueprints/ws.py
@bp.websocket("/feed")
async def feed(request, client):
    while True:
        message = json.loads(await client.recv())
        await bp.dispatch(f'ws.client.{message["event"]}')
# blueprints/chat.py
import blueprints.ws as ws

@ws.bp.signal("ws.client.chat")
async def chat(data):
    print("Chat event")

@ahopkins ahopkins marked this pull request as ready for review December 13, 2021 08:41
@ahopkins ahopkins requested a review from a team as a code owner December 13, 2021 08:41
@ahopkins ahopkins added the needs review the PR appears ready but requires a review label Dec 13, 2021
@ahopkins ahopkins merged commit b91ffed into main Dec 23, 2021
@ahopkins ahopkins deleted the signal-routing branch December 23, 2021 23:27
ChihweiLHBird pushed a commit to ChihweiLHBird/sanic that referenced this pull request Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking needs review the PR appears ready but requires a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants