Signals `http.middleware.before` and `http.middleware.after` are dispatched for _every_ middleware

[ashleysommer]

For a single request, the signals http.middleware.before and http.middleware.after are dispatched for every middle on the app.

Example app:

from sanic import Sanic
from sanic.response import html

app = Sanic(__name__)

@app.middleware("request")
def my_middleware1(request):
    print("Middleware 1 ran", flush=True)

@app.middleware("request")
def my_middleware2(request):
    print("Middleware 2 ran", flush=True)

@app.signal("http.middleware.before", condition={"attach_to": "request"})
def handle_mw_before(request, response=None):
    print("Before middleware", flush=True)

@app.signal("http.middleware.after", condition={"attach_to": "request"})
def handle_mw_after(request, response):
    print("After middleware", flush=True)

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

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

Output:

[2022-01-04 13:57:50 +1000] [47672] [INFO] Starting worker [47672]
[2022-01-04 13:57:54 +1000] [47672] [DEBUG] Dispatching signal: http.middleware.before
Before middleware
Middleware 1 ran
[2022-01-04 13:57:54 +1000] [47672] [DEBUG] Dispatching signal: http.middleware.after
After middleware
[2022-01-04 13:57:54 +1000] [47672] [DEBUG] Dispatching signal: http.middleware.before
Before middleware    #<--- Run again
Middleware 2 ran
[2022-01-04 13:57:54 +1000] [47672] [DEBUG] Dispatching signal: http.middleware.after
After middleware     #<--- Run again

Applicable section of code: https://github.com/sanic-org/sanic/blob/a7bc8b56bab01e066357e6dcc67c0dc9df864298/sanic/app.py#L1346-L1372

It appears this is done intentionally. I don't know what the usefulness of running a handler for every middleware is, except for perhaps debug tracing or handler logger.

Maybe it makes sense to create two new signals like http.middleware.before_all and http.middleware.after_all, in a place like this:

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

[ahopkins]

Correct, that is intentional. Out of curiosity, what is the use case that you are after with dispatching once before and after middleware? Is it to have control over when something executes? There are certainly other signals you could potentially use if you need to do something earlier, like when the request is created, or routing is performed. Will these help? Or, are you particularly interested in creating a sort of "priority" middleware?

The reason I ask is that there is a need for a bit of a revamp in middleware this year anyway. We have been talking about this for a long time now, and I think with some of the other improvements we made last year, we are finally getting close to being there. There are two main changes I think we should make:

1. precompute middleware and merge it into the handler itself
2. add an API for prioritizing middleware and not relying upon definition ordering

[ashleysommer]

I am trying to emulate the capabilities of Sanic-Plugin-Toolkit without monkey-patching, using the signals mechanism.

I need to be able to run a queue of registered plugin middlewares before the application's middlewares are run.

Putting it on "http.lifecycle.handle" does not make sense because the router hasn't run yet.

The most logical place to put it would be on "http.routing.after", but at that stage recv_body() hasn't been called yet, so requests have no body.

Given it is specific to a middleware use-case, that is why I proposed a signal called "http.middleware.before_all".

This leads into the other conversation in #2353 where I need to be able to check if any of those plugin middlewares produced an early response, and that needs to be passed back from the signal handler, through the signal dispatcher, to the calling site. That is not possible so I think this needs to wait until a revamp of the middleware stack.

[ahopkins]

Actually, I think better than http.middleware.before_all would be http.lifecycle.body that executes here after reading the body.

[ashleysommer]

Yeah, I thought an alternative would be after the receive_body() call.

I think middleware.after_all could be provided by http.lifecycle.respond.

[ahopkins]

We should probably also include a lifecycle diagram in the docs with the flow of handlers, middleware, and signals.

[stale[bot]]

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.

[ChihweiLHBird]

Actually, I think better than http.middleware.before_all would be http.lifecycle.body that executes here after reading the body.

I think (tested) current version of Sanic doesn't read the body if the request type is GET without any payload in the body and hence won't trigger the event http.lifecycle.read_body...

Like

app = Sanic("MyHelloWorldApp")


@app.get("/")
async def hello_world1(request):
    return text("Hello, world.")

@app.signal("http.lifecycle.read_body")
async def http_lifecycle_read_body(body):
    print("http.lifecycle.read_body")

app.run()

[ChihweiLHBird]

Never mind, because http.lifecycle.read_body is triggered inside read function, and we can still add another trigger for http.lifecycle.body somewhere after the code that is reading the body, even in the case that the body wasn't really read, it will still trigger the signal. But it remains a little bit weird to call the signal body.

[stale[bot]]

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.