Middlewares ordering

[tomaszdrozdz]

Right now request middlewares are called according to order in which they were added
and response middlewares in reverse order.
I think it could be beneficial if we had possibility to explicitly set order for calling middlewares.

What do You think, Yes or No and why ?

[ahopkins]

@app.middleware("request")
async def middleware1(request):
    print("middleware1")


@app.middleware("request")
async def middleware2(request):
    print("middleware2")


@app.middleware("response")
async def middleware3(request, response):
    print("middleware3")


@app.middleware("response")
async def middleware4(request, response):
    print("middleware4")

middleware1
middleware2
middleware4
middleware3

A few releases back this changed.

[ahopkins]

On the ordering issue...

I did get into a discussion with someone not too long ago about something like this:

@app.middleware("request", priority=20)

The idea is okay. My concern here would be that it happen only at startup. Not at execution time. Which then begs the question if it is actually helpful or not.

[tomaszdrozdz]

Currently it is exactly how I read in documentation: request middlewares in order they were registered, and response middlewares in reverse order.

I think about something almost like above:

@app.middleware("request", priority="20")

Yes - I would prefer string not int because for example some "miniapp/ blueprint" could have priorities for example like: "10.1", "10.2", "10.3" - anyway it seams to be more flexible.

And I think that you register middleware as above,
but to really sort them (at any wanted time, even at runtime, or just before app.run()) you have to

app.sort_middleware()

or maybe better app.run() does it anyway,
and you can always call app.sort_middlewares() at any time.

[ahopkins]

Currently it is exactly how I read in documentation: request middlewares in order they were registered, and response middlewares in reverse order.

Then I am a moron. 😒 I had in my mind they were both reverse.

If we were to add something like this, then I would not require a type. My biggest hesitancy is that this seems like it has the potential for some crazy scope creep. What if I want dynamic priority? What if I want a custom sort key? What if I want ad hoc sorting (as you suggested)?

[tomaszdrozdz]

Also the same story for listeners.
Now
"before_server_start", "after_server_start" listeners are executed in order they were registered
but "before_server_stop", "after_server_stop" listeners are executed in reverse order.

But we could also add priority parameter to app.listener('before_server_start', priority="A.a").

About Yours hesitancy - You are right.
But I am more worried that You can "accidentally" change order of middlewares/ listener for example by changing import order of your modules/ blueprints/ ...

So maybe it is better to take it one day at a time.
So just implement it simply - just ordering withough any extra functionality You described.
If someone wants to have it, then someone should describes accurately what he wants, then we can deal with it when we get to it.

Just one thing - it should also be compatible with what we have already
so for example there should be some default priority
and multiple middlewarres/ listeneres can "be marked" with the same priority in which case the same rules as now apply,
for example all response middlewarres with priority "C.2" are called in reverse order to registering them.

[ahopkins]

Backwards compat is a must. I would suggest making it even simpler by using itertools.count.

As new items are registered, just add a priority if not explicitly passed. Sort based on that. Just make sure we capture any TypeError if there is a mismatch in types when sorting.

[ahopkins]

If you really want to support string based priority, then make a method on sanic.Sanic called _generate_next_priority_ similar to Enums. By default it could generate numbers using itertools, but easily be overriden if needed.

[tomaszdrozdz]

OK.
Another thing I realized.

But I believe it is better to discuss everything we are aware of then just do things wrong.

---

Now we have request_middleware and named_request_middleware.
First middlewares from request_middleware are executed
and later middlewares from named_request_middleware['name_ot_route'].

So if we leave it like that then middleware ordering in general will not work as expected - it will work only within middlewares from named_request_middleware['name_ot_route'] scopes sepparattely.

Right now we have:

request_middleware = [m1, m2, m3]

named_request_middleware = { 'some_route1': [mA, mB, mC],
                             'some_route1': [mX, mY, mZ] }

I wonder why we have two separates request_middleware and named_request_middleware ??? (and what was reasons behind it)

I believe it would be better to have just one like:

request_middleware = [ ('*', ma),
                       ('*', mb),
                       ('*', mc),
                       ('some_route1', mA),
                       ('some_route1', mB),
                       ('some_route1', mC),
                       ('some_route2', mX),
                       ('some_route2', mY),
                       ('some_route2', mZ) ]

Then we could even set something like (what would provide even more flexibility and freedom):

                       ('some_route[a-zA-Z]+', mM)

Then sorting would be easy,
executing middlewares by matching routes also could be easily achieved with filter() and regexp.

[ashleysommer]

If I recall correctly, we discussed this a year or two ago when it came up then. We concluded the current order as-documented is the correct order (and I agree).
But as @tomaszdrozdz pointed out, we didn't have "named" middleware back then. When "named" middleware was implemented, I don't know if ordering of middleware was considered.
I think two separate queues (like request_middleware and named_request_middleware) was used to maintain backward compatibility with plugins or other subsystems that rely on request_middleware to keep its existing implementation.

I will mention here though, that the "Enhanced Middleware System" of Sanic Plugins Framework has the ability to set priorities on Middleware as a core feature, and it used by lots of Sanic Plugins.
Maybe there is a way @tomaszdrozdz you can utilize that for your needs?

[ahopkins]

Also keep in mind we are on a breaking change freeze through the end of the year. Any change that will not be backwards compat must wait until 2021.

[tomaszdrozdz]

@ahopkins @ashleysommer

So the way things are (I stress this is just my point of view):

1. We had middlewares (request_middleware). Then we added functionality to request middleware and attach them to given path (named_request_middleware), which was step towards right direction. But we did not merged together those both solutions. And I believe this was unfortunate ommision. One of symptom proclaiming about this is mess in documentation regarding middlewares.

2. Many ask about possibility to prioritize middlewares. For many peoples reverse priority of executing resposnse middlewares is odd, or at least not intuitive. And even You see the necessity for such functionality - you implemented it in your plugin. But I believe it should be strictly possible when adding Sanic middlewares - as this functionality is simply important.

3. I believe it all shows us we should rethink, refactor, simplify/reduce while improve/enhance.

4. I can try to do it. Even if you will not accept new approach eventually. But I will need Sanic developers community support and help. And I need Your approval for this refactoring.

5. And it is not one day work - and I preferably would not expect it to be ready before 2021 - even if it is, we can wait with merging.

[ahopkins]

I will respond in more details later today. First, I thank you you for the offer to put in the work on this.

However, there is likely to be some significant changes on this coming Q1. So any work you put in right now might become moot.

[ahopkins]

I think the ideas here (in general) need consideration and implementation. I think we should put this on hold until later in the year. This is itself dependent upon the signaling API that is in the works that will ultimate control the attaching and ordering of middleware.

[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]

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