Ability to check response from middleware signal handlers using modified context

[ashleysommer]

It seems that the only way to return a value from a signal handler is to modify or add a value in context dict. That makes sense, because there might be many handlers for a single signal, and each of them may or may not modify the context.

However in the sanic middlware handler code it appears that these returned context values are never used. Eg, when handling a http.middleware.before signal, the response value of the context is initially None, and your handler can create a response object and set it into the context. However the code in _run_request_middleware and _run_response_middleware does not check the context after the signals are run, so the response from the signal handler is ignored.

Kind of related to #2352

Example: I'd like to be able to make the signal handler act like a super-middleware:

from sanic import Sanic
from sanic.response import html

app = Sanic(__name__)

@app.signal("http.middleware.before", condition={"attach_to": "request"})
def handle_mw_before(context):
    request = context.get("request")
    ...  # some handler logic
    resp = html("<p>Overridden by handler</p>")
    context.set("response", resp)

@app.get("/")
def index(request):
    return html("<p>hello world</p>")

if __name__ == "__main__":
    app.run(debug=True, auto_reload=False, access_log=False)

This would require changes to _run_request_middleware and _run_response_middleware something like this:

        if applicable_middleware and not request.request_middleware_started:
            request.request_middleware_started = True

            for middleware in applicable_middleware:
                context = { "request": request, "response": None }
                await self.dispatch(
                    "http.middleware.before",
                    inline=True,
                    context=context,
                    condition={"attach_to": "request"},
                )
                response = context["response"]
                if isawaitable(response):
                    response = await response
                if not response:
                    response = middleware(request)
                    if isawaitable(response):
                        response = await response
                context["response"] = response
                await self.dispatch(
                    "http.middleware.after",
                    inline=True,
                    context=context,
                    condition={"attach_to": "request"},
                )
                response = context["response"]
                if isawaitable(response):
                    response = await response
                if response:
                    return response

[ashleysommer]

Note, for context, I'd like to have the ability to combine this with a the "http.middleware.before_all" signal as suggested in #2352, to potentially return a HTTPResponse using a signal handler before any middleware is run, and even if applicable_middleware is None.

[ashleysommer]

Ok, after writing some more example code, I realized this cannot work the way I imagined.
The context dict is unpacked into the signal handler:

@app.signal("http.middleware.before_all", condition={"attach_to": "request"})
def handle_mw_before(**context):
    request = context.get("request")
    ...  # some handler logic
    resp = html("<p>Overridden by handler</p>")
    context.set("response", resp)

The context dict in the handler becomes a new dict, so setting "response" on the context does not change the context at the calling point.

This will have to be put on the backburner

[ahopkins]

This is an interesting idea for sure. And, I think can be made a part of some overhaul to the middleware/handler flow in general. As I sort of mentioned in the other thread, this is something that I think should be on our radar for this year. Maybe more in the mid-year release timeframe.

This raises two points in my mind:

1. I am not sure that we really want this. This is an extra check on every request for a small use case. I think a more optimized middleware would be more appropriate for that use case.
2. You should be able to do something like this even today. Albeit, it is not a public API per se, and maybe it should be.

@app.signal(Event.HTTP_LIFECYCLE_HANDLE)
async def early(request: Request):
    # Depending upon which event you use, this will never actually be triggered. 
    # There should always be a request.stream. But, mypy wants the case 
    # covered since it starts out as None and it is not aware of when this 
    # will be triggered.
    if not request.stream:
        raise SanicException("No request stream")

    # You can force a response like this
    request.stream.respond(json({"early": True}))
    request.responded = True

Granted, since you are going outside the normal boundaries, you end up with this ugly in your logs, but it still will send to your client fine:

[2022-01-04 10:11:48 +0200] [2630172] [ERROR] The response object returned by the route handler will not be sent to client. The request has already been responded to.

[jonra1993]

Hello, @ahopkins One question relates to context. I was doing some testing with app.ctx I see the context is independent for each worker. Is there any way to have a shared context between workers? I have used Multiprocessing Manager as described here #1270 but it just works for not complex data structures like objects

[ahopkins]

Is there any way to have a shared context between workers?

I'm not sure why this is part of this discussion. Seems unrelated.

Regardless, what you are describing is the functionality of DBs. Of course it takes some work to monitor the dB instances and hydrate your app state. This is of course possible and I do something similar fairly often.

A simpler course of action is @ashleysommer package he just released. I have not used it myself yet, but it looks to do esactly what you are asking about.

https://github.com/ashleysommer/sanic-synchro-ctx/

[jonra1993]

Thanks for the information @ahopkins

[stale]

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]

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.

[ahopkins]

Hello, @ahopkins One question relates to context. I was doing some testing with app.ctx I see the context is independent for each worker. Is there any way to have a shared context between workers? I have used Multiprocessing Manager as described here #1270 but it just works for not complex data structures like objects

FWIW - #2499 Adds an API for this to be released in v22.9