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

RFC : Signals Support for Sanic Proposal #1630

Closed
harshanarayana opened this issue Jul 11, 2019 · 26 comments · Fixed by #2042
Closed

RFC : Signals Support for Sanic Proposal #1630

harshanarayana opened this issue Jul 11, 2019 · 26 comments · Fixed by #2042
Labels
Milestone

Comments

@harshanarayana
Copy link
Contributor

Proposal : Signals Support for Sanic

Introduction

This proposal defines a brief set of guidelines and behaviors with regards to a new set of entities in the sanic modules called signals. These entities will serve as a standalone entities during the lifecycle of a sanic request and will be used as a mechanism to signal a certain kind of even being taken place under the respective core components or any sanic extension that wants to adopt the signaling behavior.

Background

Currently, sanic framework provides you a way to bind into the request and response lifecycle of an incoming API request via the middleware support as @app.middleware("request") or @app.middleware("response"). However, this adds a strict binding between your code and how you want to process these events.

This can be cumbersome when it comes to adding additional sanic extensions that can help in monitoring and metrics tracking purpose. This introduces the following set of drawbacks.

  1. The process required to handle events is coupled with business logic. But the way to handle these events is very generic. So, there is no reason for them to be co-located with the business logic. Not even in the form of a decorator
  2. This adds an additional delay to each request-response cycle and this is not always a preferred behavior. All you wanted to do was to monitor the events being triggered in the lifecycle of an event. But tracing behavior should not introduce additional delay

So this proposal suggests a signal as a way to bypass this constraint. This will let you decouple the way the signals are handled and that process can reside in it's own context without impacting the business logic.

This proposal in it's basic form is a simple pub-sub model implementation where the extensions and the sanic core components will generate certain events and the subscribers to those events can handle them the way they all see fit.

Proposal

A signal in it's most simple form is just an event like in any of your pub-sub models. And the possible behavior is as follow.

  1. sanic core components will be responsible for generating a certain set of events described below.
    1. Server Lifecycle Event
      1. starting_server
      2. server_started
      3. stopping_server
      4. server_stopped
    2. Request Lifecycle Events
      1. request_started
      2. request_finished
      3. response_finished
      4. request_teardown
      5. request_exception
    3. Generic Events
      1. exception_occured
      2. notification

Each of these signal will have a concrete context define which instructs when these events will be triggered and what they represent. Any extension/plugin that wants to make you use these signal to perform a certain operation on their own are free to do so at their own discretion.

Sanic will guarantee deliver-once behavior of these signals for each and everyone who is subscribing to this signal

Signal Description

Signal Context Description
starting_server This event will be generated before a Sanic application has actually been initialized and starts serving the requests This signal corresponds to the current before_server_start listener event
server_started This event will be triggered as soon at the Sanic application has been initialized and is ready to start serving any and all kind of requests This signal corresponds to the current after_server_start listener event
stopping_server This event will be triggered just before the Sanic application context is about to be torn down and the server will stop serving any further API requests This signal corresponds to the current before_server_stop listener event
after_server_stop This event will be triggered as soon as the Sanic application context is torn down and the server has stopped serving the requests This signal corresponds to the current after_server_stop listener event
request_started This event will be triggered right before the Sanic is about to hand over the request object to the handler responsible for handling that specific request This signal corresponds to the behavior of tagging a middleware to the request context
request_finished This event will be triggered as soon as the handler has returned a response back to the Sanic server to be written back into the wire as the data for the request This signal correspond to the behavior of tagging a middleware to the response context
response_finished This event will be triggered once the Sanic application has finished returning the response back to the requester. This event can be handy in tracking certain metrics that related to the latency introduced by the network during the response write to the wire Currently Sanic doesn't handle this event in any way and this will be a new introduction as part of this proposal
request_exception This event will be triggered as part of an exception that occurs while handling any of the incoming requests This is similar to the @app.exception exception handler middlewares that Sanic provides.
exception_occured This event will be triggered when Sanic application context comes across any kind of error/exception that is outside the context of a request lifecycle. Currently Sanic doesn't handle this event in any way and this will be a new introduction as part of this proposal
notification This is a signal that will be provided by the Sanic framework that can be leveraged by the extension/plugin writer in order to notify other extension/plugin that there is a certain kind of event that has happened in their execution context. Sanic will define a predefined format that will have to be followed while generating this signal to ensure that the messages are compatible across the board. Currently Sanic doesn't handle this event in any way and this will be a new introduction as part of this proposal

Implementation Details

Helper Utilities

There are few interprocess signal communication/dispatch libraries available that can be leveraged.

  1. Blinker
  2. PyDispatcher
  3. PySignals
  4. NanoMsg with Python Bindings

Based on some evaluation, we can decide on the best-suited tooling to be used under the hood to bring the signals behavior into Sanic (If none of these existing items suit the needs to what is expected by sanic we can always fork one of these and extend them to match the need of sanic)

Implementation Requirements

  1. Ensure that the mechanism to generate and propagate the signal introduces little to negotiable overhead on the request lifecycle.
  2. Ensure that there is a deliver-once adherence to each of the generated signals
  3. Define strict guidelines to the data structure of the event used as part of the Signal
  4. Provide an easy to understand and use API to consume and produce signals
    1. signal_name.subscribe to start receiving an event
    2. signal_name.publish to generate a signal into the sanic infrastructure
  5. Ensure that the subscription mechanism is a non-blocking operation. Possibly by ensuring that we provided a mechanism to provide a callable function as a callback handler to tackle the event
  6. The event handling will be async by nature and there is no guarantee in the order in which the signal handlers are processed.
  7. The event handler should be read-only handlers (i.e. the signal event should not be changed in any form or shape.)
  8. A signal once generated is immutable in nature.
  9. A signal once delivered to all known subscriber will be taken out of memory and destroyed
  10. If a signal handler is registered at the runtime by a dynamic section of the code, that handler will only start receiving those signals that occur after the subscriber is initialized
  11. There is no TTL like behavior tagged to any of the signals thereby ensuring that we do not oversubscribe to the memory consumption. The message is lost in the void of space and time once it's been delivered to all available subscribers at the time of signal generation
  12. This feature is valid and available only via inter-process communication You are, however, free to subscribe to the signals and write them to your favorite message queue/bus like RabbitMQ or Redis to be consumed by a wide array of other processes that runs outside of sanic application process.

Impacted Areas

  1. Server startup and teardown mechanism
  2. Request Handler
  3. Response writer (HttpResponse)

Implementation Guidelines

  1. Add a new subpackage under sanic named signals to define all possible infrastructure level signals
  2. Ability to introduce new signal into the sanic infrastructure via a register api
  3. Ability to subscribe to non-stadard signals made available via register api defined in Split exceptions tests #2
  4. An internal auto defined metrics API (localhost:5000/signal/metrics) to provide the information on signal metrics (These metrics are generated based on the information available at that point in time so that they can easily be dumped into a time-series database)
    1. Number of signals of a specific type generated
    2. Number of subscribers registered to a given signal
    3. Pending events in the Queue that are yet to be broadcast to all the subscribers

To Do

  1. Fine tune the requirements and define implementation guidelines as required
  2. Evaluate possible utilities to support signal functionality
  3. A first working prototype for signal functionality
  4. Documentation and Example use cases
@yunstanford yunstanford changed the title Proposal : Signals Support for Sanic RFC : Signals Support for Sanic Proposal Jul 20, 2019
@ahopkins
Copy link
Member

I am in favor of this. As a quick reminder, some of these proposed signals will not really be supported in ASGI mode. And, I think we might want to work on some of the naming of them to stay consistent (ie after_server_stop >> server_stopped)

@ahopkins
Copy link
Member

In general, I think we need to also keep in mind ASGI lifespans. These really only correspond to server_starting and stopping_server since we are not in control of the failures. But, food for thought especially in the protocol being used to send messages.

@harshanarayana
Copy link
Contributor Author

@ahopkins When I did the initial draft of this, our ASGI support was still underway. I am working on an updated draft related to the ASGI lifespan and tweaking the signals accordingly and possibly altering some of their names to a more generic reusable form across the framework. I will reach out to you via Gitter for a review soon

@stale
Copy link

stale bot commented Nov 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

@stale stale bot added the stale label Nov 11, 2019
@harshanarayana
Copy link
Contributor Author

/reopen

@stale stale bot removed the stale label Nov 12, 2019
@ashleysommer
Copy link
Member

ashleysommer commented Jan 8, 2020

I've been thinking about this in relation to Sanic-Plugins-Framework.

For me the ultimate goal will be to deprecate the method-wrapping that SPF needs to do in order to add functionality to Sanic.

Specifically; when installed SPF wraps and replaces the sanic response handler, the request middleware runner, and the response middleware runner.

SPF needs to do this in order to provide the following features:

  • Creation of per-request sharable context objects before sanic request handling occurs
  • Running plugin-specific pre-request-middleware before the sanic app request-middleware runner
  • Running plugin-specific post-request-middleware after the sanic app request-middleware runner
  • Running plugin-specific pre-response-middleware before the sanic app response-middleware runner
  • Running plugin-specific post-response-middleware after the sanic app response-middleware runner
  • Executing plugin-specific (and some spf-builtin) cleanup-middleware after the sanic request handler has run

I think the first item and last item can proably be covered by the request_started and request_finished signals respectively.
However for the other items, I think there is need for these four extra signals:
before_request_middleware
after_request_middleware
before_response_middleware
after_response_middleware

@harshanarayana
Copy link
Contributor Author

@ashleysommer Thanks for the feedback. I think most of these signals can be easily included with little to no overhead in the current workflow. However, since these signals are only useful in cases when you are using SPF or a plugin built using that, I am wondering if we can think of making this a configurable entity whereby you get to say generate these additional signals and only then will sanic register those signal handler and deliver them.

This way, we can avoid the overhead of generating the signals in places where there is no SPF/it's extensions are being used. Would that be a sensible thing to do?

@ashleysommer
Copy link
Member

@harshanarayana
Yes, if it could be configurable to only generate those signals when there is a handler registered, that would be great!

@ahopkins
Copy link
Member

ahopkins commented Jan 9, 2020

Agreed. 👍 to handing this at startup.

@vltr
Copy link
Member

vltr commented Jan 14, 2020

Another helper utilities to leverage:

  • aiosignal: it's been used internally by aiohttp and are now being popped out as their own package;
  • asyncio_dispatch: the first asyncio signal dispatcher I came across and it's actually pretty good and have a good code coverage, even though it's been 4 years since it was last updated (I've used it in a couple of projects and have nothing to complain about). Bring it up to speed I guess would not be a problem.

@vltr
Copy link
Member

vltr commented Jan 14, 2020

(now I noticed we don't have any thread in the forums discussing this, but that's not a problem)

Personally, this is something I've been wanting to have on Sanic since day one. I tried to leverage this with my own Frankenstein project based on Sanic - well, it doesn't have any signal support, but does have a component system that is very handy - perhaps another subject for another time 😉

@harshanarayana
Copy link
Contributor Author

@vltr aiosignal and asyncio_dispatch seems pretty great options. Let me see how they can be fit into what we have in mind. I might need your suggestions on the asyncio_dispatch since you have already put it to some use.

@vltr
Copy link
Member

vltr commented Jan 16, 2020

@harshanarayana I really liked asyncio_dispatch because, at the time, it was the only solution available out of the box (aiosignal was not a thing yet) and reminded me a lot the Signals and Slots from Qt, which I used a lot in the past. I even created a small Sanic "example" with asyncio_dispatch:

import asyncio

from sanic import Sanic
from sanic.log import logger
from sanic.response import text

from asyncio_dispatch import Signal


app = Sanic(__name__)


async def my_callback(signal, senders, keys, action):
    await asyncio.sleep(3)
    if action is not None:
        logger.info(
            f"my callback called after 3 seconds says action should be: "
            f"{action}"
        )


@app.listener("before_server_start")
async def setup_signal(app, loop):
    app.signal = Signal(loop=loop, action=None)
    await app.signal.connect(my_callback)


@app.get("/")
async def test_async(request):
    await request.app.signal.send()
    return text("hello, world!")


@app.post("/")
async def test_async_post(request):
    if "action" in request.json:
        await request.app.signal.send(action=request.json.get("action"))
    return text("hello, world!")


if __name__ == '__main__':
    app.run(host="0.0.0.0", port=8000, workers=1)

Calling using cURL:

$ curl -d '{"action":"FOO"}' -H "Content-Type: application/json" http://localhost:8000/
hello, world!

Server output:

[2020-01-16 12:14:15 -0300] [99804] [INFO] Goin' Fast @ http://0.0.0.0:8000
[2020-01-16 12:14:15 -0300] [99804] [INFO] Starting worker [99804]
[2020-01-16 12:14:18 -0300] - (sanic.access)[INFO][127.0.0.1:43120]: POST http://localhost:8000/  200 13
[2020-01-16 12:14:21 -0300] [99804] [INFO] my callback called after 3 seconds says action should be: FOO

I think I have this example laying around my computer for 6 months (or more) 😬

@abuckenheimer
Copy link
Contributor

abuckenheimer commented Jan 20, 2020

I'm very much +1 on this idea, definitely something I've found myself struggling with in the past. A few thoughts:

Signal Handler Order matters

There's some nuance in deciding in what order signal handlers get called. If your trying to evaluate an accurate wall time for a request handler you need your request_started to be the last signal handler to fire before the request handler but the request_finished handler to be first to fire after it (LIFO). On the other hand there are probably plenty of use cases where your okay with your handler getting called concurrently with others (async requests to other networked services?). I think to be successful here we have to expose ordering as part of the api but maybe the default is an unordered (i.e. concurrent) bucket of handlers.

Turtles all the way down

This idea taken to its logical maximum can get kind of interesting, like technically request handlers are really just signal handlers that fire on the request.on_body event. @Tronic and I were playing around with some ideas to work around the awkwardness of streaming responses in #1645 and #1661, you could make an argument that maybe the user should be able to opt into finely chopped request events like on_headers_complete, on_message_complete, on_response_headers. Then streaming becomes

@app.on("/", "message_complete"):
def make_headers(request):
    return sanic.Headers(foo="bar")

@app.on("/", "response_headers"):
async def make_body(request, response):
    assert response.headers['foo'] == "bar"
    await response.write("baz")
    ...

To be clear I'm not suggesting we encourage this style of api for our users but there are some interesting possibilities. It be encouraging if this was generic and easy enough to use that the sanic implementation made heavy use of it. Access logs for example is something I'd expect to see as a basic signal handler that we offer as part sanic.

Who watches the watchers

Going to a signal heavy implementation does introduce some complexity in understanding the performance of your application. You could logically want to create a signal handler that tries to collect metrics / monitor performance on all signal handlers to add transparency to your app but does that justify the cost of a nested signal handler?

Back Pressure

However this gets implemented this it seems like were going to have to keep events around in memory while potentially multiple handlers are working on them (or catching up to them). There has to be some way to control how many events we can queue up for a slow handler and push that back to the things queuing them.

Versioning

The context passed to a signal handler is probably going to change over time, should we try to version this in some way so we can maintain backwards compatibility easier?

Naming Bikesheds

@ahopkins posted the link to the asgi lifespans doc and it looks like their signals have "namespaces" (seems like in convention only), I would be +1 on stealing that idea, something like turning request_started -> http.request.start, server_started -> server.start. I also kind of find "starting"/"started" a bit awkward because it looks similar but understanding the difference is important, so maybe server_starting -> server.init?

Thanks 😄

Huge shout out to @harshanarayana for kicking this off though, really excited to see where this goes!

@Tronic
Copy link
Member

Tronic commented Jan 22, 2020

How do you plan to run them concurrently? I am concerned because AFAIK you'll have to depend on asyncio for that, and that would require an alternative implementation for trio.

@harshanarayana
Copy link
Contributor Author

@abuckenheimer I like the idea of associating a namespace with the signal. It makes it more cleaner and has a definitive association. I'll include that in the changes I am working on.

@Tronic Right now, the idea is to schedule them as tasks when sending the events. I still need to explore better options if any. Or follow the serial approach for notification otherwise. I am open to suggestions or additional tooling for the parallel notification mechanism

@vltr
Copy link
Member

vltr commented Jan 22, 2020

Erm ... I'm probably ignorant enough regarding trio to talk about it's implementations, pros and cons ... All I remember is that its "nursery" concept works really well and would be awesome to have the same functionality inside asyncio (I'm just reporting what I heard), but ... To worry about making things inside Sanic to work with trio would be the same as making Sanic to work with twisted, which I see no point (or I'm missing the point) ... I'm a bit confused.

@Tronic
Copy link
Member

Tronic commented Jan 22, 2020

Sanic already works with Trio using Hypercorn because Sanic's ASGI mode does not depend on asyncio (except for file responses, which were easily fixed via a few lines of code in compat.py). I'd like to keep that compatibility and preferably not add too much new compatibility code to support it.

@vltr
Copy link
Member

vltr commented Jan 22, 2020

Ok, so, just to clarify things a little bit more: hypothetically, the signals support on Sanic would work on top of ASGI with any level of complexity on top of asyncio and would work with hypercorn using the asyncio or uvloop workers, but not with the trio worker? Or would not work at all with ASGI? I know there are already limitations to some listeners in ASGI, I'm just wondering if this would be another case.

@vltr
Copy link
Member

vltr commented Jan 23, 2020

Well, laziness of mine, I did some quick tests using a modified version of the example I shared here previously to run with daphne, hypercorn and uvicorn:

import asyncio

from sanic import Sanic
from sanic.response import text

from asyncio_dispatch import Signal


app = Sanic(__name__)


async def my_callback(signal, senders, keys, action):
    await asyncio.sleep(3)
    if action is not None:
        print(
            f"my callback called after 3 seconds says action should be: "
            f"{action}"
        )


@app.listener("after_server_start")
async def setup_signal(app, loop):
    app.signal = Signal(loop=loop, action=None)
    await app.signal.connect(my_callback)


@app.get("/")
async def test_async(request):
    await request.app.signal.send()
    return text("hello, world!")


@app.post("/")
async def test_async_post(request):
    if "action" in request.json:
        await request.app.signal.send(action=request.json.get("action"))
    return text("hello, world!")

The result is already interesting:

  • daphne did not worked at all because the listener was never called and, well, I think @ahopkins already caught why;
  • hypercorn does work only if using asyncio or uvloop workers; using trio I silently get nothing (from the signal I created), but still get my endpoint output;
  • uvicorn: worked with all event loops (except for iocp because I was not able to test it).

Now I don't know if I can start crying already 😅

@ahopkins
Copy link
Member

ahopkins commented Aug 3, 2020

I am pushing this one forward. It can be a really powerful tool for Sanic. Here are some thoughts after playing with the existing PR (and moving it to #1902 so it is on a branch here, and not a fork)

  • I think we should scrap the usage of a 3rd party library. The complexity saved is not that much, and we can gain a lot by just maintaining the implementation of the Signal ourselves
  • I like the namespace, context, action implementation, and think we should run with it
  • This should carry both a usable API for internal event handling, but also customization and extensibility
  • We will replace both the @app.listener and @app.middleware functionality and make those callers of the appropriate signals
  • This needs to work on app, bp, and bp_group
  • The callable should have the following variables injected IF they exist in the argspec: app, loop, signal
  • Potentially routing for this could be abstracted into a base class that could be the beginning of an overhaul of Sanic's router. With that said, I think we should think about maybe something like the AST based solution used by falcon that has proved to be extremely fast. Depending upon how signals are implemented, this could very well replace the router completely and even turn @app.request into a mere signal wrapper, instead of its own thing. If this is the case, then the extra args would also need to be injectable into the handlers.

With that said, the API I am proposing would look something like this:

@app.on('server.init.before')  # would be synonymous with @app.listener("before_server_start")
@app.on('http.request.start')  # basically the same as middleware
@app.on('http.request.start', restrictions=restrictions)  # where restrictions could limit dispatch on conditions, in this case maybe it is a path or a route name
@app.on('custom.email.send')  # custom namespace is for extensibility

# Both forms would be supported. The first probably would be preferred since the routing is explicit and needs and should be faster
app.signals.dispatch('custom', 'email', 'send', extra={})  # extra would get passed and be available on the `signal` instance
app.signals.dispatch('custom.email.send', where=where)  # where should match up with the restrictions for limiting

@ahopkins ahopkins added this to the v21.6 milestone Jan 11, 2021
@ahopkins
Copy link
Member

ahopkins commented Feb 1, 2021

See also: #2015 (comment)

@ahopkins
Copy link
Member

This issue has been mentioned on Sanic Community Discussion. There might be relevant details there:

https://community.sanicframework.org/t/a-fast-new-router/649/41

@ahopkins
Copy link
Member

ahopkins commented Feb 11, 2021

As mentioned in the linked discussion, I created a super simple proof of concept signal component for Sanic with an API that looks like this:

@app.signal("foo.bar.baz")
async def signal_handler(**kwargs):
    ...

@app.listener("after_server_start")
async def wait_for_event(app, loop):
    while True:
        print("> waiting")
        await app.event("foo.bar.baz")
        print("> event found")


@app.get("/")
async def trigger(request):
    await app.dispatch("foo.bar.baz")
    return response.text("Done.")

This is a super simple API implementation. A lot of the heavy work is still being done by the router. I hope to finalize #2010 PR this weekend and pull it out of draft. Therefore, it might be feasible to bump up this feature to include the API for signals in 21.3. If that is the case, I would not transition any existing functionality (middleware, listeners, etc), but just include the new API.

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

@jordangarside
Copy link

QQ: response_finished isn't implemented yet, and this will be the general way to do some post-response hook right?

@ashleysommer
Copy link
Member

@jordangarside
PR #2042 added a comprehensive signals infrastructure and api surface, to enable the ability to emit, receive, and wait on signals in Sanic.
However this #2042 does not introduce any of the new system-level signals yet.
See comment from @ahopkins above:

Therefore, it might be feasible to bump up this feature to include the API for signals in 21.3. If that is the case, I would not transition any existing functionality (middleware, listeners, etc), but just include the new API.

New system-level signals that use this new API will start to appear from Sanic version 21.6 and above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants